[PATCH net-next 0/3] net_shaper: clarify kernel API docs

[PATCH net-next 0/3] net_shaper: clarify kernel API docs

[Jakub Kicinski]

A handful of improvements to struct net_shaper_ops kdoc.
Today driver authors have to dig thru the shaper.c code
to understand the behavior. I'm covering things I wish
were explained already during recent reviews (within Meta).
There may be more things that need to be explained, incrementally.

Jakub Kicinski (3):
  net_shaper: remove incorrect comment about group leaves
  net_shaper: clarify the kernel API / comments
  net_shaper: add some notes on re-parenting

 include/net/net_shaper.h | 39 ++++++++++++++++++++++++++++++++-------
 1 file changed, 32 insertions(+), 7 deletions(-)

-- 
2.55.0

[PATCH net-next 1/3] net_shaper: remove incorrect comment about group leaves

[Jakub Kicinski]

It is true that the user-facing group() operation can only
be invoked with queues as leaves (see net_shaper_parse_leaf()),
but the driver facing op is also called when we delete a node.
When we delete a node we conceptually call

    group(parent, node.list_of_leaves)

to add node's leaves to the parent. Node deletion "mid-hierarchy"
is supported so some of the leaves may themselves be nodes.
Therefore the driver facing group() may be called with nodes.

