Re: [PATCH 2/6] net/hns3: fix inconsistent enabled RSS behavior

[Thomas Monjalon <thomas@monjalon.net>]

23/03/2022 04:05, lihuisong (C):
> 在 2022/3/23 1:13, Thomas Monjalon 写道:
> > 21/03/2022 08:14, lihuisong (C):
> >> 2022/3/10 16:08, lihuisong (C):
> >>> 2022/3/9 17:55, Ori Kam:
> >>>> From: lihuisong (C) <lihuisong@huawei.com>
> >>>>> 2022/3/3 10:47, lihuisong (C):
> >>>>>> 2022/3/2 22:07, Ori Kam:
> >>>>>>> From: lihuisong (C) <lihuisong@huawei.com>
> >>>>>>>> 2022/3/1 0:42, Ferruh Yigit:
> >>>>>>>>> On 2/28/2022 3:21 AM, Min Hu (Connor) wrote:
> >>>>>>>>>> From: Huisong Li <lihuisong@huawei.com>
> >>>>>>>>>>
> >>>>>>>>>> RSS will not be enabled if the RTE_ETH_MQ_RX_RSS_FLAG isn't be
> >>>>>>>>>> set in
> >>>>>>>>>> dev_configure phase. However, if this flag isn't set, RSS can be
> >>>>>>>>>> enabled
> >>>>>>>>>> through the ethdev ops and rte_flow API. This behavior is
> >>>>>>>>>> contrary to
> >>>>>>>>>> each
> >>>>>>>>>> other.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: c37ca66f2b27 ("net/hns3: support RSS")
> >>>>>>>>>> Cc: stable@dpdk.org
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> >>>>>>>>> Hi Huisong, Connor,
> >>>>>>>>>
> >>>>>>>>> Let's get a little more feedback for this patch, cc'ed more people.
> >>>>>>>>>
> >>>>>>>>> To enable RSS, multi queue mode should be set to
> >>>>>>>>> 'RTE_ETH_MQ_RX_RSS_FLAG'.
> >>>>>>>>>
> >>>>>>>>> But I wonder if it is required to configure RSS via flow API,
> >>>>>>>> I do not know the original purpose of adding the RSS
> >>>>>>>> configuration in
> >>>>>>>> flow API.
> >>>>>>>>
> >>>>>>> The purpose is simple, this allow to create RSS per rule and not a
> >>>>>>> global one.
> >>>>>>> For example create RSS that sends TCP to some queues while othe RSS
> >>>>>>> will send
> >>>>>>> UDP traffic to different queues.
> >>>>>> I'm a little confused now. The "per rule" also seems to be a global
> >>>>>> configuration.
> >>>>>> Example:
> >>>>>>    - start PMD with 0,1,2,3
> >>>>>>    - create TCP packets to 2,3 queues. At this moment, only 2,3 queues
> >>>>>> can be received for other types of packets.
> >>>>>> Because this rule is implemented by modifying the entry of the
> >>>>>> redirection table which is global for this device.
> >>>>> Hi, Ori and Stephen.
> >>>>> Can you help me clear up the confusion above? If some NICs behave like
> >>>>> this, what should we do about it?
> >>>> I'm not sure I understand the issue, maybe it is releated to some
> >>>> HW/PMD limitation.
> >>>> In your example non TCP traffic will be routed to one of the 4 queues
> >>>> (0,1,2,3),
> >>>> While TCP traffic will only be routed to queues 2,3.
> >>>>
> >>>> Now I can add new rule that matches on UDP packet and RSS to queue 0
> >>>> and 3 in this case:
> >>>> TCP packets will be routed to queues 0,3.
> >>>> UDP packets will be routed to queues 2,3.
> >>>> All the rest of the traffic will be routed to queues 0,1,2,3
> >>>>
> >>>> And just to be clear if now I add a rule to match all packets in
> >>>> higher priority,
> >>>> with RSS to queues 1,2. Then all traffic will be routed to queues 1,2.
> >>>>
> >>>> At least this is what is expected, from API point of view.
> >>>>
> >>>> Best,
> >>>> Ori
> >>> Thank you for your answer. I understand it.
> >>> hns3 PMD cannot implement the above functions due to hardware limitation.
> >>> we may need add a check that specified RSS queues cannot be supported
> >>> when specified packets types.
> >>> And only the packet type is specified, which meets the requirements of
> >>> rte_flow API.
> >>> The check for the RTE_ETH_MQ_RX_RSS_FLAG flag in rte_flow is not correct.
> >>> Thanks, Ori and Stephen😁
> >>>
> >>> But, I think, it is necessary for the '.rss_hash_update' and
> >>> '.reta_update' APIs
> >>> in eth_dev_ops to verify this flag. What do you think? @Thomas,
> >>> @Ferruh, @Ori and @Stephen.
> >> What's your take on it? I am looking forward to your reply. Thanks!
> > 
> > I am not sure why you want to check this flag.
> 
> I want to make sure that the behavior that PMD configured RSS is
> consistent across different interfaces. The RTE_ETH_MQ_RX_RSS_FLAG
> flag is a switch to enable RSS hash. If the switch isn't open, some
> PMD do not configure RSS function. I think the consistency is necessary.
> If not set RSS muti-queue mode, it is unnecessary to configure RSS.
> 
> > I can imagine we configure the hash and the table before enabling RSS
> > with the RTE_ETH_MQ_RX_RSS_FLAG flag.
> 
> The flag is derived from dev_configure() which also configures
> hash and key. I don't think it makes sense to configure hash and
> reta before calling dev_configure. Because they'll be updated.
> This is similar to configuring mtu.

OK I see your point.
So you would like to return an error in RSS functions
if the flag RTE_ETH_MQ_RX_RSS_FLAG is not set?
Should it be checked in ethdev library or PMDs?