Re: [RFC PATCH] net: introduce HW Rate Limiting Driver API

[Simon Horman <horms@kernel.org>]

On Fri, May 10, 2024 at 01:05:41PM +0200, Paolo Abeni wrote:
> On Thu, 2024-05-09 at 18:17 +0200, Andrew Lunn wrote:
> > > > Now the question is, how do i get between these two states? It is not
> > > > possible to mix WRR and strict priority. Any kAPI which only modifies
> > > > one queue at once will go straight into an invalid state, and the
> > > > driver will need to return -EOPNOTSUPP. So it seems like there needs
> > > > to be an atomic set N queue configuration at once, so i can cleanly go
> > > > from strict priority across 8 queues to WRR across 8 queues. Is that
> > > > foreseen?
> > > 
> > > You could delete all the WRR shapers and then create/add SP based ones.
> > 
> > But that does not match the hardware. I cannot delete the hardware. It
> > will either do strict priority or WRR. If i delete the software
> > representation of the shaper, the hardware shaper will keep on doing
> > what it was doing. So i don't see this as a good model. I think the
> > driver will create shapers to represent the hardware, and you are not
> > allowed to delete them or add more of them, because that is what the
> > hardware is. All you can do is configure the shapers that exist.
> > 
> > > The 'create' op is just an abstraction to tell the NIC to switch from
> > > the default configuration to the specified one.
> > 
> > Well, the hardware default is i think WRR for the queues, and line
> > rate. That will be what the software representation of the shapers
> > will be set to when the driver probes and creates the shapers
> > representors.
> 
> If I read correctly, allowing each NIC to expose it's own different
> starting configuration still will not solve the problem for this H/W to
> switch from WRR to SP (and vice versa).
> 
> AFAICS, what would be needed there is an atomic set of operations:
> 'set_many' (and e.v. 'delete_many', 'create_many') that will allow
> changing all the shapers at once. 
> 
> With such operations, that H/W could still fit the expected 'no-op'
> default, as WRR on the queue shapers is what we expect. I agree with
> Jakub, handling the complexity of arbitrary starting configuration
> would pose a lot of trouble to the user/admin.
> 
> If all the above stands together, I think we have a few options (in
> random order):
> 
> - add both set of operations: the ones operating on a single shaper and
> the ones operating on multiple shapers
> - use only the multiple shapers ops.
> 
> And the latter looks IMHO the simple/better. At that point I would
> probably drop the 'add' op and would rename 'delete' as
> 'reset':
> 
> int (*set)(struct net_device *dev, int how_many, const u32 *handles,
> 	   const struct net_shaper_info *shapers,
>            struct netlink_ext_ack *extack);
> int (*reset)(struct net_device *dev, int how_many, const u32 *handles,
>              struct netlink_ext_ack *extack);
> int (*move)(struct net_device *dev, int how_many, const u32 *handles,
>             const u32 *new_parent_handles,
> 	    struct netlink_ext_ack *extack);
> 
> An NIC with 'static' shapers can implement a dummy move always
> returning EOPNOTSUPP and eventually filling a detailed extack.
> 
> NIC without any constraints on mixing and matching different kind of
> shapers could implement the above as a loop over whatever they will do
> for the corresponding 'single shaper op'
> 
> NIC with constrains alike the one you pointed out could validate the
> final state before atomically applying the specified operation.
> 
> After a successful  'reset' operation, the kernel could drop any data
> it retains/caches for the relevant shapers - the current idea is to
> keep a copy of all successfully configured shaper_info in a xarray,
> using the 'handle' as the index.
> 
> Side note: the move() operation could look like a complex artifact, but
> it's the simplest instrument I could think of to support scenarios
> where the user creates/configures/sets a queue group and 'move' some
> queue under the newly created group
> 
> WDYT?

Hi Andrew,

Picking up this discussion, I'm wondering if Paolo's proposal
addresses your concerns.