| Message ID | 20220923163310.3192733-3-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add tc-taprio support for queueMaxSDU | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 16 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Fri, 23 Sep 2022 19:33:00 +0300 Vladimir Oltean wrote: > Since the driver does not act upon the max_sdu argument, deny any other > values except the default all-zeroes, which means that all traffic > classes should use the same MTU as the port itself. Don't all the driver patches make you wanna turn this into an opt-in? What are the chances we'll catch all drivers missing the validation in review?
On Mon, Sep 26, 2022 at 01:40:25PM -0700, Jakub Kicinski wrote: > On Fri, 23 Sep 2022 19:33:00 +0300 Vladimir Oltean wrote: > > Since the driver does not act upon the max_sdu argument, deny any other > > values except the default all-zeroes, which means that all traffic > > classes should use the same MTU as the port itself. > > 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? > 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.
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.
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.
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.
diff --git a/drivers/net/ethernet/engleder/tsnep_tc.c b/drivers/net/ethernet/engleder/tsnep_tc.c index c4c6e1357317..82df837ffc54 100644 --- a/drivers/net/ethernet/engleder/tsnep_tc.c +++ b/drivers/net/ethernet/engleder/tsnep_tc.c @@ -320,11 +320,15 @@ static int tsnep_taprio(struct tsnep_adapter *adapter, { struct tsnep_gcl *gcl; struct tsnep_gcl *curr; - int retval; + int retval, tc; if (!adapter->gate_control) return -EOPNOTSUPP; + for (tc = 0; tc < TC_MAX_QUEUE; tc++) + if (qopt->max_sdu[tc]) + return -EOPNOTSUPP; + if (!qopt->enable) { /* disable gate control if active */ mutex_lock(&adapter->gate_control_lock);
Since the driver does not act upon the max_sdu argument, deny any other values except the default all-zeroes, which means that all traffic classes should use the same MTU as the port itself. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: none drivers/net/ethernet/engleder/tsnep_tc.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)