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

[Vladimir Oltean]

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?

> But since tc offloads don't have real ops (heh) we need to do the
> command callback thing. This is my knee-jerk coding of something:
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 9f42fc871c3b..2d043def76d8 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -960,6 +960,11 @@ enum tc_setup_type {
>  	TC_SETUP_QDISC_FIFO,
>  	TC_SETUP_QDISC_HTB,
>  	TC_SETUP_ACT,
> +	TC_QUERY_CAPS,
> +};
> +
> +struct tc_query_caps {
> +	u32 cmd;

actually s/u32/enum tc_setup_type/

inception....

>  };

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

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