Re: [PATCH v3 03/12] net-shapers: implement NL get operation

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 30 Jul 2024 22:39:46 +0200 Paolo Abeni wrote:
> +#include <linux/types.h>
> +#include <linux/bits.h>
> +#include <linux/bitfield.h>
> +#include <linux/netdevice.h>
> +#include <linux/netlink.h>

most of these could be replaced by forward declaration of types

> +#include <uapi/linux/net_shaper.h>
> +
> +/**
> + * struct net_shaper_info - represents a shaping node on the NIC H/W
> + * zeroed field are considered not set.
> + * @handle: Unique identifier for the shaper, see @net_shaper_make_handle
> + * @parent: Unique identifier for the shaper parent, usually implied. Only
> + *   NET_SHAPER_SCOPE_QUEUE, NET_SHAPER_SCOPE_NETDEV and NET_SHAPER_SCOPE_DETACHED
> + *   can have the parent handle explicitly set, placing such shaper under
> + *   the specified parent.
> + * @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
> + * @children: Number of nested shapers, accounted only for DETACHED scope
> + */
> +struct net_shaper_info {
> +	u32 handle;
> +	u32 parent;
> +	enum net_shaper_metric metric;
> +	u64 bw_min;
> +	u64 bw_max;
> +	u64 burst;
> +	u32 priority;
> +	u32 weight;
> +	u32 children;
> +};
> +
> +/**
> + * define NET_SHAPER_SCOPE_VF - Shaper scope
> + *
> + * This shaper scope is not exposed to user-space; the shaper is attached to
> + * the given virtual function.
> + */
> +#define NET_SHAPER_SCOPE_VF __NET_SHAPER_SCOPE_MAX
> +
> +/**
> + * struct net_shaper_ops - Operations on device H/W shapers
> + *
> + * The initial shaping configuration ad device initialization is empty/
> + * a no-op/does not constraint the b/w in any way.
> + * The network core keeps track of the applied user-configuration in
> + * per device storage.
> + *
> + * Each shaper is uniquely identified within the device with an 'handle',
> + * dependent on the shaper scope and other data, see @shaper_make_handle()

we need to document locking, seems like the locking is.. missing?
Or at least I don't see what prevents two deletes from racing.

> + */
> +struct net_shaper_ops {
> +	/**
> +	 * @group: create the specified shapers group
> +	 *
> +	 * Nest the specified @inputs shapers under the given @output shaper
> +	 * on the network device @dev. The @input shaper array size is specified
> +	 * by @nr_input.
> +	 * Create either the @inputs and the @output shaper as needed,
> +	 * otherwise move them as needed. Can't create @inputs shapers with
> +	 * NET_SHAPER_SCOPE_DETACHED scope, a separate @group call with such
> +	 * shaper as @output is needed.
> +	 *
> +	 * Returns 0 on group successfully created, otherwise an negative
> +	 * error value and set @extack to describe the failure's reason.
> +	 */
> +	int (*group)(struct net_device *dev, int nr_input,
> +		     const struct net_shaper_info *inputs,
> +		     const struct net_shaper_info *output,
> +		     struct netlink_ext_ack *extack);
> +

> +	/* the default id for detached scope shapers is an invalid one
> +	 * to help the 'group' operation discriminate between new
> +	 * detached shaper creation (ID_UNSPEC) and reuse of existing
> +	 * shaper (any other value)
> +	 */
> +	id_attr = tb[NET_SHAPER_A_ID];
> +	if (id_attr)
> +		id =  nla_get_u32(id_attr);

double space