Message-ID: <c6d8052c-c5a0-48e2-8984-0063afc1e482@redhat.com>
Date: Mon, 2 Sep 2024 12:10:50 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jakub Kicinski <kuba@...nel.org>, Jiri Pirko <jiri@...nulli.us>
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>, Donald Hunter
 <donald.hunter@...il.com>, anthony.l.nguyen@...el.com,
 przemyslaw.kitszel@...el.com, intel-wired-lan@...ts.osuosl.org,
 edumazet@...gle.com
Subject: Re: [PATCH v5 net-next 02/12] net-shapers: implement NL get operation



On 8/30/24 21:14, Jakub Kicinski wrote:
> On Fri, 30 Aug 2024 17:43:08 +0200 Paolo Abeni wrote:
>> Please allow me to put a few high level questions together, to both
>> underline them as most critical, and keep the thread focused.
>>
>> On 8/30/24 03:20, Jakub Kicinski wrote:
>>   > This 'binding' has the same meaning as 'binding' in TCP ZC? :(
>>
>> I hope we can agree that good naming is difficult. I thought we agreed
>> on such naming in the past week’s discussion. The term 'binding' is
>> already used in the networking stack in many places to identify
>> different things (i.e. device tree, socket, netfilter.. ). The name
>> prefix avoids any ambiguity and I think this a good name, but if you
>> have any better suggestions, this change should be trivial.
> 
> Ack. Maybe we can cut down the number of ambiguous nouns elsewhere:
> 
> maybe call net_shaper_info -> net_shaper ?
> 
> maybe net_shaper_data -> net_shaper_hierarchy ?

Is everybody fine with the above?

>> [about separate handle from shaper_info arguments]
>>   > Wouldn't it be convenient to store the handle in the "info"
>>   > object? AFAIU the handle is forever for an info, so no risk of it
>>   > being out of sync…
>>
>> Was that way a couple of iterations ago. Jiri explicitly asked for the
>> separation, I asked for confirmation and nobody objected.
> 
> Could you link to that? I must have not read it.

https://lore.kernel.org/netdev/ZqzIoZaGVb3jIW43@nanopsycho.orion/

search for "I wonder if the handle should be part of this structure"

I must admit by wannabe reply on such point never left my outbox.

> You can keep it wrapped in a struct *_handle, that's fine.
> But it can live inside the shaper object.

That is basically the opposite of what Jiri asked. @Jiri would you be ok 
reverting to such layout?

>> Which if the 2 options is acceptable from both of you?
>>
>> [about queue limit and channel reconf]
>>   > we probably want to trim the queue shapers on channel reconfig,
>>   > then, too? :(
>>
>> what about exposing to the drivers an helper alike:
>>
>> 	net_shaper_notify_delete(binding, handle);
>>
>> that tells the core the shaper at the given handle just went away in the
>> H/W? The driver will call it in the queue deletion helper, and such
>> helper could be later on used more generically, i.e. for vf/devlink port
>> deletion.
> 
> We can either prevent disabling queues which have shapers attached,
> or auto-removing the shapers. 

I think/fear that prevent disabling queues would lead to 
weird/unexpected results and more difficult administration, I prefer the 
callback option.

> No preference on that. But put the
> callback in the core, please, netif_set_real_num_rx_queues() ?
> Why not?

It makes sense. I'll add a net_shaper_set_real_num_rx_queues() callback 
there.

/P

Powered by blists - more mailing lists