[dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration

oulijun oulijun at huawei.com
Fri Oct 16 12:04:09 CEST 2020



在 2020/10/16 7:53, Ferruh Yigit 写道:
> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>> From: Ferruh Yigit <ferruh.yigit at intel.com>
>>>>>> On 10/15/2020 1:41 PM, Lijun Ou wrote:
>>>>>>> When start the testpmd, the pmd driver initializes the RSS
>>>>>>> configuration. Generally, the recommended RSS hash key is used as
>>>>>>> the default key in the driver. In addition, the default key is
>>>>>>> different from the default RSS flow in testpmd without specifying
>>>>>>> RSS hash key. So. if you do not specify the RSS key when creating
>>>>>>> an RSS rule, the testpmd uses the default key as the default RSS
>>>>>>> key of the RSS rule. As a result, you may mistakenly consider that
>>>>>>> the RSS key in use is the valid default key of the NIC, actually,
>>>>>>> the key and the valid default key of the NIC are two values.
>>>>>>>
>>>>>>> Consider the follow usage with testpmd:
>>>>>>> 1. first, startup testpmd:
>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>> RSS functions:
>>>>>>>     all ipv4-frag ipv4-other ipv6-frag ipv6-other ip RSS key:
>>>>>>> -
>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA3803
>>> 0F
>>>>>>> -20C6A42B73BBEAC01FA
>>>>>>> 2. create a rss rule
>>>>>>> testpmd> flow create 0 ingress pattern eth / ipv4 / udp / end
>>>>>>> testpmd> actions rss \
>>>>>>> types ipv4-udp end queues end / end
>>>>>>>
>>>>>>> 3. show rss-hash key
>>>>>>> testpmd> show port 0 rss-hash key
>>>>>>> RSS functions:
>>>>>>>     all ipv4-udp udp
>>>>>>> RSS key:
>>>>>>> -
>>> 74657374706D6427732064656661756C74205253532068617368206B65792C
>>> 206F
>>>>>>> -76657272696465
>>>>>>>
>>>>>>> In order to solve the above problems, it use the NIC valid default
>>>>>>> RSS key instead of the testpmd dummy RSS key in the flow
>>>>>>> configuration when the RSS key is not specified in the flow rule.
>>>>>>> If the NIC RSS key is invalid, it will use testpmd dummy RSS key 
>>>>>>> as the
>>> default key.
>>>>>>>
>>>>>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow
>>>>>>> API")
>>>>>>> Cc: stable at dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Lijun Ou <oulijun at huawei.com>
>>>>>>> Reviewed-by: Phil Yang <phil.yang at arm.com>
>>>>>>> ---
>>>>>>> V4->V5:
>>>>>>> -rewrite the commit log
>>>>>>> -add reviewed-by
>>>>>>
>>>>>> Hi Lijun,
>>>>>>
>>>>>> There were multiple other comments, it seems they are not addressed
>>>>>> but only updated the commit log, can you please check comments to
>>> prev versions.
>>>>>>
>>>>>> Before going into the details, my question was what happens if
>>>>>> default key not provided at all?
>>>>>> It seems this has been already tried by Ophir [1], later reverted
>>>>>> back [2] bringing the initial issue back.
>>>>>>
>>>>>> According commit, the reason of revert is to support following
>>> command:
>>>>>> "flow create 0 <pattern> actions rss queues 0 1 end key_len 40 / end"
>>>>>>
>>>>>> @Ophir, @Lijun,
>>>>>> Can we ignore the 'key_len' if the 'key' is not supported and solve
>>>>>> current issue as initially intended ([1])?
>>>>>>
>>>>> Hi, Ferruh
>>>>>     I have discussed with Phil Yang about the problem in [1]. I think
>>>>> there may be other problems with the idea and there is no better
>>>>> solution. and we need to remove key_len definition from rte_rss_conf
>>>>> structure. They don't have a plan. And [1] was eventually reverted.
>>>>>
>>>>
>>>> Why ignoring 'key_len' (set it to zero) when there is no 'key'
>>>> provided doesn't work?
>>>>
>>>
>>> What do you think [1] + following update, will it work?
>>>
>>>    diff --git a/lib/librte_ethdev/rte_flow.c 
>>> b/lib/librte_ethdev/rte_flow.c
>>>    index ee4f3464fe..e7789c87b3 100644
>>>    --- a/lib/librte_ethdev/rte_flow.c
>>>    +++ b/lib/librte_ethdev/rte_flow.c
>>>    @@ -535,7 +535,7 @@ rte_flow_conv_action_conf(void *buf, const size_t
>>> size,
>>>                               }),
>>>                               size > sizeof(*dst.rss) ? 
>>> sizeof(*dst.rss) : size);
>>>                    off = sizeof(*dst.rss);
>>>    -               if (src.rss->key_len) {
>>>    +               if (src.rss->key_len && src.rss->key) {
>>>                            off = RTE_ALIGN_CEIL(off, 
>>> sizeof(*dst.rss->key));
>>>                            tmp = sizeof(*src.rss->key) * 
>>> src.rss->key_len;
>>>                            if (size >= off + tmp)
>>>
>>
>> Ferruh, your suggestion ([1] + update) looks correct. I also verified 
>> it on mlx5 PMD.
>> Advantage: it's a generic fix for all dpdk applications using 
>> rte_flows (not just testpmd).
>> It reduces code.
>> With this fix the responsibility of handling key==NULL and/or len==0 
>> is moved to the PMDs (which is good).
>> With regard to Lijun patch - I liked the approach of overriding the 
>> default testpmd key with the default PMD key.
>> But it only addresses testpmd. More code was added.
>> It seems OK to call rte_eth_dev_rss_hash_conf_get() as part of parsing 
>> RSS, but it would feel more confident if we could confirm it for all 
>> the PMDs (by testing) or at least review the PMDs rss_hash_conf_get() 
>> implementations.
>>
> 
> Lijun's idea can work. There was a problem in implementation related to 
> the key size assumption, which can be fixed.
> 
> Even it is fixed, when user gives a rss rule without a key, we are 
> getting key from device and feeding same key back to device, this is 
> unnecessary I think.
> When user didn't provide a key, rss rule shouldn't touch the key at all.
> 
Do you mean that the driver should not configure the key to the hardware 
when the RSS key is not specified?
If so, we cannot identify when the user does not specify the RSS key to 
determine whether to configure the key.
In hardware design, most vendors do not configure keys for hardware 
independently, which may be associated with other RSS config parameters.
I think it would be reasonable for us to reconfigure the RSS key with 
the default value configured and valid in the hardware as the default 
value if the user does not specify the RSS key.
> Complication was when user provides key_len without a key, I think both 
> ignoring or returning error in this case is OK.
I agree. However, I think it is meaningless to expose the RSS key length 
to users. Do you consider deleting the RSS key length directly?
> .
> 


More information about the dev mailing list