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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZrxsvRzijiSv0Ji8@nanopsycho.orion>
Date: Wed, 14 Aug 2024 10:37:17 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, Jakub Kicinski <kuba@...nel.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 03/12] net-shapers: implement NL get operation

Tue, Aug 13, 2024 at 05:17:12PM CEST, pabeni@...hat.com wrote:
>On 8/1/24 15:42, Jiri Pirko wrote:
>> Tue, Jul 30, 2024 at 10:39:46PM CEST, pabeni@...hat.com wrote:
>> > +/**
>> > + * net_shaper_make_handle - creates an unique shaper identifier
>> > + * @scope: the shaper scope
>> > + * @id: the shaper id number
>> > + *
>> > + * Return: an unique identifier for the shaper
>> > + *
>> > + * Combines the specified arguments to create an unique identifier for
>> > + * the shaper. The @id argument semantic depends on the
>> > + * specified scope.
>> > + * For @NET_SHAPER_SCOPE_QUEUE_GROUP, @id is the queue group id
>> > + * For @NET_SHAPER_SCOPE_QUEUE, @id is the queue number.
>> > + * For @NET_SHAPER_SCOPE_VF, @id is virtual function number.
>> > + */
>> > +static inline u32 net_shaper_make_handle(enum net_shaper_scope scope,
>> > +					 int id)
>> > +{
>> > +	return FIELD_PREP(NET_SHAPER_SCOPE_MASK, scope) |
>> > +		FIELD_PREP(NET_SHAPER_ID_MASK, id);
>> 
>> Perhaps some scopes may find only part of u32 as limitting for id in
>> the future? I find it elegant to have it in single u32 though. u64 may
>> be nicer (I know, xarray) :)
>
>With this code the id limit is 2^26 for each scope. The most capable H/W I'm
>aware of supports at most 64K shapers, overall. Are you aware of any specific
>constraint we need to address?

Nope. Just thinking out loud.


>
>[...]
>> > 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.


>
>> Could we have various types of binding? Something like:
>> 
>> NET_SHAPER_A_BINDING nest
>>    NET_SHAPER_A_BINDING_IFINDEX u32
>> 
>> or:
>> NET_SHAPER_A_BINDING nest
>>    NET_SHAPER_A_BINDING_DEVLINK_PORT nest
>>      DEVLINK_ATTR_BUS_NAME string
>>      DEVLINK_ATTR_DEV_NAME string
>>      DEVLINK_ATTR_PORT_INDEX u32
>> 
>> ?
>
>Somewhat related, the current get()/dump() operations currently don't return
>the shaper ifindex. I guess we can include 'scope' and 'id' under
>NET_SHAPER_A_BINDING and replace the existing handle attribute with it.
>
>It should cover eventual future devlink extensions and provide all the
>relevant info for get/dump sake.

Sounds fine.


>
>> > +
>> > static int __init shaper_init(void)
>> 
>> 
>> 
>> fetch_dev
>> fill_handle
>> parse_handle
>> sc_lookup
>> __sc_container
>> dev_shaper_flush
>> shaper_init
>> 
>> 
>> Could you perhaps maintain net_shaper_ prefix for all of there?
>
>Most of the helpers are static and should never be visible outside this
>compilation unit, so I did not bother with a prefix, I'll add it in the next
>revision.

Thanks.


>
>Thanks,
>
>Paolo
>