Message-ID: <c7e0547b-a1e4-4e47-b7ec-010aa92fbc3a@redhat.com>
Date: Fri, 23 Aug 2024 14:58:27 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jiri Pirko <jiri@...nulli.us>, Jakub Kicinski <kuba@...nel.org>
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>
Subject: Re: [PATCH v3 03/12] net-shapers: implement NL get operation

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.

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

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.

Thanks,

Paolo

Powered by blists - more mailing lists