Re: [PATCH v2 net-next 02/12] tsnep: deny tc-taprio changes to per-tc max SDU

[Jakub Kicinski <kuba@kernel.org>]

On Tue, 27 Sep 2022 00:22:53 +0000 Vladimir Oltean wrote:
> On Mon, Sep 26, 2022 at 04:29:34PM -0700, Jakub Kicinski wrote:
> > I usually put a capability field into the ops themselves.  
> 
> Do you also have an example for the 'usual' manner?

struct devlink_ops {
	/**
	 * @supported_flash_update_params:
	 * mask of parameters supported by the driver's .flash_update
	 * implemementation.
	 */
	u32 supported_flash_update_params;
	unsigned long reload_actions;
	unsigned long reload_limits;

struct ethtool_ops {
	u32     cap_link_lanes_supported:1;
	u32	supported_coalesce_params;
	u32	supported_ring_params;

> > Right, but that's what's in the tree _now_. Experience teaches that
> > people may have out of tree code which implements TAPRIO and may send
> > it for upstream review without as much as testing it against net-next :(
> > As time passes and our memories fade the chances we'd catch such code
> > when posted upstream go down, perhaps from high to medium but still,
> > the explicit opt-in is more foolproof.  
> 
> You also need to see the flip side. You're making code more self-maintainable
> by adding bureaucracy to the run time itself. Whereas things could have
> been sorted out between the qdisc and the driver in just one ndo_setup_tc()
> call via the straightforward approach (every driver rejects what it
> doesn't like), now you need two calls for the normal case when the
> driver will accept a valid configuration.

Right, the lack of a structure we can put it in is quite unfortunate :(
But I do not dare suggesting we add a structure with qdisc and cls
specific callbacks instead of the mux-y ndo_setup_tc :)
I guess we could take a shortcut and put a pointer in netdev_ops for
just the caps for now, hm.

> I get the point and I think this won't probably make a big difference
> for a slow path like qdisc offload (at least it won't for me), but from
> an API perspective, once the mechanism will go in, it will become quite
> ossified, so it's best to ask some questions about it now.
> 
> Like for example you're funneling the caps through ndo_setup_tc(), which
> has these comments in its description:
> 
>  * int (*ndo_setup_tc)(struct net_device *dev, enum tc_setup_type type,
>  *		       void *type_data);
>  *	Called to setup any 'tc' scheduler, classifier or action on @dev.
>  *	This is always called from the stack with the rtnl lock held and netif
>  *	tx queues stopped. This allows the netdevice to perform queue
>  *	management safely.
> 
> Do we need to offer guarantees of rtnl lock and stopped TX queues to a
> function which just queries capabilities (and likely doesn't need them),
> or would it be better to devise a new ndo?

The queues stopped part is not true already for classifier offloads :(

> Generally, when you have a
> separate method to query caps vs to actually do the work, different
> calling contexts is generally the justification to do that, as opposed
> to piggy-backing the caps that the driver acted upon through the same
> struct tc_taprio_qopt_offload.

If we add a new pointer for netdev_ops I'd go with a struct pointer
rather than an op, for consistency if nothing else. But if you feel
strongly either way will work.