Remove the incorrect comment, and add a comment about differences
between the Netlink API and driver facing API.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/net_shaper.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/net/net_shaper.h b/include/net/net_shaper.h
index 3939b816b001..0fcca29207ac 100644
--- a/include/net/net_shaper.h
+++ b/include/net/net_shaper.h
@@ -72,6 +72,18 @@ struct net_shaper {
  *
  * Each shaper is uniquely identified within the device with a 'handle'
  * comprising the shaper scope and a scope-specific id.
+ *
+ * Driver ops vs uAPI
+ * ------------------
+ * Members of the driver ops mirror the Netlink uAPI but driver calls do not
+ * map 1:1 to user calls. Drivers need to be careful when assuming that calls
+ * disallowed at the uAPI level will never be made at the driver level.
+ * The shaper core performs automatic reparenting and cleanup, generating
+ * additional calls. Notably:
+ *  - @group calls in the driver facing API may have nodes as leaves (user is
+ *    only allowed to construct groups with queues as leaves)
+ *  - @group calls may update leaf's parent if the parent is about
+ *    to be removed (re-parenting nodes explicitly is not supported in the uAPI)
  */
 struct net_shaper_ops {
 	/**
@@ -82,7 +94,6 @@ struct net_shaper_ops {
 	 * The @leaves arrays size is specified by @leaves_count.
 	 * Create either the @leaves and the @node shaper; or if they already
 	 * exists, links them together in the desired way.
-	 * @leaves scope must be NET_SHAPER_SCOPE_QUEUE.
 	 */
 	int (*group)(struct net_shaper_binding *binding, int leaves_count,
 		     const struct net_shaper *leaves,
-- 
2.55.0

[PATCH net-next 2/3] net_shaper: clarify the kernel API / comments

[Jakub Kicinski]

The shaper API takes some getting used to. Try to improve
the doc on struct net_shaper_ops to help driver developers.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/net_shaper.h | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/include/net/net_shaper.h b/include/net/net_shaper.h
index 0fcca29207ac..c14eb87efe5e 100644
--- a/include/net/net_shaper.h
+++ b/include/net/net_shaper.h
@@ -68,7 +68,7 @@ struct net_shaper {
  * The operations are serialized via a per device lock.
  *
  * Device not supporting any kind of nesting should not provide the
- * group operation.
+ * @group operation.
  *
  * Each shaper is uniquely identified within the device with a 'handle'
  * comprising the shaper scope and a scope-specific id.
@@ -84,16 +84,30 @@ struct net_shaper {
  *    only allowed to construct groups with queues as leaves)
  *  - @group calls may update leaf's parent if the parent is about
  *    to be removed (re-parenting nodes explicitly is not supported in the uAPI)
+ *
+ * Implicit creation
+ * -----------------
+ * Shapers are created implicitly, meaning that @set and @group operations
+ * are called both for existing and new shapers. The driver has to infer
+ * whether the operation is an update or a creation by tracking the handles.
+ * Removal of shapers is explicit and done with a @delete call.
+ *
+ * The @set operation implicitly creates NET_SHAPER_SCOPE_NETDEV and
+ * NET_SHAPER_SCOPE_QUEUE shapers.
+ * The @group operation implicitly creates NET_SHAPER_SCOPE_NETDEV and
+ * NET_SHAPER_SCOPE_NODE shapers (the group shaper itself), as well as
+ * NET_SHAPER_SCOPE_QUEUE shapers (leaves).
  */
 struct net_shaper_ops {
 	/**
-	 * @group: create the specified shapers scheduling group
+	 * @group: create a scheduling group or add leaves
 	 *
-	 * Nest the @leaves shapers identified under the * @node shaper.
+	 * Nest the @leaves shapers identified under the @node shaper.
 	 * All the shapers belong to the device specified by @binding.
-	 * The @leaves arrays size is specified by @leaves_count.
-	 * Create either the @leaves and the @node shaper; or if they already
-	 * exists, links them together in the desired way.
+	 * The @leaves array's size is specified by @leaves_count.
+	 *
+	 * @node and @leaves may or may not already exist
+	 * (see the "Implicit creation" note).
 	 */
 	int (*group)(struct net_shaper_binding *binding, int leaves_count,
 		     const struct net_shaper *leaves,
-- 
2.55.0

[PATCH net-next 3/3] net_shaper: add some notes on re-parenting

[Jakub Kicinski]

Clarify the re-parenting expectations. Specifically that @delete
on a queue removes it from the hierarchy which is a bit unusual
in the overall API structure.

IIRC the implicit delete behavior was introduced because otherwise
it would not be possible to remove a queue from the hierarchy
without changing at least one handle of the shapers. Normally
"removal" is done by "adding" to the new parent, but "outside
the hierarchy" does not have a parent we can point at.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/net_shaper.h | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/include/net/net_shaper.h b/include/net/net_shaper.h
index c14eb87efe5e..05cb625b0fe5 100644
--- a/include/net/net_shaper.h
+++ b/include/net/net_shaper.h
@@ -107,7 +107,12 @@ struct net_shaper_ops {
 	 * The @leaves array's size is specified by @leaves_count.
 	 *
 	 * @node and @leaves may or may not already exist
-	 * (see the "Implicit creation" note).
+	 * (see the "Implicit creation" note). If @node already exists,
+	 * the @leaves should be *added* to its children. In this case,
+	 * the @leaves array only holds new/modified leaves, not the full list.
+	 *
+	 * Re-parenting @leaves is implemented by a @group call on a new parent.
+	 * There's no explicit call to remove the children from the old parent.
 	 */
 	int (*group)(struct net_shaper_binding *binding, int leaves_count,
 		     const struct net_shaper *leaves,
@@ -128,6 +133,13 @@ struct net_shaper_ops {
 	 *
 	 * Removes the shaper configuration as identified by the given @handle
 	 * on the device specified by @binding, restoring the default behavior.
+	 *
+	 * Note that a @delete call on a NET_SHAPER_SCOPE_QUEUE shaper also
+	 * implicitly removes the associated queue from the scheduling
+	 * hierarchy. The driver must take care of that step.
+	 * @delete calls on NET_SHAPER_SCOPE_NODE should not require any
+	 * implicit re-parenting in the driver as core will re-parent the leaves
+	 * first, before deleting the SCOPE_NODE shaper.
 	 */
 	int (*delete)(struct net_shaper_binding *binding,
 		      const struct net_shaper_handle *handle,
-- 
2.55.0