On 8/27/24 03:55, Jakub Kicinski wrote:

On Fri, 23 Aug 2024 10:52:04 +0200 Paolo Abeni wrote:

quoted

quoted

quoted

+ * comprising the shaper scope and a scope-specific id.
+ */
+struct net_shaper_ops {
+	/**
+	 * @group: create the specified shapers scheduling group
+	 *
+	 * Nest the @leaves shapers identified by @leaves_handles under the
+	 * @root shaper identified by @root_handle. All the shapers belong
+	 * to the network device @dev. The @leaves and @leaves_handles shaper
+	 * arrays size is specified by @leaves_count.
+	 * Create either the @leaves and the @root shaper; or if they already
+	 * exists, links them together in the desired way.
+	 * @leaves scope must be NET_SHAPER_SCOPE_QUEUE.

Or SCOPE_NODE, no?

I had a few back-and-forth between the two options, enforcing only QUEUE
leaves or allowing even NODE.

I think the first option is general enough - can create arbitrary
topologies with the same amount of operations - and leads to slightly
simpler code, but no objections for allow both.

Ah, so we can only "grow the tree from the side of the leaves",
so to speak? We can't create a group in the middle of the hierarchy?

With the posted code, we can't. It can be implemented, but I think it 
will make the interface confusing.

I have no strong use for groups in between, maybe just mention in
a comment or cover letter.

I'll do, thanks.

quoted

quoted

quoted

+static int net_shaper_fill_handle(struct sk_buff *msg,
+				  const struct net_shaper_handle *handle,
+				  u32 type, const struct genl_info *info)
+{
+	struct nlattr *handle_attr;
+
+	if (handle->scope == NET_SHAPER_SCOPE_UNSPEC)
+		return 0;

In what context can we try to fill handle with scope unspec?

Uhmm... should happen only in buggy situation. What about adding adding
WARN_ON_ONCE() ?

That's better, at least it will express that it's not expected.

I added the WARN in my local build, and that reminded me the tree root 
(netdev) has UNSPEC parent. So I think we are better off with simply a 
comment there.

Thanks,

Paolo