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

[Thomas Monjalon]

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.

> #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.