| Message ID | 20230204135307.1036988-3-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5cfb45e2fb71f58e795d96c708c27e2fa13134b7 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ENETC mqprio/taprio cleanup | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next, async |
| 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: 1 this patch: 1 |
| netdev/cc_maintainers | success | CCed 8 of 8 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: 1 this patch: 1 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 123 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Hi! On Sat, 2023-02-04 at 15:52 +0200, Vladimir Oltean wrote: > Some more logic will be added to mqprio offloading, so split that > code > up from mqprio_init(), which is already large, and create a new > function, mqprio_enable_offload(), similar to > taprio_enable_offload(). > Also create the opposite function mqprio_disable_offload(). > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > Reviewed-by: Jacob Keller <jacob.e.keller@intel.com> > Reviewed-by: Simon Horman <simon.horman@corigine.com> > --- > v1->v6: none > > net/sched/sch_mqprio.c | 102 ++++++++++++++++++++++++--------------- > -- > 1 file changed, 59 insertions(+), 43 deletions(-) > > diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c > index d2d8a02ded05..3579a64da06e 100644 > --- a/net/sched/sch_mqprio.c > +++ b/net/sched/sch_mqprio.c > @@ -27,6 +27,61 @@ struct mqprio_sched { > u64 max_rate[TC_QOPT_MAX_QUEUE]; > }; > > +static int mqprio_enable_offload(struct Qdisc *sch, > + const struct tc_mqprio_qopt *qopt) > +{ > + struct tc_mqprio_qopt_offload mqprio = {.qopt = *qopt}; > + struct mqprio_sched *priv = qdisc_priv(sch); > + struct net_device *dev = qdisc_dev(sch); > + int err, i; > + > + switch (priv->mode) { > + case TC_MQPRIO_MODE_DCB: > + if (priv->shaper != TC_MQPRIO_SHAPER_DCB) > + return -EINVAL; > + break; > + case TC_MQPRIO_MODE_CHANNEL: > + mqprio.flags = priv->flags; > + if (priv->flags & TC_MQPRIO_F_MODE) > + mqprio.mode = priv->mode; > + if (priv->flags & TC_MQPRIO_F_SHAPER) > + mqprio.shaper = priv->shaper; > + if (priv->flags & TC_MQPRIO_F_MIN_RATE) > + for (i = 0; i < mqprio.qopt.num_tc; i++) > + mqprio.min_rate[i] = priv- > >min_rate[i]; > + if (priv->flags & TC_MQPRIO_F_MAX_RATE) > + for (i = 0; i < mqprio.qopt.num_tc; i++) > + mqprio.max_rate[i] = priv- > >max_rate[i]; > + break; > + default: > + return -EINVAL; > + } > + > + err = dev->netdev_ops->ndo_setup_tc(dev, > TC_SETUP_QDISC_MQPRIO, > + &mqprio); > + if (err) > + return err; > + > + priv->hw_offload = mqprio.qopt.hw; > + > + return 0; > +} > + > +static void mqprio_disable_offload(struct Qdisc *sch) > +{ > + struct tc_mqprio_qopt_offload mqprio = { { 0 } }; > + struct mqprio_sched *priv = qdisc_priv(sch); > + struct net_device *dev = qdisc_dev(sch); > + > + switch (priv->mode) { > + case TC_MQPRIO_MODE_DCB: > + case TC_MQPRIO_MODE_CHANNEL: > + dev->netdev_ops->ndo_setup_tc(dev, > TC_SETUP_QDISC_MQPRIO, > + &mqprio); > + break; > + } > +} > + > static void mqprio_destroy(struct Qdisc *sch) > { > struct net_device *dev = qdisc_dev(sch); > @@ -41,22 +96,10 @@ static void mqprio_destroy(struct Qdisc *sch) > kfree(priv->qdiscs); > } > > - if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc) { > - struct tc_mqprio_qopt_offload mqprio = { { 0 } }; > - > - switch (priv->mode) { > - case TC_MQPRIO_MODE_DCB: > - case TC_MQPRIO_MODE_CHANNEL: > - dev->netdev_ops->ndo_setup_tc(dev, > - > TC_SETUP_QDISC_MQPRIO, > - &mqprio); > - break; > - default: > - return; > - } > - } else { > + if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc) > + mqprio_disable_offload(sch); > + else > netdev_set_num_tc(dev, 0); > - } > } > > static int mqprio_parse_opt(struct net_device *dev, struct > tc_mqprio_qopt *qopt) > @@ -253,36 +296,9 @@ static int mqprio_init(struct Qdisc *sch, struct > nlattr *opt, > * supplied and verified mapping > */ > if (qopt->hw) { > - struct tc_mqprio_qopt_offload mqprio = {.qopt = > *qopt}; > - > - switch (priv->mode) { > - case TC_MQPRIO_MODE_DCB: > - if (priv->shaper != TC_MQPRIO_SHAPER_DCB) > - return -EINVAL; > - break; > - case TC_MQPRIO_MODE_CHANNEL: > - mqprio.flags = priv->flags; > - if (priv->flags & TC_MQPRIO_F_MODE) > - mqprio.mode = priv->mode; > - if (priv->flags & TC_MQPRIO_F_SHAPER) > - mqprio.shaper = priv->shaper; > - if (priv->flags & TC_MQPRIO_F_MIN_RATE) > - for (i = 0; i < mqprio.qopt.num_tc; > i++) > - mqprio.min_rate[i] = priv- > >min_rate[i]; > - if (priv->flags & TC_MQPRIO_F_MAX_RATE) > - for (i = 0; i < mqprio.qopt.num_tc; > i++) > - mqprio.max_rate[i] = priv- > >max_rate[i]; > - break; > - default: > - return -EINVAL; > - } > - err = dev->netdev_ops->ndo_setup_tc(dev, > - > TC_SETUP_QDISC_MQPRIO, > - &mqprio); > + err = mqprio_enable_offload(sch, qopt); > if (err) > return err; > - > - priv->hw_offload = mqprio.qopt.hw; > } else { > netdev_set_num_tc(dev, qopt->num_tc); > for (i = 0; i < qopt->num_tc; i++) This patch just code refactoring or it modifies the default behavior of the offloading too? I'm asking it in regards of the veth interface. When you configure mqprio, the "hw" parameter is mandatory. By default, it tries to configure it with "hw 1". However as a result, veth spit back "Invalid argument" error (before your patches). Same happens after this patch too, right? For veth hardware offloading makes no sense, but giving the "hw 0" argument explicitly as mqprio parameter might counterintuitive. Best, Ferenc
Hi Ferenc, On Thu, Feb 16, 2023 at 02:05:22PM +0100, Ferenc Fejes wrote: > This patch just code refactoring or it modifies the default behavior of > the offloading too? I'm asking it in regards of the veth interface. > When you configure mqprio, the "hw" parameter is mandatory. By default, > it tries to configure it with "hw 1". However as a result, veth spit > back "Invalid argument" error (before your patches). Same happens after > this patch too, right? Yup. iproute2 has a default queue configuration built in, if nothing else is specified, and this has "hw 1": https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/tc/q_mqprio.c#n36 > For veth hardware offloading makes no sense, but giving the "hw 0" > argument explicitly as mqprio parameter might counterintuitive. Agree that giving the right nlattrs to mqprio and trying to slalom through their validation is a frustrating minesweeper game. I have some patches which add some netlink EXT_ACK messages to make this a bit less sour. I'm regression-testing those, together with some other mqprio changes and I hope to send them soon. OTOH, "hw 1" is mandatory with the "mode", "shaper", "min_rate" and "max_rate" options. This is logical when you think about it (driver has to act upon them), but indeed it makes mqprio difficult to configure. With veth, you need to use multi-queue to make use of mqprio/taprio, have you done that? ip link add veth0 numtxqueues 8 numrxqueues 8 type veth peer name veth1
Hi Vladimir! On Thu, 2023-02-16 at 16:28 +0200, Vladimir Oltean wrote: > Hi Ferenc, > > On Thu, Feb 16, 2023 at 02:05:22PM +0100, Ferenc Fejes wrote: > > This patch just code refactoring or it modifies the default > > behavior of > > the offloading too? I'm asking it in regards of the veth interface. > > When you configure mqprio, the "hw" parameter is mandatory. By > > default, > > it tries to configure it with "hw 1". However as a result, veth > > spit > > back "Invalid argument" error (before your patches). Same happens > > after > > this patch too, right? > > Yup. iproute2 has a default queue configuration built in, if nothing > else is specified, and this has "hw 1": > https://git.kernel.org/pub/scm/network/iproute2/iproute2.git/tree/tc/q_mqprio.c#n36 > > > For veth hardware offloading makes no sense, but giving the "hw 0" > > argument explicitly as mqprio parameter might counterintuitive. > > Agree that giving the right nlattrs to mqprio and trying to slalom > through their validation is a frustrating minesweeper game. I have > some > patches which add some netlink EXT_ACK messages to make this a bit > less > sour. I'm regression-testing those, together with some other mqprio > changes and I hope to send them soon. Nice, good to hear! > > OTOH, "hw 1" is mandatory with the "mode", "shaper", "min_rate" and > "max_rate" options. This is logical when you think about it (driver > has > to act upon them), but indeed it makes mqprio difficult to configure. > > With veth, you need to use multi-queue to make use of mqprio/taprio, > have you done that? > > ip link add veth0 numtxqueues 8 numrxqueues 8 type veth peer name > veth1 Yes, usually I done it with ethtool --set-channels veth0 tx 8 but I guess that both resulting the same. Best, Ferenc
diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c index d2d8a02ded05..3579a64da06e 100644 --- a/net/sched/sch_mqprio.c +++ b/net/sched/sch_mqprio.c @@ -27,6 +27,61 @@ struct mqprio_sched { u64 max_rate[TC_QOPT_MAX_QUEUE]; }; +static int mqprio_enable_offload(struct Qdisc *sch, + const struct tc_mqprio_qopt *qopt) +{ + struct tc_mqprio_qopt_offload mqprio = {.qopt = *qopt}; + struct mqprio_sched *priv = qdisc_priv(sch); + struct net_device *dev = qdisc_dev(sch); + int err, i; + + switch (priv->mode) { + case TC_MQPRIO_MODE_DCB: + if (priv->shaper != TC_MQPRIO_SHAPER_DCB) + return -EINVAL; + break; + case TC_MQPRIO_MODE_CHANNEL: + mqprio.flags = priv->flags; + if (priv->flags & TC_MQPRIO_F_MODE) + mqprio.mode = priv->mode; + if (priv->flags & TC_MQPRIO_F_SHAPER) + mqprio.shaper = priv->shaper; + if (priv->flags & TC_MQPRIO_F_MIN_RATE) + for (i = 0; i < mqprio.qopt.num_tc; i++) + mqprio.min_rate[i] = priv->min_rate[i]; + if (priv->flags & TC_MQPRIO_F_MAX_RATE) + for (i = 0; i < mqprio.qopt.num_tc; i++) + mqprio.max_rate[i] = priv->max_rate[i]; + break; + default: + return -EINVAL; + } + + err = dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, + &mqprio); + if (err) + return err; + + priv->hw_offload = mqprio.qopt.hw; + + return 0; +} + +static void mqprio_disable_offload(struct Qdisc *sch) +{ + struct tc_mqprio_qopt_offload mqprio = { { 0 } }; + struct mqprio_sched *priv = qdisc_priv(sch); + struct net_device *dev = qdisc_dev(sch); + + switch (priv->mode) { + case TC_MQPRIO_MODE_DCB: + case TC_MQPRIO_MODE_CHANNEL: + dev->netdev_ops->ndo_setup_tc(dev, TC_SETUP_QDISC_MQPRIO, + &mqprio); + break; + } +} + static void mqprio_destroy(struct Qdisc *sch) { struct net_device *dev = qdisc_dev(sch); @@ -41,22 +96,10 @@ static void mqprio_destroy(struct Qdisc *sch) kfree(priv->qdiscs); } - if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc) { - struct tc_mqprio_qopt_offload mqprio = { { 0 } }; - - switch (priv->mode) { - case TC_MQPRIO_MODE_DCB: - case TC_MQPRIO_MODE_CHANNEL: - dev->netdev_ops->ndo_setup_tc(dev, - TC_SETUP_QDISC_MQPRIO, - &mqprio); - break; - default: - return; - } - } else { + if (priv->hw_offload && dev->netdev_ops->ndo_setup_tc) + mqprio_disable_offload(sch); + else netdev_set_num_tc(dev, 0); - } } static int mqprio_parse_opt(struct net_device *dev, struct tc_mqprio_qopt *qopt) @@ -253,36 +296,9 @@ static int mqprio_init(struct Qdisc *sch, struct nlattr *opt, * supplied and verified mapping */ if (qopt->hw) { - struct tc_mqprio_qopt_offload mqprio = {.qopt = *qopt}; - - switch (priv->mode) { - case TC_MQPRIO_MODE_DCB: - if (priv->shaper != TC_MQPRIO_SHAPER_DCB) - return -EINVAL; - break; - case TC_MQPRIO_MODE_CHANNEL: - mqprio.flags = priv->flags; - if (priv->flags & TC_MQPRIO_F_MODE) - mqprio.mode = priv->mode; - if (priv->flags & TC_MQPRIO_F_SHAPER) - mqprio.shaper = priv->shaper; - if (priv->flags & TC_MQPRIO_F_MIN_RATE) - for (i = 0; i < mqprio.qopt.num_tc; i++) - mqprio.min_rate[i] = priv->min_rate[i]; - if (priv->flags & TC_MQPRIO_F_MAX_RATE) - for (i = 0; i < mqprio.qopt.num_tc; i++) - mqprio.max_rate[i] = priv->max_rate[i]; - break; - default: - return -EINVAL; - } - err = dev->netdev_ops->ndo_setup_tc(dev, - TC_SETUP_QDISC_MQPRIO, - &mqprio); + err = mqprio_enable_offload(sch, qopt); if (err) return err; - - priv->hw_offload = mqprio.qopt.hw; } else { netdev_set_num_tc(dev, qopt->num_tc); for (i = 0; i < qopt->num_tc; i++)