Message-ID: <Zsh3ecwUICabLyHV@nanopsycho.orion>
Date: Fri, 23 Aug 2024 13:50:17 +0200
From: Jiri Pirko <jiri@...nulli.us>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Paolo Abeni <pabeni@...hat.com>, 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 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.


>> both devlink and device shapers, while I read Jakub's preference as for
>> different sets of operations that will use the same arguments to specify
>> the shaper informations.
>
>Jiri replied:
>
>  > which kind of object should implement the ndo_shaper_ops callbacks?
>
>  Whoever implements the shaper in driver. If that is net_device tight
>  shaper, driver should work with net_device. If that is devlink port
>  related shaper, driver should work on top of devlink port based api.
>
>I interpret this as having two almost identical versions of shaper ops,
>the only difference is that one takes netdev and the other devlink port.

Could be done like that. But see more below.


>We could simplify it slightly, and call the ndo for getting devlink
>port from netdev, and always pass devlink port in?

No please. Keep that separate. You can't always rely on devlink port
having netdev paired with it. Plus, it would be odd callpath from
devlink port code to netdev op. Not to mention locking :)


>
>I _think_ (but I'm not 100% sure) that Jiri does _not_ mean that we
>would be able to render the internal shaper tree as ops for the
>existing devlink rate API. Because that may cause scope creep,
>inconsistencies and duplication.

Not sure what you mean by this. Devlink rate UAPI will stay the same.
Only the backend will use new driver API instead of the existing one.


>
>> 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,
        ......
};



drivers can implement ops, for devlink_port-bound shaper like this:

static int drivery_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 devlink_port *devlink_port = net_shaper_binding_devlink_port(binding);

        ......
}

struct net_shaper_ops drivery_shaper_ops {
        .set = drivery_shaper_set;
        ......
};

static const struct devlink_port_ops drivery_devlink_port_ops = {
        .port_shaper_ops = &drivery_shaper_ops,
};



Some driver can even have one ops implementation for both,
and distinguish just by looking at binding->type.


>> 
>> Still for the record, I interpret the current proposal as not clashing
>> with Jakub's preference, and being tolerated from Jiri, again please 
>> correct me if I read too far.
>
>One more thing, Jiri said:
>
>  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.
>
>Consolidation was indeed one of the goals, and I share Jiri's concern :(

Good.

Powered by blists - more mailing lists