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 2FC7FA04BC; Fri, 9 Oct 2020 20:54:31 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 085B01D42A; Fri, 9 Oct 2020 20:54:30 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 932781D424 for ; Fri, 9 Oct 2020 20:54:27 +0200 (CEST) IronPort-SDR: Rcm7IZxHgVWuoSe4GvbyR9HvVxfVusSp3krZua1HAtK5u/mkBFFqooqmp/ZsA9uq4VkA5b5eG+ GJhCGGjwmdZQ== X-IronPort-AV: E=McAfee;i="6000,8403,9769"; a="162063003" X-IronPort-AV: E=Sophos;i="5.77,355,1596524400"; d="scan'208";a="162063003" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Oct 2020 11:54:26 -0700 IronPort-SDR: vq3PxMG5KfpcDUEAvAxeqoiRG/42cn1QfNZpkYzwdnAu62/xMcy5KYXp/SPGEae8+08+6tI29V w/0p65+iFr5w== X-IronPort-AV: E=Sophos;i="5.77,355,1596524400"; d="scan'208";a="462288873" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.252.18.7]) ([10.252.18.7]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Oct 2020 11:54:24 -0700 From: Ferruh Yigit To: oulijun , wenzhuo.lu@intel.com, beilei.xing@intel.com, adrien.mazarguil@6wind.com Cc: dev@dpdk.org, linuxarm@huawei.com, Ori Kam References: <1599702678-11142-1-git-send-email-oulijun@huawei.com> <1600955105-53176-1-git-send-email-oulijun@huawei.com> <1600955105-53176-2-git-send-email-oulijun@huawei.com> <35e9e9bb-50fa-d76f-46b0-c9556c84114b@intel.com> <51f5c046-38d9-e87b-bd04-5a2eca000203@huawei.com> <51baf0aa-9e26-56c3-569f-91cc846f8caa@intel.com> Message-ID: Date: Fri, 9 Oct 2020 19:54:23 +0100 MIME-Version: 1.0 In-Reply-To: <51baf0aa-9e26-56c3-569f-91cc846f8caa@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v4] 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" On 10/9/2020 7:27 PM, Ferruh Yigit wrote: > On 10/9/2020 12:55 PM, oulijun wrote: >> >> >> 在 2020/9/30 20:57, Ferruh Yigit 写道: >>> On 9/24/2020 2:45 PM, Lijun Ou wrote: >>>> 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. >>> >>> Can you please describe the impact, what fails without this fix? >>> >> Hi, Ferruh >>    According to Phil Yang's suggestion, I've put the impact description in the >> cover letter. >>     When a user runs a flow create cmd to configure an RSS rule >> with specifying the empty rss actions in testpmd, this mean >> that the flow gets the default RSS functions from the valid >> NIC default RSS hash key. However, the testpmd is not set >> the default RSS key incorrectly when RSS key is not specified >> in flow create cmd. >>     After the testpmd is started, run the flow create cmd without specifying >> the RSS hash key. Ensure that RSS hash key queried after >> the RSS flow is configured is the same as the default key after the >> testpmd is start. >> >>>> >>>> Fixes: ac8d22de2394 ("ethdev: flatten RSS configuration in flow API") >>>> Cc: stable@dpdk.org >>>> >>>> Signed-off-by: Lijun Ou >>>> --- >>>> V3->V4: >>>> -fix checkpatch warning and shorter commit content. >>>> >>>> V2->V3: >>>> -fix checkpatch warning. >>>> >>>> V1->V2: >>>> -fix the commit. >>>> --- >>>>   app/test-pmd/cmdline_flow.c | 8 ++++++++ >>>>   1 file changed, 8 insertions(+) >>>> >>>> diff --git a/app/test-pmd/cmdline_flow.c b/app/test-pmd/cmdline_flow.c >>>> index 6263d30..e6648da 100644 >>>> --- a/app/test-pmd/cmdline_flow.c >>>> +++ b/app/test-pmd/cmdline_flow.c >>>> @@ -4312,6 +4312,7 @@ parse_vc_action_rss(struct context *ctx, const struct >>>> token *token, >>>>           action_rss_data->queue[i] = i; >>>>       if (!port_id_is_invalid(ctx->port, DISABLED_WARN) && >>>>           ctx->port != (portid_t)RTE_PORT_ALL) { >>>> +        struct rte_eth_rss_conf rss_conf = {0}; >>>>           struct rte_eth_dev_info info; >>>>           int ret2; >>>> @@ -4322,6 +4323,13 @@ parse_vc_action_rss(struct context *ctx, const struct >>>> token *token, >>>>           action_rss_data->conf.key_len = >>>>               RTE_MIN(sizeof(action_rss_data->key), >>>>                   info.hash_key_size); >>>> + >>>> +        rss_conf.rss_key_len = sizeof(action_rss_data->key); >>>> +        rss_conf.rss_key = action_rss_data->key; >>> >>> 'rss_conf.rss_key_len' is the input parameter, it looks like it has been used >>> as the size of the 'rss_key', but as far as I can see it is not. >>> >> Yes, rss_key_len and rss_key is configured independently. In fact, when the >> user configures the rss_key， we should know rss_key_len. >>> Because of this if 'info.hash_key_size' is bigger than >>> 'sizeof(action_rss_data->key)', won't PMD overwrite the stack for the >>> remaining bytes? >> if info.hash_key_size and action_rss_data->key is set used RSS flow cmd when >> info.hash_key_size is bigger than sizeof(action_rss_data->key)， the driver >> will return an error. > > > > How can driver return an error? > Both 'rss_conf.rss_key_len' & 'rss_conf.rss_key' filled by driver, and driver > expect 'rss_conf.rss_key' is big enough to hold its key. > Here 'rss_conf.rss_key' points to fixed size 'action_rss_data->key' array > without checking the driver key length. > > Where this driver check happens, am I missing it? > >>> >>> Can you please check? >>> >>>> +        ret2 = rte_eth_dev_rss_hash_conf_get(ctx->port, &rss_conf); >>>> +        if (ret2 != 0) >>>> +            return ret2; >>> >>> If PMD not implemented the 'rss_hash_conf_get' dev_ops, should it fail the >>> RSS action? >> Yes, When the RSS hash key is not specified in the RSS flow, ensure that the >> RSS hash key remains unchanged and is the same as that configured in the >> hardware when the testpmd is started. > > OK, but this means any PMD not implemented dev_ops to get RSS key won't able to > use rte flow RSS action via testpmd, not sure if this is right. > > What this patch does is read the device configured RSS key, and feed it back to > rte flow RSS action, to be sure device key is not changed. > > Instead, can we update the rte flow RSS action to not change the key if it is > not provided, and don't provide one in the testpmd by default. > > What do you think? > cc'ed Ori for the rte flow related change suggestion. @Ori, can you please check the problem this patch address and the suggestion above?