From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id B58FCA0C47 for ; Mon, 12 Apr 2021 14:46:22 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A0958141191; Mon, 12 Apr 2021 14:46:22 +0200 (CEST) Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by mails.dpdk.org (Postfix) with ESMTP id C285E141187; Mon, 12 Apr 2021 14:46:18 +0200 (CEST) IronPort-SDR: ESyppOLMNqWgrKMHm0O2s8kGXCygmjS7pyG1HsQ54KN2AHXVDbhDjM4NCODjuj5daFxVu2Bkuy 9lYKc9ZMB4BQ== X-IronPort-AV: E=McAfee;i="6000,8403,9952"; a="279479106" X-IronPort-AV: E=Sophos;i="5.82,216,1613462400"; d="scan'208";a="279479106" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga105.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2021 05:46:17 -0700 IronPort-SDR: iiN9dat26SKITOxDQwiT9EffSO9Wa2ldyk+3xzEEmzsgckRROx5HBFo08MJJMrmZagYqYd5NRC bEMAOfw56UYQ== X-IronPort-AV: E=Sophos;i="5.82,216,1613462400"; d="scan'208";a="460150817" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.213.203.254]) ([10.213.203.254]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 12 Apr 2021 05:46:16 -0700 To: "Li, Xiaoyun" , "Yu, DapengX" , "Zhang, Qi Z" Cc: "dev@dpdk.org" , "stable@dpdk.org" References: <20210401082844.401918-1-dapengx.yu@intel.com> <500b55c4-6ec5-1466-8ddf-60728e9cefcb@intel.com> From: Ferruh Yigit X-User: ferruhy Message-ID: <23966e5e-fd54-379e-3681-98ec4be7af0f@intel.com> Date: Mon, 12 Apr 2021 13:46:12 +0100 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload reconfig cmd X-BeenThere: stable@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: patches for DPDK stable branches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: stable-bounces@dpdk.org Sender: "stable" On 4/12/2021 3:21 AM, Li, Xiaoyun wrote: > > >> -----Original Message----- >> From: Yu, DapengX >> Sent: Friday, April 9, 2021 18:29 >> To: Li, Xiaoyun ; Yigit, Ferruh ; >> Zhang, Qi Z >> Cc: dev@dpdk.org; stable@dpdk.org >> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx offload >> reconfig cmd >> >> >> >>> -----Original Message----- >>> From: Li, Xiaoyun >>> Sent: Friday, April 9, 2021 3:50 PM >>> To: Yu, DapengX ; Yigit, Ferruh >>> ; Zhang, Qi Z >>> Cc: dev@dpdk.org; stable@dpdk.org >>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx >>> offload reconfig cmd >>> >>> >>> >>>> -----Original Message----- >>>> From: Yu, DapengX >>>> Sent: Friday, April 9, 2021 13:25 >>>> To: Yigit, Ferruh ; Li, Xiaoyun >>>> ; Zhang, Qi Z >>>> Cc: dev@dpdk.org; stable@dpdk.org >>>> Subject: RE: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and Tx >>>> offload reconfig cmd >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: Yigit, Ferruh >>>>> Sent: Thursday, April 8, 2021 11:42 PM >>>>> To: Yu, DapengX ; Li, Xiaoyun >>>>> ; Zhang, Qi Z >>>>> Cc: dev@dpdk.org; stable@dpdk.org >>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix queue Rx and >>>>> Tx offload reconfig cmd >>>>> >>>>> On 4/1/2021 9:28 AM, dapengx.yu@intel.com wrote: >>>>>> From: Dapeng Yu >>>>>> >>>>>> Configure per queue rx offloading and per queue tx offloading >>>>>> command shouldn't trigger the rte_eth_dev_configure() to >>>>>> reconfigure >>>> device. >>>>>> >>>>>> The patch sets the queue reconfiguration flag only, and does not >>>>>> set the device reconfiguration flag. Therefore after port is >>>>>> restarted, >>>>>> rte_eth_dev_configure() will not be called again. >>>>>> >>>>> >>>>> Just to clarify the impact, was calling 'rte_eth_dev_configure()' >>>>> causing any problem, is this fixing any issue? >>>>> Or is this patch an optimization to eliminate an unnecessary call? >>>>> >>>> This patch does fix an issue, and it also eliminates an unnecessary call. >>>> >>>> The issue is: >>>> per-queue configuration, for example: port 0 rxq 0 rx_offload >>>> jumbo_frame off triggers the per-device configuration change: the >>>> RSS key is reconfigured and changes after rte_eth_dev_configure() is >>>> called on ICE PMD driver, that cause a test case failure. >>>> >>> >>> Hmmm. I agree on the following. It doesn't need dev_configure. That's >>> why I give you ack. >>> >>> But your issue. Shouldn't you fix the driver? The vsi->rss_key[] just >>> updated itself as a random value when dev_conf doesn't contain one. >>> But your case is dev_conf doesn't contain new rss key, but >>> vsi->rss_key is not null. In this case, it should keep the same not get a new >> random key. >>> >> In current implementation, dev->data->dev_conf.rx_adv_conf.rss_conf in PMD >> does not hold the rss_key value if user does not set it in the input parameter of >> rte_eth_dev_configure(). >> >> Even if PMD generate one automatically, it will not be saved in dev->data- >>> dev_conf.rx_adv_conf.rss_conf. >> >> When user want to get rss_key, it will be retrieved on the fly from hardware, but >> not from any variable in PMD. >> >> So PMD (ice and i40e) think only rss_key which is set by user via >> rte_eth_dev_configure() can be reused when port is reconfigured. >> >> If it is not present, PMD will generate one by itself anyway even if it is present in >> vsi->rss_key. >> >> I don’t think this behavior is wrong, so did not fix ice PMD. > > If you want to recover the rss key. The driver should store the key in vsi->rss_key also in ice_rss_hash_update(). Then when user not config rss_key in rss_conf and vs->rss_key is not 0, you should set the original value not a random value to hw. > +1 > Otherwise, you are assuming the behavior rss to different queues is right since user want random key. It's not an issue. > > Any scenario, it's not testpmd issue. Please withdraw your patch. > >> >>>> There is an unnecessary call in original implementation because both >>>> cmd_config_per_queue_rx_offload_parsed() and >>>> cmd_config_per_queue_tx_offload_parsed() >>>> does not update the "port->dev_conf" which hold the port >>>> configuration, therefore there is no need to call rte_eth_dev_configure(). >