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
