Message-ID: <f2e82924-a105-4d82-a2ad-46259be587df@redhat.com>
Date: Mon, 12 Aug 2024 16:58:33 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>,
 Donald Hunter <donald.hunter@...il.com>
Cc: netdev@...r.kernel.org, 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

On 8/6/24 09:06, Jiri Pirko wrote:
> Mon, Aug 05, 2024 at 05:11:09PM CEST, pabeni@...hat.com wrote:
>> Hi all,
>>
>> (same remark of my previous email). My replies this week will be delayed,
>> please allow for some extra latency.
>>
>> On 8/2/24 12:49, Jiri Pirko wrote:
>>> Thu, Aug 01, 2024 at 05:12:01PM CEST, pabeni@...hat.com wrote:
>>>> On 8/1/24 15:10, Jiri Pirko wrote:
>>>>> Tue, Jul 30, 2024 at 10:39:45PM CEST, pabeni@...hat.com wrote:
>>>>>> +    type: enum
>>>>>> +    name: scope
>>>>>> +    doc: the different scopes where a shaper can be attached
>>>>>> +    render-max: true
>>>>>> +    entries:
>>>>>> +      - name: unspec
>>>>>> +        doc: The scope is not specified
>>>>>> +      -
>>>>>> +        name: port
>>>>>> +        doc: The root for the whole H/W
>>>>>
>>>>> What is this "port"?
>>>>
>>>> ~ a wire plug.
>>>
>>> What's "wire plug"? What of existing kernel objects this relates to? Is
>>> it a devlink port?
>>
>>
>> I'm sorry, my hasty translation of my native language was really inaccurate.
>> Let me re-phrase from scratch: that is actually the root of the whole
>> scheduling tree (yes, it's a tree) for a given network device.
>>
>> One source of confusion is that in a previous iteration we intended to allow
>> configuring even objects 'above' the network device level, but such feature
>> has been dropped.
>>
>> We could probably drop this scope entirely.
> 
> Drop for now, correct? I agree that your patchset now only works on top
> of netdev. But all infra should be ready to work on top of something
> else, devlink seems like good candidate. I mean, for devlink port
> function rate, we will definitelly need something like that.
> 
> 
>>
>>>>>> +      -
>>>>>> +        name: netdev
>>>>>> +        doc: The main shaper for the given network device.
>>>>>> +      -
>>>>>> +        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.
>>>>>
>>>>> What is the purpose of the "detached" thing?
>>>>
>>>> I fear I can't escape reusing most of the wording above. 'detached' nodes
>>>> goal is to create groups of other shapers. i.e. queue groups,
>>>> allowing multiple levels nesting, i.e. to implement this kind of hierarchy:
>>>>
>>>> q1 ----- \
>>>> q2 - \SP / RR ------
>>>> q3 - /    	    \
>>>> 	q4 - \ SP -> (netdev)
>>>> 	q5 - /	    /
>>>>                     /
>>>> 	q6 - \ RR
>>>> 	q7 - /
>>>>
>>>> where q1..q7 are queue-level shapers and all the SP/RR are 'detached' one.
>>>> The conf. does not necessary make any functional sense, just to describe the
>>>> things.
>>>
>>> Can you "attach" the "detached" ones? They are "detached" from what?
>>
>> I see such name is very confusing. An alternative one could be 'group', but
>> IIRC it was explicitly discarded while discussing a previous iteration.
>>
>> The 'detached' name comes from the fact the such shapers are not a direct
>> representation of some well-known kernel object (queues, devices),
> 
> Understand now. Maybe "node" would make more sense? Leaves are queues
> and root is the device? Aligns with the tree terminology...
> 
>>
>>>>>> +    -
>>>>>> +      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.
>>>>>> +        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.
>>>>>
>>>>> I'm lost. Could this designt be described in details in the doc I asked
>>>>> in the cover letter? :/ Please.
>>>>
>>>> I'm unsure if the context information here and in the previous replies helped
>>>> somehow.
>>>>
>>>> The group operation creates and configure a scheduling group, i.e. this
>>>>
>>>> q1 ----- \
>>>> q2 - \SP / RR ------
>>>> q3 - /    	    \
>>>> 	q4 - \ SP -> (netdev)
>>>> 	q5 - /	    /
>>>>                     /
>>>> 	q6 - \ RR
>>>> 	q7 - /
>>>>
>>>> can be create with:
>>>>
>>>> group(inputs:[q6, q7], output:[detached,parent:netdev])
>>>> group(inputs:[q4, q5], output:[detached,parent:netdev])
>>>> group(inputs:[q1], output:[detached,parent:netdev])
>>>> group(inputs:[q2,q3], output:[detached,parent:<the detached shaper create
>>>> above>])
>>>
>>> So by "inputs" and "output" you are basically building a tree. In
>>> devlink rate, we have leaf and node, which is in sync with standard tree
>>> terminology.
>>>
>>> If what you are building is tree, why don't you use the same
>>> terminology? If you are building tree, you just need to have the link to
>>> upper noded (output in your terminology). Why you have "inputs"? Isn't
>>> that redundant?
>>
>> The idea behind the inputs/outputs naming is to represent the data flow
>> towards the wire.
>> I'm fine with the parent/children naming, but IIRC Jakub was not happy with
>> it. Is there any intermediate ground that could satisfy both of you?
> 
> It's a tree, so perhaps just stick with tree terminology, everyone is
> used to that. Makes sense? One way or another, this needs to be
> properly described in docs, all terminology. That would make things more
> clear, I believe.

@Jakub, would you be ok with:

'inputs' ->  'leaves'
'output' -> 'node'
?

Also while at it, I think renaming the 'group()' operation as 
'node_set()' could be clearer (or at least less unclear), WDYT?

Note: I think it's would be more user-friendly to keep a single 
delete/get/dump operation for 'nodes' and leaves.

Thanks,

Paolo

Powered by blists - more mailing lists