Message-ID: <20240827140351.4e0c5445@kernel.org>
Date: Tue, 27 Aug 2024 14:03:51 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Jiri Pirko <jiri@...nulli.us>, 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 Tue, 27 Aug 2024 22:43:09 +0200 Paolo Abeni wrote:
> >> The main fact is that we do not agree on the above point - unify the
> >> shaper_ops between struct net_device and struct devlink.
> >>
> >> I think a 3rd party opinion could help moving forward.
> >> @Jakub could you please share your view here?  
> >
> > I don't mind Jiri's suggestion. Driver can declare its own helper:
> >
> > static struct drv_port *
> > drv_shaper_binding_to_port(const struct net_shaper_binding *binding)
> > {
> >       if (binding->type == NET_SHAPER_BINDING_TYPE_NETDEV)
> >               return /* netdev_priv() ? */;
> >       if (binding->type == NET_SHAPER_BINDING_TYPE_DEVLINK_PORT)
> >               return /* container_of() ? */;
> >       WARN_ONCE();
> >       return NULL;
> > }
> >
> > And call that instead of netdev_priv()?  
> 
> As I wrote this does not look like something that would help
> de-deuplicate any code, but since you both seem to agree...

To be clear. Given the helper above you can replace:

	struct iavf_adapter *adapter = netdev_priv(dev);

with:

	struct iavf_adapter *adapter = iavf_from_shaper_handle(handle);

In all the callbacks, and callbacks can now take devlink or other
"handles".

> Double checking before I rewrote a significant amount of the core code:
> 
> In the NL API, I will replace ifindex with binding, the latter will
> include nested attributes ifindex, bus_name
> and dev_name.

Any mention of the netlink API makes me worried that the "internal
representation should be separate from the uAPI" point is not
agreed on or at least not understood :( The NL API does not need other
object types. And there's currently no uAPI gap for devlink, AFAIK.
So the conversation about "handles" if quite forward-looking.

> Note that 'struct net_shaper_binding' must include a ‘struct devlink
> *’ as opposed to a
> ‘struct devlink_port *’, as mentioned in the code snippet so far, or
> we will have to drop the
> shaper cache and re-introduce a get() operation.
> 
> The ops will try to fetch the net_device or devlink according to the
> provided attributes.
> At the moment the ops will error out with ENOTSUP for netlink object.

No need to error out. Driver must not receive calls with object types
they don't support (we can add capabilities for this, later on, and
core should check, or do what Jiri suggests and just hook the ops into
different structs).

> Most core helpers
> will be re-factored to accept a 'binding *' argument in place of a
> 'struct net_device *'.
> 
> Full netlink support (for the brave that will implement it later) will
> likely require at least an
> additional scope (devlink_port), include the net_shaper_ops and
> net_shaper_data inside
> struct devlink, and likely code adaptation inside the core.

Back to uAPI worries. FWIW I think that how we pass the handle to
drivers is of relatively little importance. It's small amount of
mechanical work to change it later. But you asked me about the proposal
so I answered. To me representing the other shaper APIs in terms of 
the new driver facing API *within netdev* is more important.

Literally the only change I would have expected based on this branch of
the conversation is slight adjustment to the parameters to ops. Nothing
else. No netlink changes. Only core change would be to wrap the ops.

Powered by blists - more mailing lists