Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 20 Aug 2024 17:12:23 +0200 Paolo Abeni wrote:
> diff --git a/Documentation/netlink/specs/net_shaper.yaml b/Documentation/netlink/specs/net_shaper.yaml
> new file mode 100644
> index 000000000000..a2b7900646ae
> --- /dev/null
> +++ b/Documentation/netlink/specs/net_shaper.yaml
> @@ -0,0 +1,289 @@
> +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> +
> +name: net-shaper
> +
> +doc: |
> +  Network device HW rate limiting configuration.
> +
> +  This API allows configuring HR shapers available on the network

What's HR?

> +  device at different levels (queues, network device) and allows
> +  arbitrary manipulation of the scheduling tree of the involved
> +  shapers.
> +
> +  Each @shaper is identified within the given device, by an @handle,
> +  comprising both a @scope and an @id, and can be created via two
> +  different modifiers: the @set operation, to create and update single

s/different modifiers/operations/

> +  shaper, and the @group operation, to create and update a scheduling
> +  group.
> +
> +  Existing shapers can be deleted via the @delete operation.

deleted -> deleted / reset

> +  The user can query the running configuration via the @get operation.

The distinction between "scoped" nodes which can be "set"
and "detached" "node"s which can only be created via "group" (AFAIU)
needs clearer explanation.

> +definitions:
> +  -
> +    type: enum
> +    name: scope
> +    doc: The different scopes where a shaper can be attached.

Are they attached or are they the nodes themselves?
Maybe just say that scope defines the ID interpretation and that's it.

> +    render-max: true
> +    entries:
> +      - name: unspec
> +        doc: The scope is not specified.
> +      -
> +        name: netdev
> +        doc: The main shaper for the given network device.
> +      -
> +        name: queue
> +        doc: The shaper is attached to the given device queue.
> +      -
> +        name: node
> +        doc: |
> +             The shaper allows grouping of queues or others
> +             node shapers, is not attached to any user-visible

Saying it's not attached is confusing. Makes it sound like it exists
outside of the scope of a struct net_device.

> +             network device component, and can be nested to
> +             either @netdev shapers or other @node shapers.

> +attribute-sets:
> +  -
> +    name: net-shaper
> +    attributes:
> +      -
> +        name: handle
> +        type: nest
> +        nested-attributes: handle
> +        doc: Unique identifier for the given shaper inside the owning device.
> +      -
> +        name: info
> +        type: nest
> +        nested-attributes: info
> +        doc: Fully describes the shaper.
> +      -
> +        name: metric
> +        type: u32
> +        enum: metric
> +        doc: Metric used by the given shaper for bw-min, bw-max and burst.
> +      -
> +        name: bw-min
> +        type: uint
> +        doc: Minimum guaranteed B/W for the given shaper.

s/Minimum g/G/
Please spell out "bandwidth" in user-facing docs.

> +      -
> +        name: bw-max
> +        type: uint
> +        doc: Shaping B/W for the given shaper or 0 when unlimited.

s/Shaping/Maximum/

> +      -
> +        name: burst
> +        type: uint
> +        doc: Maximum burst-size for bw-min and bw-max.

How about:

s/bw-min and bw-max/shaping. Should not be interpreted as a quantum./

?

> +      -
> +        name: priority
> +        type: u32
> +        doc: Scheduling priority for the given shaper.

Please clarify that that priority is only valid on children of 
a scheduling node, and the priority values are only compared
between siblings.

> +      -
> +        name: weight
> +        type: u32
> +        doc: |
> +          Weighted round robin weight for given shaper.

Relative weight of the input into a round robin node.

?

> +          The scheduling is applied to all the sibling
> +          shapers with the same priority.
> +      -
> +        name: scope
> +        type: u32
> +        enum: scope
> +        doc: The given shaper scope.

:)

> +      -
> +        name: id
> +        type: u32
> +        doc: |
> +          The given shaper id. 

"Numeric identifier of a shaper."

Do we ever use ID and scope directly in a nest with other attrs?
Or are they always wrapped in handle/parent ?
If they are always wrapped they can be a standalone attr set / space.

> The id semantic depends on the actual
> +          scope, e.g. for @queue scope it's the queue id, for
> +          @node scope it's the node identifier.
> +      -
> +        name: ifindex
> +        type: u32
> +        doc: Interface index owning the specified shaper.
> +      -
> +        name: parent
> +        type: nest
> +        nested-attributes: handle
> +        doc: |
> +          Identifier for the parent of the affected shaper,
> +          The parent is usually implied by the shaper handle itself,
> +          with the only exception of the root shaper in the @group operation.

Maybe just say that specifying the parent is only needed for group
operations? I think that's what you mean.

> +      -
> +        name: leaves
> +        type: nest
> +        multi-attr: true
> +        nested-attributes: info
> +        doc: |
> +           Describes a set of leaves shapers for a @group operation.
> +      -
> +        name: root
> +        type: nest
> +        nested-attributes: root-info
> +        doc: |
> +           Describes the root shaper for a @group operation

missing full stop

> +           Differently from @leaves and @shaper allow specifying
> +           the shaper parent handle, too.

Maybe this attr is better called "node", after all.

> +      -
> +        name: shaper
> +        type: nest
> +        nested-attributes: info
> +        doc: |
> +           Describes a single shaper for a @set operation.

Hm. How is this different than "info"?

$ git grep SHAPER_A_INFO
include/uapi/linux/net_shaper.h:        NET_SHAPER_A_INFO,
$

is "info" supposed to be used?

> +operations:
> +  list:
> +    -
> +      name: get
> +      doc: |
> +        Get / Dump information about a/all the shaper for a given device.

There's no need to "/ dump" and "/all".

> +      attribute-set: net-shaper

> +    -
> +      name: set
> +      doc: |
> +        Create or updates the specified shaper.

create or update

> +        On failure the extack is set accordingly.

it better be - no need to explain netlink basics

> +        Can't create @node scope shaper, use
> +        the @group operation instead.

"The set operation can't be used to create a @node scope shaper..."

> +      attribute-set: net-shaper
> +      flags: [ admin-perm ]
> +
> +      do:
> +        pre: net-shaper-nl-pre-doit
> +        post: net-shaper-nl-post-doit
> +        request:
> +          attributes:
> +            - ifindex
> +            - shaper
> +
> +    -
> +      name: delete
> +      doc: |
> +        Clear (remove) the specified shaper. When deleting
> +        a @node shaper, relink all the node's leaves to the

relink -> reattach ?

> +        deleted node parent.

delete node's parent

> +        If, after the removal, the parent shaper has no more
> +        leaves and the parent shaper scope is @node, even
> +        the parent node is deleted, recursively.
> +        On failure the extack is set accordingly.
> +      attribute-set: net-shaper
> +      flags: [ admin-perm ]
> +
> +      do:
> +        pre: net-shaper-nl-pre-doit
> +        post: net-shaper-nl-post-doit
> +        request:
> +          attributes: *ns-binding
> +
> +    -
> +      name: group
> +      doc: |
> +        Creates or updates a scheduling group, adding the specified

Please use imperative mood, like in a commit message.

adding -> attach(ing)

> +++ b/net/shaper/Makefile
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for the Generic HANDSHAKE service
> +#
> +# Copyright (c) 2024, Red Hat, Inc.

Ironic that you added the copyright given the copy/paste 
fail in the contents... ;)