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

[Ajit Khaparde <ajit.khaparde@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 5721 bytes --]

On Wed, Mar 23, 2022 at 12:04 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 23/03/2022 12:04, lihuisong (C):
> > 在 2022/3/23 17:14, Thomas Monjalon 写道:
> > > 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?
> > Yes
> > > Should it be checked in ethdev library or PMDs?
> > I think it's better to put it in the ethdev layer if we do it.
> > Should we add this check in 'rss_hash_update' and 'reta_update' APIs?
>
> I'm OK with adding the check in ethdev.
+1

Apologies for getting late to this thread.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4218 bytes --]