[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <m25xslp8nh.fsf@gmail.com>
Date: Wed, 31 Jul 2024 22:13:22 +0100
From: Donald Hunter <donald.hunter@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.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>
Subject: Re: [PATCH v3 02/12] netlink: spec: add shaper YAML spec
Paolo Abeni <pabeni@...hat.com> writes:
> diff --git a/Documentation/netlink/specs/shaper.yaml b/Documentation/netlink/specs/shaper.yaml
> new file mode 100644
> index 000000000000..7327f5596fdb
> --- /dev/null
> +++ b/Documentation/netlink/specs/shaper.yaml
It's probably more user-friendly to use the same filename as the spec
name, so net-shaper.yaml
> @@ -0,0 +1,262 @@
> +# SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause)
> +
> +name: net-shaper
> +
> +doc: Network device HW Rate Limiting offload
> +
> +definitions:
> + -
> + type: enum
> + name: scope
> + doc: the different scopes where a shaper can be attached
Nit: upper case 'The' to be consistent with rest of docs.
> + render-max: true
> + entries:
> + - name: unspec
> + doc: The scope is not specified
What are the semantics of 'unspec' ? When can it be used?
> + -
> + name: port
> + doc: The root for the whole H/W
> + -
> + name: netdev
> + doc: The main shaper for the given network device.
What are the semantic differences between netdev and port?
> + -
> + name: queue
> + doc: The shaper is attached to the given device queue.
> + -
> + name: detached
> + doc: |
> + The shaper is not attached to any user-visible network
> + device component and allows nesting and grouping of
> + queues or others detached shapers.
I assume that shapers are always owned by the netdev regardless of
attach status?
> + -
> + type: enum
> + name: metric
> + doc: different metric each shaper can support
Nit: upper case here as well.
> + entries:
> + -
> + name: bps
> + doc: Shaper operates on a bits per second basis
> + -
> + name: pps
> + doc: Shaper operates on a packets per second basis
> +
> +attribute-sets:
> + -
> + name: net-shaper
> + attributes:
> + -
> + name: ifindex
> + type: u32
> + doc: Interface index owing the specified shaper[s]
Typo: this should be 'owning' ?
> + -
> + name: handle
> + type: nest
> + nested-attributes: handle
> + doc: Unique identifier for the given 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
> + -
> + name: bw-max
> + type: uint
> + doc: Shaping B/W for the given shaper or 0 when unlimited
> + -
> + name: burst
> + type: uint
> + doc: Maximum burst-size for bw-min and bw-max
> + -
> + name: priority
> + type: u32
> + doc: Scheduling priority for the given shaper
> + -
> + name: weight
> + type: u32
> + doc: |
> + Weighted round robin weight for given shaper.
> + The scheduling is applied to all the sibling
> + shapers with the same priority
> + -
> + name: scope
> + type: u32
> + enum: scope
> + doc: The given handle scope
> + -
> + name: id
> + type: u32
> + doc: |
> + The given handle id. The id semantic depends on the actual
> + scope, e.g. for 'queue' scope it's the queue id, for
> + 'detached' scope it's the shaper group identifier.
If scope and id are only ever used as attributes of a handle then they
would be better specified as a separate attribute-set, instead of
mixing them in here and using a subset.
You use 'quoted' references here and @refs elsewhere. It would be good
to be consistent. See note below about @ in htmldocs.
> + -
> + name: parent
> + type: nest
> + nested-attributes: handle
> + doc: |
> + Identifier for the parent of the affected shaper,
> + The parent handle value is implied by the shaper handle itself,
> + except for the output shaper in the 'group' operation.
Nit: quoted ref again here
> + -
> + name: inputs
> + type: nest
> + multi-attr: true
> + nested-attributes: ns-info
> + doc: |
> + Describes a set of inputs shapers for a @group operation
The @group renders exactly as-is in the generated htmldocs. There may be
a more .rst friendly markup you can use that will render better.
> + -
> + name: output
> + type: nest
> + nested-attributes: ns-output-info
> + doc: |
> + Describes the output shaper for a @group operation
> + Differently from @inputs and @shaper allow specifying
> + the shaper parent handle, too.
> +
Nit: remove the extra blank line here
> + -
> + name: shaper
> + type: nest
> + nested-attributes: ns-info
> + doc: |
> + Describes a single shaper for a @set operation
> + -
> + name: handle
> + subset-of: net-shaper
> + attributes:
> + -
> + name: scope
> + -
> + name: id
> + -
> + name: ns-info
> + subset-of: net-shaper
> + attributes:
> + -
> + name: handle
> + -
> + name: metric
> + -
> + name: bw-min
> + -
> + name: bw-max
> + -
> + name: burst
> + -
> + name: priority
> + -
> + name: weight
> + -
> + name: ns-output-info
> + subset-of: net-shaper
> + attributes:
> + -
> + name: parent
> + -
> + name: handle
> + -
> + name: metric
> + -
> + name: bw-min
> + -
> + name: bw-max
> + -
> + name: burst
> + -
> + name: priority
> + -
> + name: weight
> +
> +operations:
> + list:
> + -
> + name: get
> + doc: |
> + Get / Dump information about a/all the shaper for a given device
> + attribute-set: net-shaper
> +
> + do:
> + request:
> + attributes:
> + - ifindex
> + - handle
> + reply:
> + attributes: &ns-attrs
> + - parent
> + - handle
> + - metric
> + - bw-min
> + - bw-max
> + - burst
> + - priority
> + - weight
> +
> + dump:
> + request:
> + attributes:
> + - ifindex
> + reply:
> + attributes: *ns-attrs
> + -
> + name: set
> + doc: |
> + Create or configures the specified shaper.
> + On failures the extack is set accordingly.
> + Can't create @detached scope shaper, use
> + the @group operation instead.
> + attribute-set: net-shaper
> + flags: [ admin-perm ]
> +
> + do:
> + request:
> + attributes:
> + - ifindex
> + - shaper
> +
> + -
> + name: delete
> + doc: |
> + Clear (remove) the specified shaper. If after the removal
> + the parent shaper has no more children and the parent
> + shaper scope is @detached, even the parent is deleted,
> + recursively.
> + On failures the extack is set accordingly.
> + attribute-set: net-shaper
> + flags: [ admin-perm ]
> +
> + do:
> + request:
> + attributes:
> + - ifindex
> + - handle
> +
> + -
> + name: group
> + doc: |
> + Group the specified input shapers under the specified
> + output shaper, eventually creating the latter, if needed.
> + Input shapers scope must be either @queue or @detached.
It says above that you cannot create a detached shaper, so how do you
create one to use as an input shaper here? Is this group op more like a
multi-create op?
> + Output shaper scope must be either @detached or @netdev.
> + When using an output @detached scope shaper, if the
> + @handle @id is not specified, a new shaper of such scope
> + is created and, otherwise the specified output shaper
> + must be already existing.
> + The operation is atomic, on failures the extack is set
> + accordingly and no change is applied to the device
> + shaping configuration, otherwise the output shaper
> + handle is provided as reply.
> + attribute-set: net-shaper
> + flags: [ admin-perm ]
Does there need to be a reciprocal 'ungroup' operation? Without it,
create / group / delete seems like they will have ambiguous semantics.
> + do:
> + request:
> + attributes:
> + - ifindex
> + - inputs
> + - output
> + reply:
> + attributes:
> + - handle
Powered by blists - more mailing lists