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 1B5C4A0C4F for ; Thu, 22 Jul 2021 13:03:35 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 0AC85410DD; Thu, 22 Jul 2021 13:03:35 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 098754014D; Thu, 22 Jul 2021 13:03:33 +0200 (CEST) Received: from [192.168.100.116] (unknown [37.139.99.76]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 4ABF97F502; Thu, 22 Jul 2021 14:03:32 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 4ABF97F502 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1626951812; bh=jfgjs0Cu8zgjKu8w+jXaFGt/BzWigKSqeR37iU/EQ+I=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=HoMqwxXxbcKz1QKr4q/KuridDLIa0rDjY2u6/FsFHNdbNX3PRKENqfPur/qGPODQE 4aVpHceo+EesSTULUhaU22Ythl4k081XfvIubAiWG/pF2DqVgx82AfoxvnCihd9EDU BhpQmwrrHVrsccD/CaiXzZLHVVXy+8VWGjG1VLOI= To: Ferruh Yigit , "Wang, Jie1X" , "Li, Xiaoyun" , "dev@dpdk.org" Cc: "stable@dpdk.org" References: <20210715113314.8837-1-jie1x.wang@intel.com> <20210715115720.9981-1-jie1x.wang@intel.com> <2102efc6-84e4-f9e2-9053-d5c1a9119a22@intel.com> <6ccf6403-7103-d397-83fa-de233959db78@intel.com> From: Andrew Rybchenko Message-ID: Date: Thu, 22 Jul 2021 14:03:31 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: <6ccf6403-7103-d397-83fa-de233959db78@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash offload 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 7/19/21 7:18 PM, Ferruh Yigit wrote: > On 7/19/2021 10:55 AM, Wang, Jie1X wrote: >> >> >>> -----Original Message----- >>> From: Yigit, Ferruh >>> Sent: Friday, July 16, 2021 4:52 PM >>> To: Li, Xiaoyun ; Wang, Jie1X ; >>> dev@dpdk.org >>> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org >>> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd doesn't show >>> RSS hash offload >>> >>> On 7/16/2021 9:30 AM, Li, Xiaoyun wrote: >>>>> -----Original Message----- >>>>> From: stable On Behalf Of Li, Xiaoyun >>>>> Sent: Thursday, July 15, 2021 12:54 >>>>> To: Wang, Jie1X ; dev@dpdk.org >>>>> Cc: andrew.rybchenko@oktetlabs.ru; stable@dpdk.org >>>>> Subject: Re: [dpdk-stable] [PATCH v4] app/testpmd: fix testpmd >>>>> doesn't show RSS hash offload >>>>> >>>>>> -----Original Message----- >>>>>> From: Wang, Jie1X >>>>>> Sent: Thursday, July 15, 2021 19:57 >>>>>> To: dev@dpdk.org >>>>>> Cc: Li, Xiaoyun ; >>>>>> andrew.rybchenko@oktetlabs.ru; Wang, Jie1X ; >>>>>> stable@dpdk.org >>>>>> Subject: [PATCH v4] app/testpmd: fix testpmd doesn't show RSS hash >>>>>> offload >>>>>> >>>>>> The driver may change offloads info into dev->data->dev_conf in >>>>>> dev_configure which may cause port->dev_conf and port->rx_conf >>>>>> contain >>>>> outdated values. >>>>>> >>>>>> This patch updates the offloads info if it changes to fix this issue. >>>>>> >>>>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings") >>>>>> Cc: stable@dpdk.org >>>>>> >>>>>> Signed-off-by: Jie Wang >>>>>> --- >>>>>> v4: delete the whitespace at the end of the line. >>>>>> v3: >>>>>> - check and update the "offloads" of "port->dev_conf.rx/txmode". >>>>>> - update the commit log. >>>>>> v2: copy "rx/txmode.offloads", instead of copying the entire struct >>>>>> "dev->data- >>>>>>> dev_conf.rx/txmode". >>>>>> --- >>>>>> app/test-pmd/testpmd.c | 27 +++++++++++++++++++++++++++ >>>>>> 1 file changed, 27 insertions(+) >>>>> >>>>> Acked-by: Xiaoyun Li >>>> >>>> Although I gave my ack, app shouldn't touch rte_eth_devices which this patch >>> does. Usually, testpmd should only call function like >>> eth_dev_info_get_print_err(). >>>> But dev_info doesn't contain the info dev->data->dev_conf which the driver >>> modifies. >>>> >>>> Probably we need a better fix. >>>> >>> >>> Agree, an application accessing directly to 'rte_eth_devices' is sign of something >>> missing/wrong. >>> >>> In this case there is no way for application to know what is the configured >>> offload settings per port and queue. Which is missing part I think. >>> >>> As you said normally we get data from PMD mainly via 'rte_eth_dev_info_get()', >>> which is an overloaded function, it provides many different things, like driver >>> default values, limitations, current config/status, capabilities etc... >>> >>> So I think we can do a few things: >>> 1) Add current offload configuration to 'rte_eth_dev_info_get()', so application >>> can get it and use it. >>> The advantage is this API already called many places, many times, so there is a >>> big chance that application already have this information when it needs. >>> Disadvantage is, as mentioned above the API already big and messy, making it >>> bigger makes more error prone and makes easier to break ABI. >>> >> I prefer to choose the 1st suggestion. >> >> Normally PMD gets data via 'rte_eth_dev_info_get()'. When we add offloads configuration >> to it, we can get offloads as same as getting other info. >> > > Most probably it is easier to implement 1), I see your point but as said before > I think 'rte_eth_dev_info_get()' is already messy and I am worried to make it > even bigger. IMHO, (1) is not an option. > I prefer option 2). I'm not sure that API function for each config parameter is an option as well. We should find a balance. May be I'd add something like rte_eth_dev_get_conf(uint16_t port_id, const struct rte_eth_conf **conf) which returns a pointer to up-to-date configuration. I.e. option (3). The tricky part here is to ensure that all specific API which modifies various bits of the configuration updates dev_conf. > > @Thomas, @Andrew, what do you think? > > >>> 2) Add a new API to get configured offload information, so a specific API for it. >>> >>> 3) Get a more generic API to get configured config (dev_conf) which will cover >>> offloads too. >>> Disadvantage can be leaking out too many internal config to user unintentionally. I don't understand it. dev_conf is provided by user on rte_eth_dev_configure().