[net-next PATCH] taprio: don't reject same mqprio settings

[net-next PATCH] taprio: don't reject same mqprio settings

[Ivan Khoronzhuk]

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)
+{
+	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++)
+		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

Re: [net-next PATCH] taprio: don't reject same mqprio settings

[Vladimir Oltean]

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
>

Re: [net-next PATCH] taprio: don't reject same mqprio settings

[Vinicius Costa Gomes]

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>

Re: [net-next PATCH] taprio: don't reject same mqprio settings

[David Miller]

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.