Re: [PATCH net-next 0/2][pull request] 10GbE Intel Wired LAN Driver Updates 2022-03-10

[Leon Romanovsky <leon@kernel.org>]

On Fri, Mar 11, 2022 at 09:19:23AM -0600, Samudrala, Sridhar wrote:
> On 3/10/2022 10:34 PM, Leon Romanovsky wrote:
> > On Thu, Mar 10, 2022 at 03:12:33PM -0800, Tony Nguyen wrote:
> > > Sudheer Mogilappagari says:
> > > 
> > > Add support to enable inline flow director which allows uniform
> > > distribution of flows among queues of a TC. This is configured
> > > on a per TC basis using devlink interface.
> > > 
> > > Devlink params are registered/unregistered during TC creation
> > > at runtime. To allow that commit 7a690ad499e7 ("devlink: Clean
> > > not-executed param notifications") needs to be reverted.
> > > 
> > > The following are changes since commit 3126b731ceb168b3a780427873c417f2abdd5527:
> > >    net: dsa: tag_rtl8_4: fix typo in modalias name
> > > and are available in the git repository at:
> > >    git://git.kernel.org/pub/scm/linux/kernel/git/tnguy/next-queue 10GbE
> > > 
> > > Kiran Patil (1):
> > >    ice: Add inline flow director support for channels
> > > 
> > > Sridhar Samudrala (1):
> > >    devlink: Allow parameter registration/unregistration during runtime
> > Sorry, NO to whole series.
> > 
> > I don't see any explanation why it is good idea and must-to-be
> > implemented one to configure global TC parameter during runtime.
> 
> This parameter is applicable only after splitting the netdevice queues into
> queue groups(TCs) via tc mqprio command.
> The queue groups can be created/destroyed during runtime.
> So the patch is trying to register/unregister this parameter when TCs are
> created and destroyed.
> 
> > 
> > You created TC with special tool, you should use that tool to configure
> > TC and not devlink. Devlink parameters can be seen as better replacement
> > of module parameters, which are global by nature. It means that this
> > tc_inline_fd can be configured without relation if TC was created or
> > not.
> 
> Extending tc qdisc mqprio to add this parameter is an option we could explore.
> Not sure if it allows changing parameters without reloading the qdisc.

It was one of the options. I don't mind if it is going to be in devlink
or TC, as long as devlink_params_register() is not changed to be dynamic.

> 
> > 
> > I didn't look too deeply in revert patch, but from glance view it
> > is not correct too as it doesn't have any protection from users
> > who will try to configure params during devlink_params_unregister().
> 
> Is there any limitation that devlink params can be registered only during
> probe time?

Yes, we don't want races between userspace that tries to access devlink
parameters and kernel code that adds/deletes them.

> Would it be OK if we register this parameter during probe time, but allow
> changing it only after TCs are created?

Of course, this is exactly how it is supposed to be.
For an example, see commit e890acd5ff18 ("net/mlx5: Add devlink flow_steering_mode parameter")

In your case, you will have some sort of "if (!tc_configured) ..." line
that will prevent to set this parameter.

Thanks

> 
> -Sridhar
>