Message-ID: <ad5be943-2aa6-4f60-be90-929f889e6057@redhat.com>
Date: Fri, 23 Aug 2024 10:35:05 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, Jiri Pirko <jiri@...nulli.us>,
 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>, Donald Hunter <donald.hunter@...il.com>
Subject: Re: [PATCH v4 net-next 02/12] netlink: spec: add shaper YAML spec

On 8/23/24 03:48, Jakub Kicinski wrote:
> 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?

Type: HW

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

How about re-phrasing the previous paragraph as:

   Each @shaper is identified within the given device, by an @handle,
   comprising both a @scope and an @id.

   Depending on the @scope value, the shapers are attached to specific
   HW objects (queues, devices) or, for @node scope, represent a
   scheduling group that can be placed in an arbitrary location of
   the scheduling tree.

   Shapers can be created with two different operations: the @set
   operation, to create and update single "attached" shaper, and
   the @group operation, to create and update a scheduling
   group. Only the @group operation can create @node scope shapers.


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

Will do.

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

What about:

   Can be placed in any arbitrary location of
   the scheduling tree, except leaves and root.

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

I would avoid mentioning 'input' unless we rolls back to the previous 
naming scheme.

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

Will do in the next revision.

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

Fine by me, but would that cause some confusion with the alias scope 
value?

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

left over from the previous revision, I will drop it.

>> +++ 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... ;)

Strictly speaking, since it was not intentional, it is more careless or 
stupid on my side.

Thanks,

Paolo

Powered by blists - more mailing lists