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 8113141B9D; Wed, 1 Feb 2023 10:29:33 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6FD0D42D0B; Wed, 1 Feb 2023 10:29:33 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 1FE4742BC9 for ; Wed, 1 Feb 2023 10:29:31 +0100 (CET) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 33E2B50; Wed, 1 Feb 2023 12:29:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 33E2B50 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1675243770; bh=0TNt1DVPhICFbHvByc7wzoXb9Qn6x8pGJVwmBKgIuSk=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=lYq5FOt+A5xpZfN0++O0Y61ebX+f0b+AxxZ1EquJuL7Sn+oFFRKX25loCHgZ+vjYT WUALoadE3y6I+elXZT9R0BHs/97diuIvVCxSzU6H6z10NhyEzCFN5l3ggtNWEFMKNy a+CtN/Qpkvcrd5YzicUm1FkTAIFY6uhrnmxW6aSQ= Message-ID: <056883f1-72c8-3540-b2ad-e12d1cfc4dae@oktetlabs.ru> Date: Wed, 1 Feb 2023 12:29:29 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.6.0 Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation Content-Language: en-US To: Jerin Jacob Cc: Thomas Monjalon , Ori Kam , Ivan Malov , Ivan Malov , Ferruh Yigit , Nithin Kumar Dabilpuram , Aman Singh , Yuying Zhang , "dev@dpdk.org" , Hanumanth Reddy Pothula , Slava Ovsiienko , Jerin Jacob Kollanukkaran , "david.marchand@redhat.com" References: <20221220200250.2413443-1-hpothula@marvell.com> <3180381.AJdgDx1Vlc@thomas> <0e383338-0bd8-8f1c-00f1-50605030d84f@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 2/1/23 12:14, Jerin Jacob wrote: > On Wed, Feb 1, 2023 at 2:37 PM Andrew Rybchenko > wrote: >> >> On 2/1/23 12:05, Thomas Monjalon wrote: >>> 01/02/2023 10:00, Ori Kam: >>>> Hi all, >>>> >>>> Sorry for jumping in late, >>>> >>>>> -----Original Message----- >>>>> From: Jerin Jacob >>>>> Sent: Wednesday, 1 February 2023 10:53 >>>>> >>>>> On Wed, Feb 1, 2023 at 12:46 PM Andrew Rybchenko >>>>> wrote: >>>>>> >>>>>> On 2/1/23 09:10, Ivan Malov wrote: >>>>>>> Hello everyone, >>>>>>> >>>>>>> Since making automatic, or implicit, offload decisions does >>>>>>> not belong in testpmd responsibility domain, it should be >>>>>>> safer to avoid calling the "negotiate metadata delivery" >>>>>>> API with some default selection unless the user asks to >>>>>>> do so explicitly, via internal CLI or app options. >>>>>>> >>>>>>> With that in mind, port config ... sounds OK. >>>>>>> >>>>>>> PMDs that support flow primitives which can generate metadata >>>>>>> but, if in started state, can't enable its delivery may pass >>>>>>> appropriate rte_error messages to the user suggesting >>>>>>> they enable delivery of such metadata from NIC to PMD >>>>>>> first. This way, if the person operating testpmd >>>>>>> enters a flow create command and that fails, >>>>>>> they can figure out the inconsistency, stop >>>>>>> the port, negotiate, start and try again. >>>>>>> >>>>>>> As for non-debug applications, their developers shall >>>>>>> be properly informed about the problem of enabling >>>>>>> delivery of metadata from NIC to PMD. This way, >>>>>>> they will invoke the negotiate API by default >>>>>>> in their apps, with the feature selection (eg. >>>>>>> MARK) as per nature of the app's business. >>>>>>> >>>>>>> This API should indeed be helpful to some PMDs with >>>>>>> regard to collecting upfront knowledge like this. >>>>>>> At the same time, should be benign to those PMDs >>>>>>> who do not need this knowledge and can enable >>>>>>> delivery of metadata right when inserting the >>>>>>> flow rules. So I hope the API does not create >>>>>>> too much discomfort to vendors not needing it. >>>>>>> >>>>>>> Thank you. >>>>>>> >>>>>>> On Wed, 1 Feb 2023, Thomas Monjalon wrote: >>>>>>> >>>>>>>> 31/01/2023 17:17, Jerin Jacob: >>>>>>>>> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon >>>>> >>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>> 27/01/2023 11:42, Nithin Kumar Dabilpuram: >>>>>>>>>>> From: Thomas Monjalon >>>>>>>>>>>> 27/01/2023 06:02, Nithin Kumar Dabilpuram: >>>>>>>>>>>>> From: Thomas Monjalon >>>>>>>>>>>>>> Ferruh is proposing to have a command "port config >>>>> ..." >>>>>>>>>>>>>> to configure the flags to negotiate. >>>>>>>>>>>>>> Are you OK with this approach? >>>>>>>>>>>>> >>>>>>>>>>>>> Yes, we are fine to have such command to enable and disable the >>>>>>>>>>>>> feature >>>>>>>>>>>>> with default being it disabled if supported by PMD. >>>>>>>>>>>>> Is default being disabled fine if the feature is supported by a >>>>>>>>>>>>> PMD ? >>>>>>>>>>>> >>>>>>>>>>>> I think the default should be enabled for ease of use. >>>>>>>>>>> >>>>>>>>>>> Since testpmd is used extensively for benchmarking purposes, we >>>>>>>>>>> thought it should have minimum features >>>>>>>>>>> enabled by default. The default testpmd doesn't have any Rx/Tx >>>>>>>>>>> offloads enabled(except for FAST FREE), default >>>>>>>>>>> fwd mode being "iofwd" and the Rx metadata is only referenced >>>>> when >>>>>>>>>>> dumping packets. >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> Do we have similar features disables by default? >>>>>>>>>>>> I mean do we know features in testpmd which require a "double >>>>>>>>>>>> enablement" >>>>>>>>>>>> like one configuration command + one rte_flow rule? >>>>>>>>>>> >>>>>>>>>>> Spec itself is that way i.e "RTE_FLOW_RULE + >>>>>>>>>>> RX_METADATA_NEGOTIATE(once)" >>>>>>>>>>> >>>>>>>>>>> Isn't it enough if >>>>>>>>>>> >>>>>>>>>>> #1 We have enough print when rte_flow is being create without >>>>>>>>>>> negotiation done and ask user to enable rx metadata using >>>>>>>>>>> "port config ..." >>>>>>>>>>> #2 Provide testpmd app arg to enable Rx metadata(for example " >>>>>>>>>>> --rx-metadata") like other features to avoid calling another >>>>>>>>>>> command before rte flow create. >>>>>>>>>> >>>>>>>>>> I'm not sure what is best. >>>>>>>>>> I will let others discuss this part. >>>>>>>>> >>>>>>>>> IMO, enabling something always defeat the purpose to negotiate. IMO, >>>>>>>>> someone needs to negotiate >>>>>>>>> for a feature if the feature is needed. I think, the double enablement >>>>>>>>> is part of the spec itself. In case, The PMD >>>>>>>>> drivers won't like double enablement, no need to implement the PMD >>>>>>>>> callback. That way, there is no change in the existing flow. >>>>>>>>> >>>>>>>>> The reason why cnxk driver thought of leveraging negotiate() feature >>>>>>>>> so that it helps for optimization. e.s.p >>>>>>>>> function template for multiprocess case as we know what features >>>>>>>>> needed in fastpath upfront. >>>>>>>>> >>>>>>>>> If there still concerns with patch we can take up this to TB decide >>>>>>>>> one way or another to make forward progress. Let us know. >>>>>>>> >>>>>>>> Ferruh, Andrew, Ori, Ivan, what is your opinion? >>>>>>>> Let's progress with this patch to make it in -rc1. >>>>>> >>>>>> As I understand all agreed that we need testpmd command to >>>>>> control negotiated Rx metadata. May be even command-line >>>>>> option would be useful. >>>>>> >>>>>> So, remaining question is what should be the default value in >>>>>> testpmd. Note that it is just testpmd question since default >>>>>> value in an abstract application is nothing negotiated >>>>>> (if I'm not mistaken). >>>>>> >>>>>> The key advantaan ge of the current behaviour is to avoid >>>>>> "double-enabling" in testpmd. It preserves behaviour which >>>>>> we had before before the API addition. It is a strong >>>>>> argument. Basically it puts the feature into the same >>>>>> basket as FAST_FREE - need an action to run faster. >>>>> >>>>> I think, there is a disconnect here. FAST_FREE is enabled by default. >> >> Sorry, I'm lost here. Don't we need to enable FAST_FREE >> offload? As far as I know all offloads are disabled by default. > > Not the case for FAST_FREE as disabling needs "more cycles on processor" side. > > See app/test-pmd/testpmd.c > /* > * Ethernet device configuration. > */ > struct rte_eth_rxmode rx_mode; > > struct rte_eth_txmode tx_mode = { > .offloads = RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE, > }; > Surprised, thanks. So, it one more difference of the testpmd defaults from an abstract application. >> >>>>> i.e We don't need any specific action to run faster. To align with performance >>>>> test application, by default the configuration should be run faster. User >>>>> needs to give explicit configuration to allow more offload or the one causes >>>>> the mpps drops. IMO, That is the theme followed in testpmd. >>>>> >>>>> >>>> I agree with Andrew, the default should stay the same, as now, PMD may already implement >>>> logic to only enable the feature if there is a flow rule. >>>> Changing the default will result in breaking applications. >>> >>> That's not what is discussed here. >>> We are talking only about testpmd default. >>> >>>> I want to suggest new approach for this feature, >>>> maybe we can use the rte_flow_configure, and add a new bit that says if those >>>> actions are going to be used. >>>> What do you think? >>> >>> Let's not change the API please. >>> >>> >>>>>> I see no problem in such approach. >>>>>> >>>>>> The key disadvantage is the difference in testpmd and >>>>>> other applications default behaviour. >>>>>> >>>>>> I'd look at the feature in the following way: >>>>>> if an application theoretically wants to use >>>>>> USER_FLAG, USER_MARK or TUNNEL_ID it must negotiate >>>>>> corresponding Rx metadata to ensure that the feature is >>>>>> available and HW is informed that application may need it. >>>>>> Since testpmd supports corresponding flow API actions and >>>>>> flow tunnels, it tries to negotiate it by default, but do >>>>>> not fail if the negotiation fails. >>>>>> >>>>>> So, I'd would vote to keeping the default value as is. Two above paragraphs still stand. 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.