netdev - Re: [PATCH v3 03/12] net-shapers: implement NL get operation

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4cb6fe12-a561-47a4-9046-bb54ad1f4d4e@redhat.com>
Date: Mon, 19 Aug 2024 11:33:28 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: 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>, Jakub Kicinski <kuba@...nel.org>
Subject: Re: [PATCH v3 03/12] net-shapers: implement NL get operation

On 8/16/24 11:16, Jiri Pirko wrote:
> Fri, Aug 16, 2024 at 10:52:58AM CEST, pabeni@...hat.com wrote:
>> On 8/14/24 10:37, Jiri Pirko wrote:
>>> Tue, Aug 13, 2024 at 05:17:12PM CEST, pabeni@...hat.com wrote:
>>>> On 8/1/24 15:42, Jiri Pirko wrote:
>>>> [...]
>>>>>> int net_shaper_nl_get_doit(struct sk_buff *skb, struct genl_info *info)
>>>>>> {
>>>>>> -	return -EOPNOTSUPP;
>>>>>> +	struct net_shaper_info *shaper;
>>>>>> +	struct net_device *dev;
>>>>>> +	struct sk_buff *msg;
>>>>>> +	u32 handle;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	ret = fetch_dev(info, &dev);
>>>>>
>>>>> This is quite net_device centric. Devlink rate shaper should be
>>>>> eventually visible throught this api as well, won't they? How do you
>>>>> imagine that?
>>>>
>>>> I'm unsure we are on the same page. Do you foresee this to replace and
>>>> obsoleted the existing devlink rate API? It was not our so far.
>>>
>>> Driver-api-wise, yes. I believe that was the goal, to have drivers to
>>> implement one rate api.
>>
>> I initially underlooked at this point, I'm sorry.
>>
>> Re-reading this I think we are not on the same page.
>>
>> The net_shaper_ops are per network device operations: they are aimed (also)
>> at consolidating network device shaping related callbacks, but they can't
>> operate on non-network device objects (devlink port).
> 
> Why not?

Isn't the whole point of devlink to configure objects that are directly 
related to any network device? Would be somewhat awkward accessing 
devlink port going through some net_device?

Side note: I experimented adding the 'binging' abstraction to this API 
and gives a quite significant uglification to the user syntax (due to 
the additional nesting required) and the code.

Still, if there is a very strong need for controlling devlink rate via 
this API _and_ we can assume that each net_device "relates" 
(/references/is connected to) at most a single devlink object (out of 
sheer ignorance on my side I'm unsure about this point, but skimming 
over the existing implementations it looks so), the current API 
definition would be IMHO sufficient and clean enough to reach for both 
devlink port rate objects and devlink node rate objects.

We could define additional scopes for each of such objects and use the 
id to discriminate the specific port or node within the relevant devlink.

I think such scopes definition should come with related implementation, 
e.g. not with this series.

Thanks,

Paolo