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

[Jiri Pirko <jiri@resnulli.us>]

Mon, Aug 19, 2024 at 11:33:28AM CEST, pabeni@redhat.com wrote:
>
>
>On 8/16/24 11:16, Jiri Pirko wrote:
>> Fri, Aug 16, 2024 at 10:52:58AM CEST, pabeni@redhat.com wrote:
>> > On 8/14/24 10:37, Jiri Pirko wrote:
>> > > Tue, Aug 13, 2024 at 05:17:12PM CEST, pabeni@redhat.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

Yeah, not even network. Just "a device".


>port going through some net_device?

I'm not sure why you are asking that. I didn't suggest anything like
that. On contrary, as your API is netdev centric, I suggested to
disconnect from netdev to the shapers could be used not only with them.
This is what I understood was a plan from very beginning. I may be wrong
though....


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

Don't assume this. Not always true.


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

But you still want to use some netdevice as a handle IIUC, is that
right?


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