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@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---

Reviewed-by: Vladimir Oltean <olteanv@xxxxxxxxx>
Tested-by: Vladimir Oltean <olteanv@xxxxxxxxx>

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
>