Message-ID: <e971cd64-9cbf-46d2-89fc-008548d1d211@redhat.com>
Date: Mon, 5 Aug 2024 16:35:29 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Donald Hunter <donald.hunter@...il.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

Hi all,

My replies this week will be delayed, please allow for some extra latency.

On 8/2/24 13:15, Donald Hunter wrote:
> Paolo Abeni <pabeni@...hat.com> writes:
> 
>> On 7/31/24 23:13, Donald Hunter wrote:
>>> Paolo Abeni <pabeni@...hat.com> writes:
>>>
>>>> +        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.
>>
>> Uhm... AFAICS the problem is the target (e.g. 'group') is outside the htmldoc section itself, I
>> can't find any existing markup to serve this purpose well. What about sticking to quotes ''
>> everywhere?
>>
>> FTR, I used @ following the kdoc style.
> 
> Yeah, I was just thinking of using .rst markup like ``code`` or
> `italics`, but the meaning of @ is pretty obvious when reading the spec.
> If you stick with @ then we could always teach ynl-to-rst to render it
> as ``code``.

I'm fine with using @ everywhere.

>> [...]
>>>> +    -
>>>> +      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?
>>
>> The group operation has the main goal of configuring a single WRR or SP scheduling group
>> atomically. It can creates the needed shapers as needed, see below.
>>
>> The need for such operation sparks from some H/W constraints:
>>
>> https://lore.kernel.org/netdev/9dd818dc-1fef-4633-b388-6ce7272f9cb4@lunn.ch/
>>
>>>> +        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.
>>
>> I guess we need a better description. Can you please tell where/how the current one is
>> ambiguous?
> 
> My expectation for 'group' would be to group existing things, with a
> reciprocal 'ungroup' operation. I think you intend 'group' to both be
> able to group existing shapers/groups and create a group of shapers.
> 
> Am I right in saying that delete lets you delete something from a group
> (with side-effect of deleting group if it becomes empty), or delete a
> whole group?

In the current incarnation, delete() on the whole group is explicitly 
forbidden. Jakub suggested we should allow such behavior for the 
delegation use-case.

> It feels a lot like each of 'set', 'group' and 'delete' are doing
> multiple things and the interaction between them all becomes challenging
> to describe, or to handle all the corner case > I think part of the
> problem is the mixed terminology of input, output for groups, handle,
> parent for shapers and using detached to differentiate from 'implicitly
> attached to a resource'.
> 
> Perhaps the API would be better if you had:
> 
> - shaper-new
> - shaper-delete
> - shaper-get/dump
> - shaper-set
> - group-new
> - group-delete
> - group-get/dump
> - group-set
> 
> If you went with Jakub's suggestion to give every shaper n x inputs and
> an output, then you could recombine groups and shapers and just have 4
> ops. And you could rename 'detached' to 'shaper' so that an attachment
> is one of port, netdev, queue or shaper.

I'm unsure I read the above correctly, and I'm unsure it's in the same 
direction of Jakub's suggestion. AFACS the above is basically the same 
interface we proposed in the past iteration and was explicitly nacked 
from Jakub,

Additionally, one of the constraint to be addressed here is allowing to 
setup/configures all the nodes in a 'group' with a single operation, to 
deal with H/W limitations. How would the above address such constraint?

Thanks,

Paolo

Powered by blists - more mailing lists