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 B180041B9D; Wed, 1 Feb 2023 10:15:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 5D4FB42D48; Wed, 1 Feb 2023 10:15:17 +0100 (CET) Received: from mail-ua1-f50.google.com (mail-ua1-f50.google.com [209.85.222.50]) by mails.dpdk.org (Postfix) with ESMTP id 33CE842D3B for ; Wed, 1 Feb 2023 10:15:15 +0100 (CET) Received: by mail-ua1-f50.google.com with SMTP id u29so3385874uaa.8 for ; Wed, 01 Feb 2023 01:15:15 -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=cg3KWimcKRuuT2mCoyt2SH7C8VLhoEOlprrmL5zXzrU=; b=KXDvydJg9qdp5+lE40Lj30GFqteCw6M1ZYb5DMTpkaOvWUSSmlJUg3DU9EEtIKmhRQ etaFp8WhdFWp36/E2cO0SDS2feTT8Ek1RGPf4Yd4/fv05pGAsIUNVYZty1KEv6vQiA7W XCOUtXn2QSValNnddDRs1FuXBGoT0ip0niw2ekCVENGdoNDUu7QjvkPwrjYnjxac0H8B 7H2A134cE3NSAlb0/ie2OG0liGGkPnr2t2GuCTx/beJL5lXGM0MG/497YQeU5O/ZZfWS EjwcVQkS3V7MmVuAzOR9x1l3ST/4fMuuhTHuRfJYsnICxWfum4SgkndaUIxA39h5DyoK fSQA== 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=cg3KWimcKRuuT2mCoyt2SH7C8VLhoEOlprrmL5zXzrU=; b=pc4H629r9xMuk+7sYc0erGvwstUzlo/hwKi5IR6fiD0dJIma+vqdNRG8VH6ckvpaBr rA0NjorbS0OIX4pbwepB52/VOqtItXv/yV29E2YOhJMfvA7WSrcErWKpTVgWW/jgZ2Kz CHI//K77tnoMPECUqvb+auh9Ll+McIkuD84yrqtdY/j8ma7fdqvqh4clNPeerbI0WQxb 75wYWt0KNC+RLQQDoVHYPHt6WCgm1iVSieSALXT/lDSMeCv90NrfblvD5YerqGHB2cso VWzfxyqsUYBIP81kbx4ldldYPBQk3RtQNgjKET3S4Nip8mBIadETM6/nSc7E32KjKCMg PJKA== X-Gm-Message-State: AO0yUKUsPNDAvpAYsg712qDj1TMnWzhNt+OLHfcGcv7i57rkjIfPlev9 Qy8c/K02zFfgEBZFBO7mJrKcoNl+A5c5tR2mdK8= X-Google-Smtp-Source: AK7set/S6ZIMfphLvvown4k4vDCd0whQpcZLnV2ijcyLwizIVQQZSmNr9y8geluqZTj/zMjDA4x+r+bwtbicen7FEJ4= X-Received: by 2002:ab0:3418:0:b0:66c:f3bf:fc86 with SMTP id z24-20020ab03418000000b0066cf3bffc86mr256559uap.24.1675242914518; Wed, 01 Feb 2023 01:15:14 -0800 (PST) MIME-Version: 1.0 References: <20221220200250.2413443-1-hpothula@marvell.com> <3180381.AJdgDx1Vlc@thomas> <0e383338-0bd8-8f1c-00f1-50605030d84f@oktetlabs.ru> In-Reply-To: <0e383338-0bd8-8f1c-00f1-50605030d84f@oktetlabs.ru> From: Jerin Jacob Date: Wed, 1 Feb 2023 14:44:47 +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: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, }; > > >>> 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. > >>>> > >> > > > > > > > > > > >