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

["lihuisong (C)" <lihuisong@huawei.com>]

在 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?
>
>
>
> .