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 18:07:14 +0800 [thread overview]
Message-ID: <0ca62632-e4ba-c431-57ab-cc80442ca21d@huawei.com> (raw)
In-Reply-To: <f5437009-bae5-6eb1-52ed-7e8fdbb36529@intel.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
>
next prev parent reply other threads:[~2020-10-21 10:07 UTC|newest]
Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-10 1:51 [dpdk-dev] [PATCH v3] " 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
2020-10-21 9:38 ` Ferruh Yigit
2020-10-21 10:07 ` oulijun [this message]
[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=0ca62632-e4ba-c431-57ab-cc80442ca21d@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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).