Message-ID: <ZqzIoZaGVb3jIW43@nanopsycho.orion>
Date: Fri, 2 Aug 2024 13:53:05 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.org>,
	Madhu Chittim <madhu.chittim@...el.com>,
	Sridhar Samudrala <sridhar.samudrala@...el.com>,
	Simon Horman <horms@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Sunil Kovvuri Goutham <sgoutham@...vell.com>,
	Jamal Hadi Salim <jhs@...atatu.com>
Subject: Re: [PATCH v3 03/12] net-shapers: implement NL get operation

Tue, Jul 30, 2024 at 10:39:46PM CEST, pabeni@...hat.com wrote:

[...]


>+
>+/**
>+ * 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

"rate" to be consitent with the text one line above?


>+ * @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

"children" are "inputs"? Or "nested shapers". I'm lost in the
terminology, again.


>+ */
>+struct net_shaper_info {
>+	u32 handle;

I wonder if the handle should be part of this structure. The structure
describes the shaper attributes. The handle is identification. In the
ops below, you sometimes pass the handle as a part of net_shaper_info
structure (group, set), yet for delete op you pass handle directly.
Wouldn't it be nicer to pass the same handle attr every time so it is
clear what it is?


>+	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

This is unused, why do you introduce it here?


>+
>+/**
>+ * struct net_shaper_ops - Operations on device H/W shapers
>+ *
>+ * The initial shaping configuration ad device initialization is empty/

s/as/and ?


>+ * a no-op/does not constraint the b/w in any way.

"bw","b/w","rate". Better to be consistent, I suppose.


>+ * The network core keeps track of the applied user-configuration in
>+ * per device storage.

"per device storage" reads weird. "net_device structure"? IDK.


>+ *
>+ * Each shaper is uniquely identified within the device with an 'handle',
>+ * dependent on the shaper scope and other data, see @shaper_make_handle()

The name of the function is net_shaper_make_handle(). You refer to
"other data". What is that? I see no such thing in the code.


>+ */
>+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.

I don't understand the sentense. So this creates either passed inputs
and output shaper, or if they already exist, it links them together in
desired way? Or what do you mean by "move"?


>+                                        Can't create @inputs shapers with
>+	 * NET_SHAPER_SCOPE_DETACHED scope, a separate @group call with such
>+	 * shaper as @output is needed.

I don't understand meaning of the sentence. The caller should make sure
that NET_SHAPER_SCOPE_DETACHED is not in the inputs, so drivers do not
have to sanitize it one by one. Then this sentence is not needed here,
is it?


>+	 *
>+	 * 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,

Perhaps better name would be "nr_inputs" or "inputs_count"?


>+		     const struct net_shaper_info *inputs,
>+		     const struct net_shaper_info *output,
>+		     struct netlink_ext_ack *extack);
>+
>+	/**
>+	 * @set: Updates the specified shaper
>+	 *
>+	 * Updates or creates the specified @shaper on the given device
>+	 * @dev. Can't create NET_SHAPER_SCOPE_DETACHED shapers, use @group
>+	 * instead.

Again, similar to the NET_SHAPER_SCOPE_DETACHED comment above, the
caller should sanitize this. Why is this needed here then?


>+	 *
>+	 * Returns 0 on group successfully created, otherwise an negative

Which group? C&P mistake I suppose?



>+	 * error value and set @extack to describe the failure's reason.
>+	 */
>+	int (*set)(struct net_device *dev,
>+		   const struct net_shaper_info *shaper,
>+		   struct netlink_ext_ack *extack);
>+
>+	/**
>+	 * @delete: Removes the specified shaper from the NIC
>+	 *
>+	 * Removes the shaper configuration as identified by the given @handle
>+	 * on the specified device @dev, restoring the default behavior.
>+	 *
>+	 * Returns 0 on group successfully created, otherwise an negative

Which group? C&P mistake I suppose?


>+	 * error value and set @extack to describe the failure's reason.
>+	 */
>+	int (*delete)(struct net_device *dev, u32 handle,
>+		      struct netlink_ext_ack *extack);
>+};
>+
>+#define NET_SHAPER_SCOPE_SHIFT	26
>+#define NET_SHAPER_ID_MASK	GENMASK(NET_SHAPER_SCOPE_SHIFT - 1, 0)
>+#define NET_SHAPER_SCOPE_MASK	GENMASK(31, NET_SHAPER_SCOPE_SHIFT)
>+
>+#define NET_SHAPER_ID_UNSPEC NET_SHAPER_ID_MASK
>+
>+/**
>+ * net_shaper_make_handle - creates an unique shaper identifier
>+ * @scope: the shaper scope
>+ * @id: the shaper id number
>+ *
>+ * Return: an unique identifier for the shaper
>+ *
>+ * Combines the specified arguments to create an unique identifier for
>+ * the shaper. The @id argument semantic depends on the
>+ * specified scope.
>+ * For @NET_SHAPER_SCOPE_QUEUE_GROUP, @id is the queue group id
>+ * For @NET_SHAPER_SCOPE_QUEUE, @id is the queue number.
>+ * For @NET_SHAPER_SCOPE_VF, @id is virtual function number.
>+ */
>+static inline u32 net_shaper_make_handle(enum net_shaper_scope scope,

Why is this in the header? It is used only in shaper.c. Should be
private to it.


>+					 int id)
>+{
>+	return FIELD_PREP(NET_SHAPER_SCOPE_MASK, scope) |
>+		FIELD_PREP(NET_SHAPER_ID_MASK, id);
>+}
>+
>+/**
>+ * net_shaper_handle_scope - extract the scope from the given handle
>+ * @handle: the shaper handle
>+ *
>+ * Return: the corresponding scope
>+ */
>+static inline enum net_shaper_scope net_shaper_handle_scope(u32 handle)
>+{
>+	return FIELD_GET(NET_SHAPER_SCOPE_MASK, handle);
>+}
>+
>+/**
>+ * net_shaper_handle_id - extract the id number from the given handle
>+ * @handle: the shaper handle
>+ *
>+ * Return: the corresponding id number
>+ */
>+static inline int net_shaper_handle_id(u32 handle)

I undertand that you use the handle for uapi and xaarray index purposes
as consolidated u32. But, for the driver ops, would it be better to pass
scope and id trough the ops to drivers? Driver then don't need to call
these helpers and they can all stay private to shaper.c


>+{
>+	return FIELD_GET(NET_SHAPER_ID_MASK, handle);
>+}
>+
>+#endif
>+

[...]

Powered by blists - more mailing lists