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 2355541BAA; Thu, 2 Feb 2023 09:50:50 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C064B406A2; Thu, 2 Feb 2023 09:50:49 +0100 (CET) Received: from agw.arknetworks.am (agw.arknetworks.am [79.141.165.80]) by mails.dpdk.org (Postfix) with ESMTP id 7FDCB40689 for ; Thu, 2 Feb 2023 09:50:48 +0100 (CET) Received: from debian (unknown [78.109.70.208]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by agw.arknetworks.am (Postfix) with ESMTPSA id 6B95AE0AF8; Thu, 2 Feb 2023 12:50:47 +0400 (+04) Date: Thu, 2 Feb 2023 12:50:53 +0400 (+04) From: Ivan Malov To: Ferruh Yigit cc: Jerin Jacob , Thomas Monjalon , Andrew Rybchenko , Ori Kam , Nithin Kumar Dabilpuram , Aman Singh , Yuying Zhang , "dev@dpdk.org" , Hanumanth Reddy Pothula , Slava Ovsiienko , Jerin Jacob Kollanukkaran , "david.marchand@redhat.com" Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation In-Reply-To: Message-ID: <3776bc5d-b8b-c765-4feb-62944e709c4c@arknetworks.am> References: <20221220200250.2413443-1-hpothula@marvell.com> <98a80c20-a5e4-deea-f7dc-c6aa5d52800b@oktetlabs.ru> <2490780.4XsnlVU6TS@thomas> <22a65100-bee8-1726-6e27-14b9028a29d4@amd.com> <01d3b455-3f3e-2658-db51-4da7cfc3cdeb@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Thu, 2 Feb 2023, Ferruh Yigit wrote: > On 2/1/2023 3:22 PM, Jerin Jacob wrote: >> On Wed, Feb 1, 2023 at 8:20 PM Ferruh Yigit wrote: >>> >>> On 2/1/2023 1:48 PM, Jerin Jacob wrote: >>>> On Wed, Feb 1, 2023 at 5:06 PM Ferruh Yigit wrote: >>>>> >>>>> On 2/1/2023 11:15 AM, Jerin Jacob wrote: >>>>>> On Wed, Feb 1, 2023 at 4:35 PM Thomas Monjalon wrote: >>>>>>> >>>>>>> 01/02/2023 11:58, Andrew Rybchenko: >>>>>>>> On 2/1/23 13:48, Jerin Jacob wrote: >>>>>>>>> On Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko >>>>>>>>> wrote: >>>>>>>>>> Frankly speaking I don't understand why default value is so >>>>>>>>>> important if we have a way to change it. Reasons should be >>>>>>>>>> really strong to change existing defaults. >>>>>>>>> >>>>>>>>> The only reason is, typically testpmd will be used performance >>>>>>>>> benchmarking as an industry standard. It is difficult to tell/educate >>>>>>>>> the QA or customers >>>>>>>>> that, "BTW if you need to get better performance add more flag to >>>>>>>>> testpmd command line". >>>>>>> >>>>>>> I disagree. >>>>>>> When you do performance benchmark, you tune settings accordingly. >>>>>> >>>>>> IMO, We tune the system resources like queue depth not the disabling >>>>>> features for raw performance. >>>>>> queue depth etc people know to tune so it is obvious. What is not >>>>>> obvious is, testpmd only >>>>>> negotiated some features by default.I am not using that feature, hence >>>>>> I need to explicitly >>>>>> disable it. >>>>>> >>>>> >>>>> When 'rte_eth_rx_metadata_negotiate()' API is NOT used at all, and I >>>>> believe that is the case for almost all applications since API is a >>>>> relatively new one, PMD default behavior should be to enable Rx metadata >>>>> flow rules, in case user requests them later. >>>>> >>>>> So, enabling all in application is same with not calling the API at all. >>>>> >>>>> In this perspective, disabling Rx metadata is additional >>>>> optimization/tuning that application can do if it is sure that Rx >>>>> metadata flow rules won't be used at all. >>>>> And API is more meaningful when it is used to disable Rx metadata. >>>>> >>>>> I think it is reasonable to enable all Rx metadata by default in testpmd >>>>> with a capability to disable it when wanted. >>>>> >>>>> OR >>>>> >>>>> May be we don't call 'rte_eth_rx_metadata_negotiate()' API by default in >>>>> testpmd, it is only called when it is requested explicitly from user, >>>>> enable or disable. >>>> >>>> Second option looks good to me. >>>> When >>>> 1) user request for action which is needed negotiate(), >>>> AND >>>> 2) rte_eth_rx_metadata_negotiate() != ENOSUP >>>> then, testpmd print a warning that need to enable >>>> rte_eth_rx_metadata_negotiate(). >>>> >>> >>> We are not suggesting same thing. >>> >>> What you described above assumes PMD disabled Rx metadata flow rule >>> support by default, and it needs to be enabled explicitly by >>> 'rte_eth_rx_metadata_negotiate()' API. This API becomes mandatory for >>> functionality. >>> >>> As far as I understand PMD wants to disable this flow rule by default >>> because of performance concerns. But this creates inconsistency between >>> PMDs, because rest of them will enable this flow rule by default (if it >>> is supported) and be ready to use it when proper flow rule created. >>> >>> With this approach some PMDs will need 'rte_eth_rx_metadata_negotiate()' >>> to enable Rx metadata flow rules, some won't. This can be confusing for >>> applications that *some* PMDs require double enabling with specific API >>> call. >>> >>> >>> Instead what I was trying to suggest is reverse, >>> all PMDs enable the Rx metadata flow rule by default, and don't require >>> double enabling. >>> But if application knows that it won't use Rx metadata flow rule, it can >>> disable it to optimize the performance. >>> This makes 'rte_eth_rx_metadata_negotiate()' functionally optional, and >>> for testpmd context it can be called via a command on demand by user for >>> optimization purpose. >> >> This won't solve concern I have outlined earlier[1]. >> > > Yes, it won't. > >> I think, The part of the problem there is no enough adaption of >> rte_eth_rx_metadata_negotiate(), >> >> The view is total different from PMD maintainer PoV vs testpmd application PoV. >> > > Agree, > and I assume it is different for user application too, which may > prioritize consistency and portability. > > Overall, I am not fan of the 'rte_eth_rx_metadata_negotiate()' API, I > think it is confusing. Forgive me, in which way is it confusing? > >> Just to avoid back and forth. We will call off this patch and remove >> rte_eth_rx_metadata_negotiate() >> PMD callback from cnxk driver. Keep it as old behavior, so we don't need to care >> about rte_eth_rx_metadata_negotiate(). >> > > When you remove 'rx_metadata_negotiate' callback, what will be the PMD > behavior? I assume PMD will do the required preparations as if all Rx > metadata is enabled. > And what is the performance impact, is removing callback improve the > performance? > > >> [1] >> The only reason is, typically testpmd will be used performance >> benchmarking as an industry standard. It is difficult to tell/educate >> the QA or customers >> that, "BTW if you need to get better performance add more flag to >> testpmd command line". >> To make that worst, only some PMD needs to give the additional >> parameter to get better number. >> And also, testpmd usage will be treated as application modeling. >> >>> >>> >>> >>> >>>> >>>>>> >>>>>>> >>>>>>>>> To make that worst, only some PMD needs to give the additional >>>>>>>>> parameter to get better number. >>>>>>>>> And also, testpmd usage will be treated as application modeling. >>>>>>>>> >>>>>>>>> Since this feature only used on sfc and cnxk driver, What is the >>>>>>>>> situation with sfc driver? >>>>>>>>> Keeping it as negotiated and not use the feature, will impact the per >>>>>>>>> core performance of sfc or >>>>>>>>> is it just PCI bandwidth thing which really dont show any difference in testpmd? >>>>>>>> >>>>>>>> Yes, sfc could run faster if no Rx metadata are negotiated. So, >>>>>>>> it is better to negotiate nothing by default. But it is always >>>>>>>> painful to change defaults. You need to explain that now you >>>>>>>> need to negotiate Rx metadata to use mark, flag and tunnel offloads. >>>>>>>> Yes, it will be required on sfc and cnxk only. >>>>>>>> As an sfc maintainer I don't mind to change testpmd defaults. >>>>>>> >>>>>>> If we change testpmd defaults to "do nothing", >>>>>>> then we should disable MBUF_FAST_FREE as well. >>>>>> >>>>>> if you see MBUF_FAST_FREE, it does nothing. Actually, >>>>>> !MBUF_FAST_FREE is doing more work. >>>>>> >>>>>> >>>>>>> >>>>>>> >>>>> >>> > >