All of lore.kernel.org
 help / color / mirror / Atom feed
From: oulijun <oulijun@huawei.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Ophir Munk <ophirmu@nvidia.com>,
	"wenzhuo.lu@intel.com" <wenzhuo.lu@intel.com>,
	"beilei.xing@intel.com" <beilei.xing@intel.com>,
	NBU-Contact-Adrien Mazarguil <adrien.mazarguil@6wind.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	"linuxarm@huawei.com" <linuxarm@huawei.com>,
	Ori Kam <orika@nvidia.com>
Subject: Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration
Date: Wed, 21 Oct 2020 16:19:32 +0800	[thread overview]
Message-ID: <f7bbf509-8daf-cec9-021f-835406bf1ed9@huawei.com> (raw)
In-Reply-To: <57914daa-fb4d-458c-0749-40448f2e416d@intel.com>



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

  reply	other threads:[~2020-10-21  8:23 UTC|newest]

Thread overview: 44+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-10  1:51 [dpdk-dev] [PATCH v3] app/testpmd: fix the default RSS key configuration Lijun Ou
2020-09-22  9:51 ` Phil Yang
2020-09-22 13:50   ` oulijun
2020-09-22 15:44     ` Phil Yang
2020-09-24 13:45 ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Lijun Ou
2020-09-24 13:45   ` [dpdk-dev] [PATCH v4] app/testpmd: fix the default RSS key configuration Lijun Ou
2020-09-29 15:35     ` Phil Yang
2020-09-30 12:57     ` Ferruh Yigit
2020-10-09 11:55       ` oulijun
2020-10-09 18:27         ` Ferruh Yigit
2020-10-09 18:54           ` Ferruh Yigit
2020-09-30 13:36     ` Ferruh Yigit
2020-10-09 11:59       ` oulijun
2020-10-09 18:36         ` Ferruh Yigit
2020-10-15 12:41     ` [dpdk-dev] [PATCH v5] " Lijun Ou
2020-10-15 13:52       ` Ferruh Yigit
2020-10-15 14:04         ` oulijun
2020-10-15 14:43           ` Ferruh Yigit
2020-10-15 16:05             ` Ferruh Yigit
2020-10-15 23:21               ` Ophir Munk
2020-10-15 23:53                 ` Ferruh Yigit
2020-10-16  0:14                   ` Ajit Khaparde
2020-10-16  6:46                   ` Ophir Munk
2020-10-16 10:05                     ` oulijun
2020-10-16 15:13                       ` Ophir Munk
2020-10-16 10:04                   ` oulijun
2020-10-16 10:57                     ` Ferruh Yigit
2020-10-16 14:59                       ` Ophir Munk
2020-10-20  9:00                       ` oulijun
2020-10-20 10:02                         ` Ferruh Yigit
2020-10-20 13:35                           ` oulijun
2020-10-20 14:34                             ` Ferruh Yigit
2020-10-21  8:19                               ` oulijun [this message]
2020-10-21  9:38                                 ` Ferruh Yigit
2020-10-21 10:07                                   ` oulijun
     [not found]       ` <20201015195637.26476-1-robot@bytheb.org>
2020-10-16  9:39         ` [dpdk-dev] |WARNING| pw80899 " oulijun
2020-10-16  9:41           ` David Marchand
2020-09-30 13:17   ` [dpdk-dev] [PATCH v4] RSS key use with testpmd Ferruh Yigit
2020-10-09 12:09     ` oulijun
2020-10-09 18:52       ` Ferruh Yigit
2020-10-10  3:07         ` Phil Yang
2020-10-14  6:15         ` oulijun
2020-10-14  8:41           ` Ferruh Yigit
2020-10-15 12:30 [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration Lijun Ou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f7bbf509-8daf-cec9-021f-835406bf1ed9@huawei.com \
    --to=oulijun@huawei.com \
    --cc=adrien.mazarguil@6wind.com \
    --cc=beilei.xing@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=linuxarm@huawei.com \
    --cc=ophirmu@nvidia.com \
    --cc=orika@nvidia.com \
    --cc=wenzhuo.lu@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.