Re: [Intel-wired-lan] [PATCH v5 net-next 02/12] net-shapers: implement NL get operation

[Paolo Abeni <pabeni@redhat.com>]

Hi,

Please allow me to put a few high level questions together, to both 
underline them as most critical, and keep the thread focused.

On 8/30/24 03:20, Jakub Kicinski wrote:
 > This 'binding' has the same meaning as 'binding' in TCP ZC? :(

I hope we can agree that good naming is difficult. I thought we agreed 
on such naming in the past week’s discussion. The term 'binding' is 
already used in the networking stack in many places to identify 
different things (i.e. device tree, socket, netfilter.. ). The name 
prefix avoids any ambiguity and I think this a good name, but if you 
have any better suggestions, this change should be trivial.

[about per device shaper lock]
 > I've been wondering if we shouldn't move this lock
 > directly into net_device and combine it with the RSS lock.
 > Create a "per-netdev" lock, instead of having multiple disparate
 > mutexes which are hard to allocate?

The above looks like a quite unrelated refactor and one I think it will 
not be worthy. The complexity of locking code in this series is very 
limited, and self-encapsulated. Different locks for different things 
increases scalability. Possibly we will not see much contention on the 
same device, but some years ago we did not think there would be much 
contention on RTNL...

Additionally, if we use a per _network device_ lock, future expansion of 
the core to support devlink objects will be more difficult.

[about separate handle from shaper_info arguments]
 > Wouldn't it be convenient to store the handle in the "info"
 > object? AFAIU the handle is forever for an info, so no risk of it
 > being out of sync…

Was that way a couple of iterations ago. Jiri explicitly asked for the 
separation, I asked for confirmation and nobody objected.

Which if the 2 options is acceptable from both of you?

[about queue limit and channel reconf]
 > we probably want to trim the queue shapers on channel reconfig,
 > then, too? :(

what about exposing to the drivers an helper alike:

	net_shaper_notify_delete(binding, handle);

that tells the core the shaper at the given handle just went away in the 
H/W? The driver will call it in the queue deletion helper, and such 
helper could be later on used more generically, i.e. for vf/devlink port 
deletion.

[about capabilities support]
 > It's not just for introspection, it's also for the core to do
 > error checking.

Actually, in the previous discussions it was never mentioned to use 
capabilities to fully centralize the error checking.

This really looks like another feature, and can easily be added in a 
second time (say, a follow-up series), with no functionality loss.

I (or anybody else) can’t keep adding new features at every iteration. 
At some point we need to draw a line, and we should agree that the scope 
of this activity has already expanded a lot in the past year. I would 
like to draw such a line here.

Thanks,

Paolo