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

[Simon Horman <horms@kernel.org>]

On Wed, May 15, 2024 at 04:41:09PM +0200, Andrew Lunn wrote:
> > + * struct net_shaper_info - represents a shaping node on the NIC H/W
> > + * @metric: Specify if the bw limits refers to PPS or BPS
> > + * @bw_min: Minimum guaranteed rate for this shaper
> > + * @bw_max: Maximum peak bw allowed for this shaper
> > + * @burst: Maximum burst for the peek rate of this shaper
> > + * @priority: Scheduling priority for this shaper
> > + * @weight: Scheduling weight for this shaper
> > + */
> > +struct net_shaper_info {
> > +	enum net_shaper_metric metric;
> > +	u64 bw_min;	/* minimum guaranteed bandwidth, according to metric */
> > +	u64 bw_max;	/* maximum allowed bandwidth */
> > +	u32 burst;	/* maximum burst in bytes for bw_max */
> > +	u32 priority;	/* scheduling strict priority */
> > +	u32 weight;	/* scheduling WRR weight*/
> > +};
> 
> ...
> 
> > +	/** set - Update the specified shaper, if it exists
> > +	 * @dev: Netdevice to operate on.
> > +	 * @handle: the shaper identifier
> > +	 * @shaper: Configuration of shaper.
> > +	 * @extack: Netlink extended ACK for reporting errors.
> > +	 *
> > +	 * Return:
> > +	 * * %0 - Success
> > +	 * * %-EOPNOTSUPP - Operation is not supported by hardware, driver,
> > +	 *                  or core for any reason. @extack should be set to
> > +	 *                  text describing the reason.
> > +	 * * Other negative error values on failure.
> > +	 */
> > +	int (*set)(struct net_device *dev, u32 handle,
> > +		   const struct net_shaper_info *shaper,
> > +		   struct netlink_ext_ack *extack);
> 
> > + * net_shaper_make_handle - creates an unique shaper identifier
> > + * @scope: the shaper scope
> > + * @vf: virtual function number
> > + * @id: queue group or queue id
> > + *
> > + * Return: an unique identifier for the shaper
> > + *
> > + * Combines the specified arguments to create an unique identifier for
> > + * the shaper.
> > + * The virtual function number is only used within @NET_SHAPER_SCOPE_VF,
> > + * @NET_SHAPER_SCOPE_QUEUE_GROUP and @NET_SHAPER_SCOPE_QUEUE.
> > + * The @id number is only used for @NET_SHAPER_SCOPE_QUEUE_GROUP and
> > + * @NET_SHAPER_SCOPE_QUEUE, and must be, respectively, the queue group
> > + * identifier or the queue number.
> > + */
> > +u32 net_shaper_make_handle(enum net_shaper_scope scope, int vf, int id);
> 
> One thing i'm missing here is a function which does the opposite of
> net_shaper_make_handle(). Given a handle, it returns the scope, vf and
> the id.
> 
> When the set() op is called, i somehow need to find the software
> instance representing the hardware block. If i know the id, it is just
> an array access. Otherwise i need additional bookkeeping, maybe a
> linked list of handles and pointers to structures etc.
> 
> Or net_shaper_make_handle() could maybe take an addition void * priv,
> and provide a function void * net_shape_priv(u32 handle);

Hi Andrew,

I think that, initially at least, the implementation of
net_shaper_make_handle() can be fairly simple, involving packing
fields into an integer in a static manner.

As such I think implementing a helper or helpers to an to extract fields
should be trivial.

In other words, yes, I think that can be added.