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

[oulijun <oulijun@huawei.com>]

在 2020/10/21 17:38, Ferruh Yigit 写道:
> On 10/21/2020 9:19 AM, oulijun wrote:
>>
>>
>> 在 2020/10/20 22:34, Ferruh Yigit 写道:
>>> On 10/20/2020 2:35 PM, oulijun wrote:
>>>>
>>>>
>>>> 在 2020/10/20 18:02, Ferruh Yigit 写道:
>>>>> On 10/20/2020 10:00 AM, oulijun wrote:
>>>>>>
>>>>>>
>>>>>> 在 2020/10/16 18:57, Ferruh Yigit 写道:
>>>>>>> On 10/16/2020 11:04 AM, oulijun wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> 在 2020/10/16 7:53, Ferruh Yigit 写道:
>>>>>>>>> On 10/16/2020 12:21 AM, Ophir Munk wrote:
>>>>>>>>>>> From: Ferruh Yigit <ferruh.yigit@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@dpdk.org
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Signed-off-by: Lijun Ou <oulijun@huawei.com>
>>>>>>>>>>>>>>> Reviewed-by: Phil Yang <phil.yang@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?
>>>>>>>
>>>>>>> We are talking about 'key' in RSS rte flow rule, not generally 
>>>>>>> configuring the device for RSS.
>>>>>>>
>>>>>> Yes, I know. However, I am talking about some implementations. The 
>>>>>> configuration interface may be the same as that for configuring RSS
>>>>>> parameters of the device in general mode.
>>>>>>> If a specific rss rte flow rule, lets say to filter some defined 
>>>>>>> packets to some specific queues, don't have 'key' as a part of 
>>>>>>> rule, I am suggesting not touch 'key' configuration of the device.
>>>>>>>
>>>>>> Yes, I agree. I think your point of view is very reasonable and a 
>>>>>> more
>>>>>> appropriate implementation. However, for the sake of simplicity and
>>>>>> other considerations in the architecture design of some devices, the
>>>>>> hardware may support reasonable hybrid configuration for paramter 
>>>>>> configuration in the RSS config, rather than independent 
>>>>>> configuration
>>>>>> for hw, that is, maybe touch A of rss configuration of the device and
>>>>>> must touch B of rss configuration. As a result, If the preceding 
>>>>>> scenario occurs, it is reasonable to configure an RSS config that 
>>>>>> does not change and specified.What do you think?
>>>>>>>> If so, we cannot identify when the user does not specify the RSS 
>>>>>>>> key to determine whether to configure the key.
>>>>>>>
>>>>>>> I think we can. If the rule has 'key' attribute, driver can use 
>>>>>>> that key to program the device, if 'key' attribute is missing, 
>>>>>>> don't do anything related to the rss key.
>>>>>>>
>>>>>> Yes, if 'key' attribute is missing, don't do anything related to 
>>>>>> the rss key. It's more reasonable.
>>>>>> But, If he can't do that, I think it is reasonable to configure 
>>>>>> the default key that already exists on the device. The only 
>>>>>> disadvantage is wasteful.
>>>>>>>> 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.
>>>>>>>
>>>>>>> There are already APIs to update the RSS configuration, like RSS 
>>>>>>> key, hash functions etc.. They are independent from the rte flow 
>>>>>>> RSS rule.
>>>>>>> Application should configure RSS according needs using those APIs.
>>>>>>>
>>>>>> Yes, should do this. Are there any unreasonable users who simply 
>>>>>> do not know these APIs or do not want to use them and want to 
>>>>>> configure some parameters through the rte flow RSS rule without 
>>>>>> changing other parameters?
>>>>>>> The question here is about each RSS rte flow rule that can update 
>>>>>>> the key. Am I missing something?
>>>>>> I don't think you've misunderstood the general situation.
>>>>>> Even if the RSS key is not specified in a rule, the driver uses 
>>>>>> the default key value to re-touch the device.
>>>>>>>
>>>>>>>>> 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?
>>>>>>>
>>>>>>> Isn't both 'key' and 'key_len' needed to program the RSS key? Can 
>>>>>>> the driver know the size of the 'key' without 'key_len'?
>>>>>>> And driver should verify these provided values.
>>>>>>>
>>>>>> I know and agree.It is only during the analysis of the [1] scheme, 
>>>>>> from the time it was proposed to the time it was withdrawn, that I 
>>>>>> found some guys in the community questioned the significance of 
>>>>>> key_len.
>>>>>>
>>>>>> Back to the subject, what is your plan for the solution in the patch?
>>>>>
>>>>> I think, Ophir's old reverted patch [1] + 
>>>>> 'rte_flow_conv_action_conf()' update I proposed above can work. Is 
>>>>> this working for you? If so can you please send a patch?
>>>>>
>>>>>
>>>>> [1] https://git.dpdk.org/dpdk/commit/?id=a4391f8bae8
>>>>> .
>>>> Hi, Ferruh
>>>>     I've just used [1] + 'rte_flow_conv_action_conf()' for testing 
>>>> based on the Kunpeng920 NIC plation and use hns3 pmd driver. it can 
>>>> be resolve the RSS key question.
>>>>     testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>>>
>>>> testpmd> flow create 0 ingress pattern end actions rss func 
>>>> symmetric_toeplitz types all end / end
>>>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
>>>> hardware support, requested:7f83fffc configured:3ef8
>>>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using 
>>>> default
>>>> Flow rule #0 created
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-tcp ipv4-udp ipv4-sctp ipv4-other ipv6-frag 
>>>> ipv6-tcp ipv6-udp ipv6-sctp ipv6-other ip udp tcp sctp
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>>>
>>>>
>>>> However, I also tested the case. I am not specified the RSS key and 
>>>> specified the key_len when create a RSS flow rule, it can create 
>>>> success.
>>>> testpmd> flow create 0 ingress pattern end actions rss queues 0 1 
>>>> end key_len 40 / end
>>>> 0000:7d:00.0 hns3_config_rss_filter(): modified RSS types based on 
>>>> hardware support, requested:a38c configured:2288
>>>> 0000:7d:00.0 hns3_parse_rss_key(): no valid RSS key config, using 
>>>> default
>>>> Flow rule #0 created
>>>> testpmd> show port 0 rss-hash key
>>>> RSS functions:
>>>>   all ipv4-frag ipv4-other ipv6-frag ipv6-other ip
>>>> RSS key:
>>>> 6D5A56DA255B0EC24167253D43A38FB0D0CA2BCBAE7B30B477CB2DA38030F20C6A42B73BBEAC01FA 
>>>>
>>>>
>>>
>>> So are you happy with this solution?
>>> .
>> Yes, thanks.
> 
> cool, will you able to send a new version? Thanks.
> .
Yes
>