Re: [EXT] Re: [PATCH v5 2/2] app/testpmd: add command to process Rx metadata negotiation

[Thomas Monjalon <thomas@monjalon.net>]

31/01/2023 17:17, Jerin Jacob:
> On Fri, Jan 27, 2023 at 8:31 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > 27/01/2023 11:42, Nithin Kumar Dabilpuram:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 27/01/2023 06:02, Nithin Kumar Dabilpuram:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > Ferruh is proposing to have a command "port config <port_id> ..."
> > > > > > 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 <port_id>..."
> > > #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.