From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 56557A04DD; Wed, 21 Oct 2020 10:23:07 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 14463ACE9; Wed, 21 Oct 2020 10:19:48 +0200 (CEST) Received: from huawei.com (szxga06-in.huawei.com [45.249.212.32]) by dpdk.org (Postfix) with ESMTP id C1A75ACD6 for ; Wed, 21 Oct 2020 10:19:44 +0200 (CEST) Received: from DGGEMS408-HUB.china.huawei.com (unknown [172.30.72.59]) by Forcepoint Email with ESMTP id 7D30EC517D6A7E18AA95; Wed, 21 Oct 2020 16:19:42 +0800 (CST) Received: from [10.67.103.119] (10.67.103.119) by DGGEMS408-HUB.china.huawei.com (10.3.19.208) with Microsoft SMTP Server id 14.3.487.0; Wed, 21 Oct 2020 16:19:32 +0800 To: Ferruh Yigit , Ophir Munk , "wenzhuo.lu@intel.com" , "beilei.xing@intel.com" , NBU-Contact-Adrien Mazarguil CC: "dev@dpdk.org" , "linuxarm@huawei.com" , Ori Kam References: <1600955105-53176-2-git-send-email-oulijun@huawei.com> <1602765662-43299-1-git-send-email-oulijun@huawei.com> <1b2b0e11-1458-e2d9-5fde-91db35b8bc73@intel.com> <51676746-ec95-aff2-dc00-b76480ab6cba@huawei.com> <8c543941-e881-aa88-b52a-a70bfe8f6fcf@intel.com> <8d592d3e-bace-eb50-a3af-4a0bb87ddae8@intel.com> <7618978b-9675-dbf3-31d0-3abc98fc4304@huawei.com> <0fabbf85-156e-f578-ce2d-b1115c8640b8@intel.com> <7d7837d9-733c-c203-c260-8966c8945399@huawei.com> <57914daa-fb4d-458c-0749-40448f2e416d@intel.com> From: oulijun Message-ID: Date: Wed, 21 Oct 2020 16:19:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 MIME-Version: 1.0 In-Reply-To: <57914daa-fb4d-458c-0749-40448f2e416d@intel.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 8bit X-Originating-IP: [10.67.103.119] X-CFilter-Loop: Reflected Subject: Re: [dpdk-dev] [PATCH v5] app/testpmd: fix the default RSS key configuration X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 在 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 >>>>>>>>>>>> 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 >>>>>>>>>>>>> Reviewed-by: Phil Yang >>>>>>>>>>>>> --- >>>>>>>>>>>>> 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 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. >