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 C9B8B41B9D; Wed, 1 Feb 2023 10:08:00 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B6D9240DFB; Wed, 1 Feb 2023 10:08:00 +0100 (CET) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 6F0DE406A2 for ; Wed, 1 Feb 2023 10:07:58 +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)) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id D849D50; Wed, 1 Feb 2023 12:07:57 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru D849D50 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1675242477; bh=ZOaPiOAkpTCcTGtuzvT3Yzmi001dzncWQUsmanpuFFc=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=Ox/mJSvyTr3j1MAx2dDJ+yt1MrSaJKvC+7roNluosoXuAiam5c1lDfxpjF4A3nsi5 +j9wZJxegSpAgOYY5Aj/pB+AmWWRfBmn+pzIDaZbX6Rl6Zhjn0yc9Sm37nICV+EwmT 1v2UKe32DciRCgg/BMvIQ0dhROIPAClyt56+WyuM= Message-ID: <0e383338-0bd8-8f1c-00f1-50605030d84f@oktetlabs.ru> Date: Wed, 1 Feb 2023 12:07:57 +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: Thomas Monjalon , Jerin Jacob , Ori Kam Cc: 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> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <3180381.AJdgDx1Vlc@thomas> 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: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. >>> 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. >>>> >> > > > > >