[EXT] Re: [PATCH v3 1/1] app/testpmd: control passing Rx metadata to PMD

[Thomas Monjalon]

05/12/2022 08:59, Hanumanth Reddy Pothula:
> From: Thomas Monjalon <thomas at monjalon.net>
> > 02/12/2022 17:14, Hanumanth Reddy Pothula:
> > > -----Original Message-----
> > > > 27/10/2022 09:34, Thomas Monjalon:
> > > > > 17/10/2022 10:32, Andrew Rybchenko:
> > > > > > On 10/6/22 21:35, 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, introducing command line argument, 'nic-to-pmd-rx-
> > metadata'
> > > > > > > to control passing rx metadata to PMD. By default it’s disabled.
> > > > > > >
> > > > > > > Signed-off-by: Hanumanth Pothula <hpothula at marvell.com>
> > > > > >
> > > > > > Acked-by: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> > > > > >
> > > > > > Applied to dpdk-next-net/main, thanks.
> > > > >
> > > > > I'm not sure this patch is really acceptable.
> > > > > It is disabling Rx metadata by default just for benchmarking
> > > > > reason because your driver is doing some processing even if
> > > > > metadata is not
> > > > required.
> > > > >
> > > > > From a user perspective, if a command requesting metadata is
> > > > > entered, it won't work until we enable this new option on startup.
> > > > > It looks terrible.
> > > > >
> > > > > Please tell me I misunderstood something.
> > > >
> > > > While pulling, I see that the name is not compliant with others.
> > > > I think it should start with "enable-", use hyphens and be sorted.
> > > >
> > > > I'll drop it from the pull for now, we can have it in -rc3.
> > > >
> > >
> > > @Thomas Monjalon I missed your comment, sorry for the delayed
> > response.
> > >
> > > Sending Rx metadata to PMD is added recently, which breaking our driver
> > performance.
> > > Normally any feature added to testpmd will be disabled by default, to
> > make sure it won't affect other code(PMD).
> > > Hence adding new testpmd command line argument to disable this
> > feature by default.
> > 
> > No, disabling by default doesn't mean you should enable with option.
> > It should be enabled if required by a command.
> > 
> Yeah, got it, user can enable/disable the feature on the fly without restarting the application(testpmd).
> Will introduce new command, like below, 
>   'port config rx-nic-to-pmd-metadata on/off'
> 
> Please let me know your thoughts on this command implementation.
> If it's fine, will start implementing the command. Suggest if there is any other better way to implement the same.

What about enabling automatically when a flow command requests metadata?