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 4C776A04DC; Tue, 20 Oct 2020 15:35:27 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 880CABABC; Tue, 20 Oct 2020 15:35:25 +0200 (CEST) Received: from huawei.com (szxga07-in.huawei.com [45.249.212.35]) by dpdk.org (Postfix) with ESMTP id AC247BAB6 for ; Tue, 20 Oct 2020 15:35:22 +0200 (CEST) Received: from DGGEMS407-HUB.china.huawei.com (unknown [172.30.72.58]) by Forcepoint Email with ESMTP id 2EF93BC2363242E4CD87; Tue, 20 Oct 2020 21:35:20 +0800 (CST) Received: from [10.67.103.119] (10.67.103.119) by DGGEMS407-HUB.china.huawei.com (10.3.19.207) with Microsoft SMTP Server id 14.3.487.0; Tue, 20 Oct 2020 21:35:12 +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> From: oulijun Message-ID: <7d7837d9-733c-c203-c260-8966c8945399@huawei.com> Date: Tue, 20 Oct 2020 21:35:12 +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: <0fabbf85-156e-f578-ce2d-b1115c8640b8@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 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 >