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.yi...@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 <hpoth...@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".
