lkml.org 
[lkml]   [2022]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v2 net-next 02/12] tsnep: deny tc-taprio changes to per-tc max SDU
On Tue, 27 Sep 2022 00:50:49 +0300 Vladimir Oltean wrote:
> > Don't all the driver patches make you wanna turn this into an opt-in?
>
> Presumably you're thinking of a way through which the caller of
> ndo_setup_tc(TC_SETUP_QDISC_TAPRIO, struct tc_taprio_qopt_offload *)
> knows whether the driver took the new max_sdu field into consideration,
> and not just accepted it blindly?
>
> I'm not exactly up to date with all the techniques which can achieve
> that without changes in drivers, and I haven't noticed other qdisc
> offloads doing it either... but this would be a great trick to learn for
> sure. Do you have any idea?

I usually put a capability field into the ops themselves. 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;
};

/* These structures hold the attributes of bpf state that are being passed
diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h
index 2ff80cd04c5c..2416151a23db 100644
--- a/include/net/pkt_sched.h
+++ b/include/net/pkt_sched.h
@@ -155,6 +155,12 @@ struct tc_etf_qopt_offload {
s32 queue;
};

+struct tc_taprio_drv_caps {
+ struct tc_query_caps base;
+
+ bool accept_max_sdu;
+};
+
struct tc_taprio_sched_entry {
u8 command; /* TC_TAPRIO_CMD_* */

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 136ae21ebce9..68302ee33937 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1219,6 +1219,7 @@ static int taprio_enable_offload(struct net_device *dev,
struct sched_gate_list *sched,
struct netlink_ext_ack *extack)
{
+ struct tc_taprio_drv_caps caps = { { .cmd = TC_SETUP_QDISC_TAPRIO, }, };
const struct net_device_ops *ops = dev->netdev_ops;
struct tc_taprio_qopt_offload *offload;
int err = 0;
@@ -1229,6 +1230,12 @@ static int taprio_enable_offload(struct net_device *dev,
return -EOPNOTSUPP;
}

+ ops->ndo_setup_tc(dev, TC_QUERY_CAPS, &caps);
+ if (!caps.accept_max_sdu && taprio_is_max_sdu_used(...)) {
+ NL_SET_ERR_MSG(extack, "nope.");
+ return -EOPNOTSUPP;
+ }
+
offload = taprio_offload_alloc(sched->num_entries);
if (!offload) {
NL_SET_ERR_MSG(extack,
> > What are the chances we'll catch all drivers missing the validation
> > in review?
>
> Not that slim I think, they are all identifiable if you search for
> TC_SETUP_QDISC_TAPRIO.

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.

\
 
 \ /
  Last update: 2022-09-27 01:30    [W:2.239 / U:0.184 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site