Re: [PATCH V3 3/7] net/mlx5: add new devargs to control probe optimization

[Thomas Monjalon <thomas@monjalon.net>]

04/11/2024 09:36, Slava Ovsiienko:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 29/10/2024 17:20, Stephen Hemminger:
> > > On Tue, 29 Oct 2024 15:42:52 +0200
> > > "Minggang Li(Gavin)" <gavinl@nvidia.com> wrote:
> > >
> > > > From: Rongwei Liu <rongweil@nvidia.com>
> > > >
> > > > Add a new devarg probe_opt_en to control probe optimization in PMD.
> > > >
> > > > By default, the value is 0 and no behavior changed.
> > > >
> > > > Signed-off-by: Rongwei Liu <rongweil@nvidia.com>
> > > > Acked-by: Viacheslav Ovsiienko <viacheslavo@nvidia.com>
> > >
> > > Once again, every option you introduce expands the test space by 2X.
> > > "Do or Do not. There is no try"
> > > Either it works all the time or it is a bad idea.
> > 
> > I fully agree.
> > We should not merge this series before providing a good answer, or making
> > it automatic.
> > 
> > One more thing: a commit log should always explain "why".
> > Here it should say why it is not automatic.
> > Is there a good reason to disable this feature?
> 
> The feature is event-driven and depends on the system/DPDK environment.
> Example:
> - DPDK handles interrupts in the single dedicated EAL thread
> - the failsafe PMD in "interrupt"  handler  performs device probe() action,
> that might take a long time and DPDK event/interrupt handle experiences the
> significant delays, sometime causing the malfunction.
> 
> Another concern - Netlink buffers have limited capacity, and with high message
> rate might be overflown.
> 
> This is just the examples, sure, most of the time feature works reliably.
> The feature is needed for few users only, that's why we follow conservative
> approach. Do you think we should put all the stuff above in the commit log?

Yes the reasoning must be explained in the commit log.
And the user documentation must explained clearly when to use it or not.

But honestly, I feel it would be better to make sure it works always
and avoid the option.