Hi,

It looks like most of the open points here are related lack of clarity
or misunderstanding. I propose to discuss them in the upcoming
reviewer's meeting.

Some replies below, just in case I magically achieved superior written
natural language skills meanwhile ;) 

On Fri, 2024-05-31 at 09:00 -0700, Jakub Kicinski wrote:

On Fri, 31 May 2024 11:22:46 +0200 Paolo Abeni wrote:

quoted

On Tue, 2024-05-28 at 10:18 -0700, Jakub Kicinski wrote:

quoted

quoted

+	u32 priority;	/* scheduling strict priority */
+	u32 weight;	/* scheduling WRR weight*/  

I wonder if we should somehow more clearly specify what a node can do.
Like Andrew pointed out, if we have a WRR node, presumably the weights
go on the children, since there's only one weigh param. But then the
WRR node itself is either empty (no params) or has rate params... which
is odd.

Maybe shaping nodes and RR nodes should be separate node classes,
somehow?  

Possibly clarifying the meaning of 'weight' field would help?  
It means: this node is scheduled WRR according to the specified weight
among the sibling shapers with the same priority.

I think it's quite simpler than introducing different node classes with
separate handling. My understanding is that the latter would help only
to workaround some H/W limitation and will make the implementation more
difficult for more capable H/W.

What kind of problems do you foresee with the above definition?

The problem Andrew mentioned, basically. There may not be a path to
transition one fully offloaded hierarchy to another (e.g. switching
strict prio to WRR). TBH I haven't really grasped your proposal in:
https://lore.kernel.org/all/db51b7ccff835dd5a96293fb84d527be081de062.camel@redhat.com/ (local)

The problem Andrew reported is that some H/W, in some configuration,
can't change per-queue shapers individually.

The solution proposed in the above link is to let the API change
multiple shapers with a single op "atomically".

quoted

quoted

quoted

+ * NET_SHAPER_SCOPE_PORT and NET_SHAPER_SCOPE_VF are only available on
+ * PF devices, usually inside the host/hypervisor.
+ * NET_SHAPER_SCOPE_NETDEV, NET_SHAPER_SCOPE_QUEUE_GROUP and
+ * NET_SHAPER_SCOPE_QUEUE are available on both PFs and VFs devices.
+ */
+enum net_shaper_scope {
+	NET_SHAPER_SCOPE_PORT,
+	NET_SHAPER_SCOPE_NETDEV,
+	NET_SHAPER_SCOPE_VF,  

I realized now that we do indeed need this VF node (if we want to
internally express the legacy SRIOV NDOs in this API), as much 
as I hate it. Could you annotate somehow my nack on ever exposing
the ability to hook on the VF to user space?  

This work sparked from the need to allow configuring a shaper on
specific queues of some VF from the host. I hope this is not what you
are nacking here? Could you please elaborate a bit what concern you
with 'hook on the VF to user space'? Would the ability of attaching a
shaper to the VF from the host hit your nack?

Queue configuration, for the VF, from the hypervisor?
I thought it was from the VF.
In any case, hypervisor has the representors. 
Use the representor's NETDEV scope?

It looks like even the NETDEV scope + representor alternative should
fit the intended use-case.

quoted

quoted

quoted

+	NET_SHAPER_SCOPE_QUEUE_GROUP,  

We may need a definition for a queue group. Did I suggest this?  

I think this was mentioned separately by you, John Fastabend and Intel.

Oh ugh, I can't type. I think I meant to say "Why do we need..."

quoted

quoted

Isn't queue group just a bunch of queues feeding a trivial RR node?
Why does it need to be a "scope"?  

The goal is allowing arbitrary manipulation at the queue group level.
e.g. you can have different queue groups with different priority, or
weigh or shaping, and below them arbitrary shaping at the queue level.

Note that a similar concept could be introduced for device (or VFs)
groups.

Why would you like to constraint the features avail at the queue
groups?

Wait! You don't have a way to create pure RR nodes other than queue
group now? Perhaps that's what I'm missing...

No, it's not needed. To have RR on some queues, just set the same
weight and priority on them.

Queue groups could be used to do something more complex, e.g. shaping
on the specified set of RR queues.

quoted

quoted

quoted

+	NET_SHAPER_SCOPE_QUEUE,
+};
+
+/**
+ * struct net_shaper_ops - Operations on device H/W shapers
+ * @add: Creates a new shaper in the specified scope.  

"in a scope"? Isn't the scope just defining the ingress and egress
points of the scheduling hierarchy?  

This is purely lexical matter, right? The scope, and more specifically
the full 'handle' comprising more scoped-related information, specifies
'where' the shaper is located. Do you have a suggested alternative
wording?

... and also what confused me here.

How are you going to do 2 layers of grouping with arbitrary shaping?
We need arbitrary inner nodes. Unless I'm missing a trick.

I guess this part really needs some talk. I also don't understand your
doubt above.

quoted

I hope we can discuss this tomorrow (and the other points, if/as
needed).

Thanks!

Paolo