* Re: [net-next PATCH] taprio: don't reject same mqprio settings
2019-11-15 1:56 [net-next PATCH] taprio: don't reject same mqprio settings Ivan Khoronzhuk
@ 2019-11-15 20:38 ` Vladimir Oltean
2019-11-15 21:21 ` Vinicius Costa Gomes
2019-11-16 20:42 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Vladimir Oltean @ 2019-11-15 20:38 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, David S. Miller, Vinicius Costa Gomes, Jamal Hadi Salim,
Cong Wang, Jiri Pirko, lkml
On Fri, 15 Nov 2019 at 03:58, Ivan Khoronzhuk
<ivan.khoronzhuk@linaro.org> wrote:
>
> The taprio qdisc allows to set mqprio setting but only once. In case
> if mqprio settings are provided next time the error is returned as
> it's not allowed to change traffic class mapping in-flignt and that
> is normal. But if configuration is absolutely the same - no need to
> return error. It allows to provide same command couple times,
> changing only base time for instance, or changing only scheds maps,
> but leaving mqprio setting w/o modification. It more corresponds the
> message: "Changing the traffic mapping of a running schedule is not
> supported", so reject mqprio if it's really changed.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.com>
I would even kindly suggest a tag:
Fixes: a3d43c0d56f1 ("taprio: Add support adding an admin schedule")
since the patch is doing nothing but making the tc-taprio command
idempotent, aka running it 10 times in a row produces the same result.
Previously, it would have worked the first time but failed the rest of
9 times, which is catastrophic for any sort of scripted environments.
It should have behaved like this from the beginning.
The problem is that it conflicts trivially with 9c66d1564676 ("taprio:
Add support for hardware offloading"), which made its appearance in
5.4. It's up to you if you want to rebase this on top of 5.4 as well,
for the stable trees.
> net/sched/sch_taprio.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 7cd68628c637..bd844f2cbf7a 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1347,6 +1347,26 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
> return err;
> }
>
> +static int taprio_mqprio_cmp(struct net_device *dev,
> + struct tc_mqprio_qopt *mqprio)
> +{
> + int i;
> +
> + if (mqprio->num_tc != dev->num_tc)
> + return -1;
> +
> + for (i = 0; i < mqprio->num_tc; i++)
> + if (dev->tc_to_txq[i].count != mqprio->count[i] ||
> + dev->tc_to_txq[i].offset != mqprio->offset[i])
> + return -1;
> +
> + for (i = 0; i < TC_BITMASK + 1; i++)
Huh, odd, I wonder what's wrong with <= these days. I do see it's
being used like that in 2 more places in the code, so let's opt for
consistency.
> + if (dev->prio_tc_map[i] != mqprio->prio_tc_map[i])
> + return -1;
> +
> + return 0;
> +}
> +
> static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
> struct netlink_ext_ack *extack)
> {
> @@ -1398,6 +1418,10 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
> admin = rcu_dereference(q->admin_sched);
> rcu_read_unlock();
>
> + /* no changes - no new mqprio settings */
> + if (mqprio && !taprio_mqprio_cmp(dev, mqprio))
> + mqprio = NULL;
> +
> if (mqprio && (oper || admin)) {
> NL_SET_ERR_MSG(extack, "Changing the traffic mapping of a running schedule is not supported");
> err = -ENOTSUPP;
> --
> 2.20.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [net-next PATCH] taprio: don't reject same mqprio settings
2019-11-15 1:56 [net-next PATCH] taprio: don't reject same mqprio settings Ivan Khoronzhuk
2019-11-15 20:38 ` Vladimir Oltean
@ 2019-11-15 21:21 ` Vinicius Costa Gomes
2019-11-16 20:42 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: Vinicius Costa Gomes @ 2019-11-15 21:21 UTC (permalink / raw)
To: Ivan Khoronzhuk, netdev, davem
Cc: jhs, xiyou.wangcong, jiri, linux-kernel, Ivan Khoronzhuk
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> writes:
> The taprio qdisc allows to set mqprio setting but only once. In case
> if mqprio settings are provided next time the error is returned as
> it's not allowed to change traffic class mapping in-flignt and that
> is normal. But if configuration is absolutely the same - no need to
> return error. It allows to provide same command couple times,
> changing only base time for instance, or changing only scheds maps,
> but leaving mqprio setting w/o modification. It more corresponds the
> message: "Changing the traffic mapping of a running schedule is not
> supported", so reject mqprio if it's really changed.
>
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> net/sched/sch_taprio.c | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index 7cd68628c637..bd844f2cbf7a 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1347,6 +1347,26 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
> return err;
> }
>
> +static int taprio_mqprio_cmp(struct net_device *dev,
> + struct tc_mqprio_qopt *mqprio)
Nitpick: for these kinds of functions I like to add a 'const' to the parameters
at least as documentation that it doesn't modify its arguments.
> +{
> + int i;
> +
> + if (mqprio->num_tc != dev->num_tc)
> + return -1;
Optional: you could move the check for a NULL mqprio inside this
function. Perhaps, for that to make sense you would need to change the
function name to taprio_mqprio_check() or something.
These are all optional.
Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [net-next PATCH] taprio: don't reject same mqprio settings
2019-11-15 1:56 [net-next PATCH] taprio: don't reject same mqprio settings Ivan Khoronzhuk
2019-11-15 20:38 ` Vladimir Oltean
2019-11-15 21:21 ` Vinicius Costa Gomes
@ 2019-11-16 20:42 ` David Miller
2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2019-11-16 20:42 UTC (permalink / raw)
To: ivan.khoronzhuk
Cc: netdev, vinicius.gomes, jhs, xiyou.wangcong, jiri, linux-kernel
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Fri, 15 Nov 2019 03:56:07 +0200
> @@ -1347,6 +1347,26 @@ static int taprio_parse_clockid(struct Qdisc *sch, struct nlattr **tb,
> return err;
> }
>
> +static int taprio_mqprio_cmp(struct net_device *dev,
> + struct tc_mqprio_qopt *mqprio)
> +{
...
> static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
> struct netlink_ext_ack *extack)
> {
> @@ -1398,6 +1418,10 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
> admin = rcu_dereference(q->admin_sched);
> rcu_read_unlock();
>
> + /* no changes - no new mqprio settings */
> + if (mqprio && !taprio_mqprio_cmp(dev, mqprio))
> + mqprio = NULL;
> +
I like Vinicius's feedback, please make the new helper function have
the signature:
static int taprio_mqprio_cmp(const struct net_device *dev,
const struct tc_mqprio_qopt *mqprio)
And make the NULL check in there instead of at the caller.
Please also remember to add the Fixes: tag.
Thanks.
^ permalink raw reply [flat|nested] 4+ messages in thread