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

[Thomas Monjalon]

Please reply inline below instead of doing an incomplete copy
of the replies on top.


25/01/2023 15:42, Nithin Kumar Dabilpuram:
> > >Will it work to enable them all by default and add capability to disable
> > >it in testpmd, which helps to run performance tests also to verify the
> > > impact of the API?
> 
> The spirit of the negotiating features/Rx/Tx offloads upfront is to have it disabled by default and enable the feature only when needed. Having the features enabled by default is probably against that spirit.
> 
> We understand the concerns with drivers that didn't not implement that API.

There is no such concern I think.

> Why not disable it by default(like other offloads) and modify rte_flow action creation in testpmd to check for if !ENOSUP and feature disabled and add print to enable. So for the PMD's that won't support rte_eth_rx_metadata_negotiate(), there won't be any difference and for very few PMD's that support this API, they need to enable it before using RTE_FLOW with MARK/FLAG.
> Behavior change would be seen only with two PMD's(cnxk, sfc).

I think you missed the whole point.
Ferruh is proposing to have a command "port config <port_id> ..."
to configure the flags to negotiate.
Are you OK with this approach?

> > Note: I don't understand why we don't have
> > RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> > negotiated in this function. Probably something to add.
> 
> The purpose of negotiate is to tell the PMD upfront so that PMD can prepare
> HW appropriately.  Having these new actions would be very late to inform PMD and
> I think won't solve the purpose.

I am not talking about your problem here.
I am just saying that TAG and META should be negotiated as well
in rte_eth_rx_metadata_negotiate().

> From: Thomas Monjalon <thomas at monjalon.net>
> > 25/01/2023 14:55, Ferruh Yigit:
> > > On 1/25/2023 12:55 PM, Thomas Monjalon wrote:
> > > > 25/01/2023 10:30, Hanumanth Reddy Pothula:
> > > >> ++ Ivan Malov and Andrew Rybchenko
> > > >>
> > > >> From: Ferruh Yigit <ferruh.yigit at amd.com>
> > > >>> On 12/21/2022 2:07 AM, Hanumanth Pothula wrote:
> > > >>>> Presently, Rx metadata is sent to PMD by default, leading to a
> > > >>>> performance drop as processing for the same in Rx path takes extra
> > > >>>> cycles.
> > > >>>>
> > > >>>> Hence, add new testpmd command,
> > > >>>>   'enable port <port_id> nic_to_pmd_rx_metadata'
> > > >>>>
> > > >>>> This command helps in sending Rx metadata to PMD and thereby Rx
> > > >>>> metadata flow command requests are processed.
> > > >>>>
> > > >>>> Signed-off-by: Hanumanth Pothula <hpothula at marvell.com>
> > > >>>
> > > >>> Hi Hanumanth,
> > > >>>
> > > >>> I agree with Thomas for the patch.
> > > >>>
> > > >>> 'eth_rx_metadata_negotiate_mp()' requests all Rx metadata offloads to be
> > > >>> enabled, but at this stage if there is no flow rule for Rx metadata why it is
> > > >>> consuming extra cycles?
> > > >>>
> > > >>> Can you update driver code to process Rx metadata when it is enabled by
> > > >>> application (via 'rte_eth_rx_metadata_negotiate()') AND there is at least
> > > >>> one flow rule for it?
> > > >>
> > > >> #1 What is the purpose of rte_eth_rx_metadata_negotiate() API if it is always called by
> > testpmd.
> > > >> We thought it was added so that when that metadata is not needed, application need
> > not call this
> > > >> thereby saving cycles/bandwidth.
> > > >
> > > > testpmd is for testing all features. That's why all is negotiated.
> > > > Cycles should be saved if you don't enable it until a flow rule requires it.
> > > >
> > >
> > > Hi Thomas,
> > >
> > > Not just for saving cycles, but from testing perspective too, do you
> > > think does it work if a way to disable these Rx metadata added by
> > > keeping default behavior as it is?
> > >
> > > And new command can be in a consistent command syntax like:
> > > "port config <port_id> ..."
> > 
> > Yes I agree it would be good to have a way to test different values.
> > And it would allow to completely disable metadata I suppose.
> > 
> > Note: I don't understand why we don't have
> > RTE_FLOW_ACTION_TYPE_SET_TAG and RTE_FLOW_ACTION_TYPE_SET_META
> > negotiated in this function. Probably something to add.
> > 
> > 
> > > >> #2 We use this API similar to Rx/Tx offload flags so that we can set things up before
> > device is
> > > >> configured. We thought that is the purpose of having this negotiate API and avoid
> > depleting offload flags.
> > > >
> > > > It is just a configuration negotiation specific to metadata.
> > > >
> > > >> #3 Generally any new offloads added to DPDK would be in disabled state in testpmd
> > and we would have
> > > >> an option to enable it. In this case, testpmd is by default calling this negotiation.
> > > >
> > > > Negotiating is not enabling.
> > > >
> > > >> We can update the driver if the purpose of this API is clear.
> > > >
> > > > Please do.
> > >
> > > Is following understanding correct?
> > >
> > >      API        Flow Rule       Result
> > >     -----    ------------     --------
> > >     Enable    No Rule	       Feature Disabled
> > >     Enable    Rule exist       Feature Enabled
> > >     Disable     X              Feature Disabled
> > 
> > In the API column, you should say "negotiated" instead of "Enable".
> > 
> > 
> 
>