Message-ID: <ZsiQSfTNr5G0MA58@nanopsycho.orion>
Date: Fri, 23 Aug 2024 15:36:09 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Jakub Kicinski <kuba@...nel.org>, 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 03/12] net-shapers: implement NL get operation

Fri, Aug 23, 2024 at 02:58:27PM CEST, pabeni@...hat.com wrote:
>On 8/23/24 13:50, Jiri Pirko wrote:
>> Fri, Aug 23, 2024 at 12:56:08AM CEST, kuba@...nel.org wrote:
>> > On Thu, 22 Aug 2024 22:30:35 +0200 Paolo Abeni wrote:
>> > > > > I'm not saying this is deal breaker for me. I just think that if the api
>> > > > > is designed to be independent of the object shaper is bound to
>> > > > > (netdev/devlink_port/etc), it would be much much easier to extend in the
>> > > > > future. If you do everything netdev-centric from start, I'm sure no
>> > > > > shaper consolidation will ever happen. And that I thought was one of the
>> > > > > goals.
>> > > > > 
>> > > > > Perhaps Jakub has opinion.
>> > > > 
>> > > > I think you and I are on the same page :) Other than the "reference
>> > > > object" (netdev / devlink port) the driver facing API should be
>> > > > identical. Making it possible for the same driver code to handle
>> > > > translating the parameters into HW config / FW requests, whether
>> > > > they shape at the device (devlink) or port (netdev) level.
>> > > > 
>> > > > Shaper NL for netdevs is separate from internal representation and
>> > > > driver API in my mind. My initial ask was to create the internal
>> > > > representation first, make sure it can express devlink and handful of
>> > > > exiting netdev APIs, and only once that's merged worry about exposing
>> > > > it via a new NL.
>> > > > 
>> > > > I'm not opposed to showing devlink shapers in netdev NL (RO as you say)
>> > > > but talking about it now strikes me as cart before the horse.
>> > > 
>> > > FTR, I don't see both of you on the same page ?!?
>> > > 
>> > > I read the above as Jiri's preference is a single ndo set to control
>> 
>> "Ndo" stands for netdev op and they are all tightly coupled with
>> netdevices. So, "single ndo set to control both devlink and netdev
>> shapers" sounds like nonsense to me.
>
>In this context, "NDOs" == set of function pointers operating on the same
>object.
>
>> > > Or to phrase the above differently, Jiri is focusing on the shaper
>> > > "binding" (how to locate/access it) while Jakub is focusing on the
>> > > shaper "info" (content/definition/attributes). Please correct me If I
>> > > misread something.
>> 
>> Two(or more) similar ops structs looks odd to me. I think that the ops
>> should should be shared and just the "binding point" should be somehow
>> abstracted out. Code speaks, let me draft how it could be done:
>> 
>> enum net_shaper_binding_type {
>>          NET_SHAPER_BINDING_TYPE_NETDEV,
>>          NET_SHAPER_BINDING_TYPE_DEVLINK_PORT,
>> };
>> 
>> struct net_shaper_binding {
>>          enum net_shaper_binding_type type;
>>          union {
>>                  struct net_device *netdev;
>>                  struct devlink_port *devlink_port;
>>          };
>> };
>> 
>> struct net_shaper_ops {
>> +       /**
>> +        * @group: create the specified shapers scheduling group
>> +        *
>> +        * Nest the @leaves shapers identified by @leaves_handles under the
>> +        * @root shaper identified by @root_handle. All the shapers belong
>> +        * to the network device @dev. The @leaves and @leaves_handles shaper
>> +        * arrays size is specified by @leaves_count.
>> +        * Create either the @leaves and the @root shaper; or if they already
>> +        * exists, links them together in the desired way.
>> +        * @leaves scope must be NET_SHAPER_SCOPE_QUEUE.
>> +        *
>> +        * Returns 0 on group successfully created, otherwise an negative
>> +        * error value and set @extack to describe the failure's reason.
>> +        */
>> +       int (*group)(const struct net_shaper_binding *binding, int leaves_count,
>> +                    const struct net_shaper_handle *leaves_handles,
>> +                    const struct net_shaper_info *leaves,
>> +                    const struct net_shaper_handle *root_handle,
>> +                    const struct net_shaper_info *root,
>> +                    struct netlink_ext_ack *extack);
>> +
>> +       /**
>> +        * @set: Updates the specified shaper
>> +        *
>> +        * Updates or creates the @shaper identified by the provided @handle
>> +        * on the given device @dev.
>> +        *
>> +        * Returns 0 on success, otherwise an negative
>> +        * error value and set @extack to describe the failure's reason.
>> +        */
>> +       int (*set)(const struct net_shaper_binding *binding,
>> +                  const struct net_shaper_handle *handle,
>> +                  const struct net_shaper_info *shaper,
>> +                  struct netlink_ext_ack *extack);
>> +
>> +       /**
>> +        * @delete: Removes the specified shaper from the NIC
>> +        *
>> +        * Removes the shaper configuration as identified by the given @handle
>> +        * on the specified device @dev, restoring the default behavior.
>> +        *
>> +        * Returns 0 on success, otherwise an negative
>> +        * error value and set @extack to describe the failure's reason.
>> +        */
>> +       int (*delete)(const struct net_shaper_binding *binding,
>> +                     const struct net_shaper_handle *handle,
>> +                     struct netlink_ext_ack *extack);
>> +};
>>
>> 
>> static inline struct net_device *
>> net_shaper_binding_netdev(struct net_shaper_binding *binding)
>> {
>>          WARN_ON(binding->type != NET_SHAPER_BINDING_TYPE_NETDEV)
>>          return binding->netdev;
>> }
>> 
>> static inline struct devlink_port *
>> net_shaper_binding_devlink_port(struct net_shaper_binding *binding)
>> {
>>          WARN_ON(binding->type != NET_SHAPER_BINDING_TYPE_DEVLINK_PORT)
>>          return binding->devlink_port;
>> }
>> 
>> Then whoever calls the op fills-up the binding structure accordingly.
>> 
>> 
>> drivers can implement ops, for netdev-bound shaper like this:
>> 
>> static int driverx_shaper_set(const struct net_shaper_binding *binding,
>>                                const struct net_shaper_handle *handle,
>>                                const struct net_shaper_info *shaper,
>>                                struct netlink_ext_ack *extack);
>> {
>>          struct net_device *netdev = net_shaper_binding_netdev(binding);
>>          ......
>> }
>> 
>> struct net_shaper_ops driverx_shaper_ops {
>>          .set = driverx_shaper_set;
>>          ......
>> };
>> 
>> static const struct net_device_ops driverx_netdev_ops = {
>>          .net_shaper_ops = &driverx_shaper_ops,
>>          ......
>> };
>
>If I read correctly, the net_shaper_ops caller will have to discriminate
>between net_device and devlink, do the object-type-specific lookup to get the
>relevant net_device (or devlink) object and then pass to such net_device (or
>devlink) a "generic" binding. Did I misread something? If so I must admit I
>really dislike such interface.

You are right. For example devlink rate set command handler will call
the op with devlink_port binding set.

>
>I personally think it would be much cleaner to have 2 separate set of
>operations, with exactly the same semantic and argument list, except for the
>first argument (struct net_device or struct devlink).

I think it is totally subjective. You like something, I like something
else. Both works. The amount of duplicity and need to change same
things on multiple places in case of bugfixes and extensions is what I
dislike on the 2 separate sets. Plus, there might be another binding in
the future, will you copy the ops struct again then?


>
>The driver implementation could still de-duplicate a lot of code, as far as
>the shaper-related arguments are the same.
>
>Side note, if the intention is to allow the user to touch/modify the
>queue-level and queue-group-level shapers via the devlink object? if that is
>the intention, we will need to drop the shaper cache and (re-)introduce a
>get() callback, as the same shaper could be reached via multiple
>binding/handle pairs and the core will not know all of such pairs for a given
>shaper.

That is a good question, I don't know. But gut feeling is "no".


>
>Thanks,
>
>Paolo
>

Powered by blists - more mailing lists