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

[Ferruh Yigit <ferruh.yigit@intel.com>]

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.