On Tue, 19-11-05, 11:11, Thomas Monjalon wrote:
> Hi Jack,
>
> If it is a fix, please change your title.
>
Ok, I will add 'fix' in title.
> 05/11/2019 10:16, Xiaoyu Min:
> > When rte_flow_expand_rss expands rte_flow item list based on the RSS
> > types.
>
> There is no verb in this sentence.
>
It seems I missed some words here. I'll update it.
> > In another word, it will add some rte_flow items if the user
>
> What is "it"?
>
I mean rte_flow_expande_rss.
> > specified items are not complete, for example:
> >
> > ... pattern eth / end actions rss type tcp end ...
>
> Please explain why it is not complete.
>
What I mean is user provides above instead of:
... pattern eth / ipv6 / tcp / end actions rss type tcp end ...
This one is complete.
> >
> > above flow will be expaned to:
> >
> > ... pattern eth / end actions rss types tcp
> > ... pattern eth / ipv4 / tcp / end actions rss types tcp ...
> > ... pattern eth / ipv6 / tcp / end actsion rss types tcp ...
>
> There are several typos in this text. Please check.
>
Sure.
> > However the expansion is just simply expanding items without
> > checking last items' spec, means the expanded rules could have conflicting
> > settings which is not reasonable and leads to some HW error, for
> > example:
>
> This wording is really not clear.
> Please make short sentences.
>
OK, I'll make it shorter.
> > ... pattern eth type is 0x86DD / end actions rss types tcp ...
> >
> > is expaneded to:
> >
> > ... pattern eth type is 0x86DD / ipv4 / tcp end ...
> >
> > which has conflicting values: 0x86DD vs. ipv4 and HW will refuse create
> > flow on some NICs.
> >
> > This patch will fix above by checking the last item's spec and try to
> > complete the item list.
> >
> > Currently only support to complete item list based on L2/L3 layer.
> >
> > Fixes: 4ed05fcd441b ("ethdev: add flow API to expand RSS flows")
> > Cc: sta...@dpdk.org
>
> Missing empty line here to separate blocks.
>
Ok, I'll add empty line here.
> > Signed-off-by: Xiaoyu Min <jack...@mellanox.com>
> > ---
> > lib/librte_ethdev/rte_flow.c | 132 +++++++++++++++++++++++++++++++++--
> > 1 file changed, 127 insertions(+), 5 deletions(-)
>
> It's a big change. It needs to be carefully reviewed.
>
>
