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

[Thomas Monjalon <thomas@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@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@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".