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 2FBD441B9D; Wed, 1 Feb 2023 11:48:39 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 1DC9A406A2; Wed, 1 Feb 2023 11:48:39 +0100 (CET) Received: from mail-ua1-f45.google.com (mail-ua1-f45.google.com [209.85.222.45]) by mails.dpdk.org (Postfix) with ESMTP id D9CD04021F for ; Wed, 1 Feb 2023 11:48:37 +0100 (CET) Received: by mail-ua1-f45.google.com with SMTP id q19so3425486uac.10 for ; Wed, 01 Feb 2023 02:48:37 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=OoPF7tNH8VM2kOmJVb1to4EdDX9Cu6hfzGZWmjUcs4Y=; b=HzwE9WNyPF7HvxKZj2PsCFnoIz4jquE99SNVEaIAyA6syXEj67aDHcXGj+V+dB+Uu5 h6fjZYH20rqDIq4qxAuU0FcYLlPnSN6nb7EZf8j1WHqgcxyn5Pgs7XTOtbR2lRva0LhX 7ogKcEW16HWP8h4uk1r7iwDi+4zEBqvmlT6sOShV4KdTsmIvsWH6Ff8g8LJjrSkTwstr n1Fv5elTJvFWk89NqQTjL2kH5aDy5xIdy4u3fnsFZZbir2g34uhlQy7txvzU+dVFM8Hj PP/fTOaAh9qZLgMli7hZDhmMEvCoe75brXwEeQU73yw9gnvojcPVLex58NtgWstGXU76 pUrQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=OoPF7tNH8VM2kOmJVb1to4EdDX9Cu6hfzGZWmjUcs4Y=; b=YO16YPegS29Pv5B3JqdfSw3iPU5em9pXqKe9h/2sfG/0evsJyQQ7j6JFWoUIYfoJEv ifzultBUUmGk+OB8wvpg1BK/kBTJehgdrtVVODAiHCKt/K4SRTGrhZ0jCtxoGVVOlySx k3/bRGZxaZN/yoBuKjH1JKO87MNOxgq0U+AiAAB1DRrd1UkSVfAjWeXWvK86ODAHB6DJ KGCniMaxvgjiP4VDjtl5K/X9wPzwssZE4ilxlO1SP0J7izAFUwPvvc+ugBf7k1Q0digJ gb58efEpSFX97NHt+YjeTKKUzcs06kpikxoNi8efaWamZ110Sx9I33i4ZqDmv+FjCx39 yOmA== X-Gm-Message-State: AO0yUKXneGuocDt5ydBEqeePsIAL5TA3M+a4lgvaqjruUOUvb73rKC50 vWncoFtnhbyjO+yf0VD25dd6akl46/La78m77Gw= X-Google-Smtp-Source: AK7set/OaqaskdfBRETFAyvlRmjslvRlcvL23Xq5mK0V+DyAVDQn5H0W8L4rtfKxjdufNYMSwrcsTQiWUduFZPWrss8= X-Received: by 2002:ab0:3418:0:b0:66c:f3bf:fc86 with SMTP id z24-20020ab03418000000b0066cf3bffc86mr295946uap.24.1675248517117; Wed, 01 Feb 2023 02:48:37 -0800 (PST) MIME-Version: 1.0 References: <20221220200250.2413443-1-hpothula@marvell.com> <3180381.AJdgDx1Vlc@thomas> <0e383338-0bd8-8f1c-00f1-50605030d84f@oktetlabs.ru> <056883f1-72c8-3540-b2ad-e12d1cfc4dae@oktetlabs.ru> In-Reply-To: <056883f1-72c8-3540-b2ad-e12d1cfc4dae@oktetlabs.ru> From: Jerin Jacob Date: Wed, 1 Feb 2023 16:18:10 +0530 Message-ID: Subject: Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation To: Andrew Rybchenko 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" Content-Type: text/plain; charset="UTF-8" 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 Wed, Feb 1, 2023 at 2:59 PM Andrew Rybchenko wrote: > > 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. It is not, even l2fwd is same as testpmd. In my view, the ideal application just to select what is really needed. > > >> > >>>>> 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. But none of the other applications in not negotiating by default. > >>>>>> > >>>>>> 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. 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. 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?