[PATCH net v2 0/3] taprio: Some fixes

[PATCH net v2 0/3] taprio: Some fixes

[Vinicius Costa Gomes]

Hi,

Changes from v1:
  - Fixed ignoring the 'flags' argument when adding a new
    instance (Vladimir Oltean);
  - Changed the order of commits;

Updated cover letter:

Some fixes for taprio, mostly (2/3 and 3/3) related to the 'flags'
argument:

Patch 1/3: Reported by Po Liu, is more of a improvement of usability for
drivers implementing offloading features, now they can rely on the
value of dev->num_tc, instead of going through some hops to get this
value.

Patch 2/3: Use 'q->flags' as the source of truth for the offloading
flags.

Patch 3/3: Fixes the issue that changing the flags during runtime was
still allowed (with bad results). The solution was to initialize
'q->flags' with an invalid value.


Cheers,
--
Vinicius

Vinicius Costa Gomes (3):
  taprio: Fix enabling offload with wrong number of traffic classes
  taprio: Allow users not to specify "flags" when changing schedules
  taprio: Fix still allowing changing the flags during runtime

 net/sched/sch_taprio.c | 87 ++++++++++++++++++++++++++----------------
 1 file changed, 54 insertions(+), 33 deletions(-)

-- 
2.25.0

[PATCH net v2 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[Vinicius Costa Gomes]

If the driver implementing taprio offloading depends on the value of
the network device number of traffic classes (dev->num_tc) for
whatever reason, it was going to receive the value zero. The value was
only set after the offloading function is called.

So, moving setting the number of traffic classes to before the
offloading function is called fixes this issue. This is safe because
this only happens when taprio is instantiated (we don't allow this
configuration to be changed without first removing taprio).

Fixes: 9c66d1564676 ("taprio: Add support for hardware offloading")
Reported-by: Po Liu <po.liu@nxp.com>
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/sched/sch_taprio.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index c609373c8661..ad0dadcfcdba 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1444,6 +1444,19 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 
 	taprio_set_picos_per_byte(dev, q);
 
+	if (mqprio) {
+		netdev_set_num_tc(dev, mqprio->num_tc);
+		for (i = 0; i < mqprio->num_tc; i++)
+			netdev_set_tc_queue(dev, i,
+					    mqprio->count[i],
+					    mqprio->offset[i]);
+
+		/* Always use supplied priority mappings */
+		for (i = 0; i <= TC_BITMASK; i++)
+			netdev_set_prio_tc_map(dev, i,
+					       mqprio->prio_tc_map[i]);
+	}
+
 	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
 		err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
 	else
@@ -1471,19 +1484,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->advance_timer.function = advance_sched;
 	}
 
-	if (mqprio) {
-		netdev_set_num_tc(dev, mqprio->num_tc);
-		for (i = 0; i < mqprio->num_tc; i++)
-			netdev_set_tc_queue(dev, i,
-					    mqprio->count[i],
-					    mqprio->offset[i]);
-
-		/* Always use supplied priority mappings */
-		for (i = 0; i <= TC_BITMASK; i++)
-			netdev_set_prio_tc_map(dev, i,
-					       mqprio->prio_tc_map[i]);
-	}
-
 	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
 		q->dequeue = taprio_dequeue_offload;
 		q->peek = taprio_peek_offload;
-- 
2.25.0

Re: [PATCH net v2 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[David Miller]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Tue, 28 Jan 2020 15:52:25 -0800

> @@ -1444,6 +1444,19 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
>  
>  	taprio_set_picos_per_byte(dev, q);
>  
> +	if (mqprio) {
> +		netdev_set_num_tc(dev, mqprio->num_tc);
> +		for (i = 0; i < mqprio->num_tc; i++)
> +			netdev_set_tc_queue(dev, i,
> +					    mqprio->count[i],
> +					    mqprio->offset[i]);
> +
> +		/* Always use supplied priority mappings */
> +		for (i = 0; i <= TC_BITMASK; i++)
> +			netdev_set_prio_tc_map(dev, i,
> +					       mqprio->prio_tc_map[i]);
> +	}
> +
>  	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
>  		err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
>  	else

This feedback applies to the existing code too, but don't we need to have
a call to netdev_reset_tc() in the error paths after we commit these
settings?

Because ->num_tc for the device should be reset to zero for sure if we
can't complete this configuration change successfully.

Re: [PATCH net v2 1/3] taprio: Fix enabling offload with wrong number of traffic classes

[Vinicius Costa Gomes]

Hi,

David Miller <davem@davemloft.net> writes:

> This feedback applies to the existing code too, but don't we need to have
> a call to netdev_reset_tc() in the error paths after we commit these
> settings?
>
> Because ->num_tc for the device should be reset to zero for sure if we
> can't complete this configuration change successfully.

As we can only change ->num_tc in the _init() path, if any error happens,
_init() will fail and taprio_destroy() will be called, reseting num_tc to
zero.

And, yeah, in taprio_destoy() calling netdev_reset_tc() is better than
netdev_set_num_tc(dev, 0). Will fix this.


Cheers,
--
Vinicius

[PATCH net v2 2/3] taprio: Allow users not to specify "flags" when changing schedules

[Vinicius Costa Gomes]

When any offload mode is enabled, users had to specify the
"flags" parameter when adding a new "admin" schedule.

This fix allows that parameter to be omitted when adding a new
schedule.

This will make that we have one source of truth for 'flags'.

Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 net/sched/sch_taprio.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index ad0dadcfcdba..110143fba114 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1402,7 +1402,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->flags = taprio_flags;
 	}
 
-	err = taprio_parse_mqprio_opt(dev, mqprio, extack, taprio_flags);
+	err = taprio_parse_mqprio_opt(dev, mqprio, extack, q->flags);
 	if (err < 0)
 		return err;
 
@@ -1457,7 +1457,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 					       mqprio->prio_tc_map[i]);
 	}
 
-	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
+	if (FULL_OFFLOAD_IS_ENABLED(q->flags))
 		err = taprio_enable_offload(dev, mqprio, q, new_admin, extack);
 	else
 		err = taprio_disable_offload(dev, q, extack);
@@ -1477,14 +1477,14 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		q->txtime_delay = nla_get_u32(tb[TCA_TAPRIO_ATTR_TXTIME_DELAY]);
 	}
 
-	if (!TXTIME_ASSIST_IS_ENABLED(taprio_flags) &&
-	    !FULL_OFFLOAD_IS_ENABLED(taprio_flags) &&
+	if (!TXTIME_ASSIST_IS_ENABLED(q->flags) &&
+	    !FULL_OFFLOAD_IS_ENABLED(q->flags) &&
 	    !hrtimer_active(&q->advance_timer)) {
 		hrtimer_init(&q->advance_timer, q->clockid, HRTIMER_MODE_ABS);
 		q->advance_timer.function = advance_sched;
 	}
 
-	if (FULL_OFFLOAD_IS_ENABLED(taprio_flags)) {
+	if (FULL_OFFLOAD_IS_ENABLED(q->flags)) {
 		q->dequeue = taprio_dequeue_offload;
 		q->peek = taprio_peek_offload;
 	} else {
@@ -1501,7 +1501,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 		goto unlock;
 	}
 
-	if (TXTIME_ASSIST_IS_ENABLED(taprio_flags)) {
+	if (TXTIME_ASSIST_IS_ENABLED(q->flags)) {
 		setup_txtime(q, new_admin, start);
 
 		if (!oper) {
@@ -1528,7 +1528,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 
 		spin_unlock_irqrestore(&q->current_entry_lock, flags);
 
-		if (FULL_OFFLOAD_IS_ENABLED(taprio_flags))
+		if (FULL_OFFLOAD_IS_ENABLED(q->flags))
 			taprio_offload_config_changed(q);
 	}
 
-- 
2.25.0

Re: [PATCH net v2 2/3] taprio: Allow users not to specify "flags" when changing schedules

[David Miller]

From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Tue, 28 Jan 2020 15:52:26 -0800

> When any offload mode is enabled, users had to specify the
> "flags" parameter when adding a new "admin" schedule.
> 
> This fix allows that parameter to be omitted when adding a new
> schedule.
> 
> This will make that we have one source of truth for 'flags'.
> 
> Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>

This will visibly change behavior for a feature in a released
kernel (v5.3 and later) and it means that newer tools will do
things that don't work in older kernels.

I think your opportunity to adjust these semantics, has therefore,
long passed.

Sorry.

Re: [PATCH net v2 2/3] taprio: Allow users not to specify "flags" when changing schedules

[Vladimir Oltean]

Hi David,

On Wed, 29 Jan 2020 at 12:14, David Miller <davem@davemloft.net> wrote:
>
> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Tue, 28 Jan 2020 15:52:26 -0800
>
> > When any offload mode is enabled, users had to specify the
> > "flags" parameter when adding a new "admin" schedule.
> >
> > This fix allows that parameter to be omitted when adding a new
> > schedule.
> >
> > This will make that we have one source of truth for 'flags'.
> >
> > Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
> > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> > Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> This will visibly change behavior for a feature in a released
> kernel (v5.3 and later) and it means that newer tools will do
> things that don't work in older kernels.
>
> I think your opportunity to adjust these semantics, has therefore,
> long passed.
>
> Sorry.

This is where the kernel-userspace policy escapes me a little bit.
How is this different from having a bug that would cause the "flags"
field to e.g. be ignored? Would the kernel policy make it impossible
for that bug to be fixed?
At some point, the 5.3 kernel will go EOL. When would be a good time
to make the "flags" optional on "tc qdisc replace", without concerns
about different behavior across versions?

Regards,
-Vladimir

Re: [PATCH net v2 2/3] taprio: Allow users not to specify "flags" when changing schedules

[David Miller]

From: Vladimir Oltean <olteanv@gmail.com>
Date: Wed, 29 Jan 2020 13:24:30 +0200

> At some point, the 5.3 kernel will go EOL. When would be a good time
> to make the "flags" optional on "tc qdisc replace", without concerns
> about different behavior across versions?

5.3, and 5.4, and... and how long do distros ship that kernel?

This is why it is absolutely critical to flesh out all public
facing interface concerns before the feature is merged into
the tree rather than later.

Re: [PATCH net v2 2/3] taprio: Allow users not to specify "flags" when changing schedules

[Vladimir Oltean]

On Wed, 29 Jan 2020 at 14:10, David Miller <davem@davemloft.net> wrote:
>
> From: Vladimir Oltean <olteanv@gmail.com>
> Date: Wed, 29 Jan 2020 13:24:30 +0200
>
> > At some point, the 5.3 kernel will go EOL. When would be a good time
> > to make the "flags" optional on "tc qdisc replace", without concerns
> > about different behavior across versions?
>
> 5.3, and 5.4, and... and how long do distros ship that kernel?
>
> This is why it is absolutely critical to flesh out all public
> facing interface concerns before the feature is merged into
> the tree rather than later.

So the answer to "when would be a good time" is "never"?

-Vladimir

Re: [PATCH net v2 2/3] taprio: Allow users not to specify "flags" when changing schedules

[Vinicius Costa Gomes]

David Miller <davem@davemloft.net> writes:

> From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> Date: Tue, 28 Jan 2020 15:52:26 -0800
>
>> When any offload mode is enabled, users had to specify the
>> "flags" parameter when adding a new "admin" schedule.
>> 
>> This fix allows that parameter to be omitted when adding a new
>> schedule.
>> 
>> This will make that we have one source of truth for 'flags'.
>> 
>> Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
>> Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
>
> This will visibly change behavior for a feature in a released
> kernel (v5.3 and later) and it means that newer tools will do
> things that don't work in older kernels.
>
> I think your opportunity to adjust these semantics, has therefore,
> long passed.

Understood. Another lesson learned.

I'll need to send another version then. This semantic change have
creeped up to the "rcu stall" fix.


Cheers,
--
Vinicius

[PATCH net v2 3/3] taprio: Fix still allowing changing the flags during runtime

[Vinicius Costa Gomes]

Because 'q->flags' starts as zero, and zero is a valid value, we
aren't able to detect the transition from zero to something else
during "runtime".

The solution is to initialize 'q->flags' with an invalid value, so we
can detect if 'q->flags' was set by the user or not.

This was causing the following RCU stall:

[ 1730.558249] rcu: INFO: rcu_preempt detected stalls on CPUs/tasks:
[ 1730.558258] rcu: 	  6-...0: (190 ticks this GP) idle=922/0/0x1 softirq=25580/25582 fqs=16250
[ 1730.558264] 		  (detected by 2, t=65002 jiffies, g=33017, q=81)
[ 1730.558269] Sending NMI from CPU 2 to CPUs 6:
[ 1730.559277] NMI backtrace for cpu 6
[ 1730.559277] CPU: 6 PID: 0 Comm: swapper/6 Tainted: G            E     5.5.0-rc6+ #35
[ 1730.559278] Hardware name: Gigabyte Technology Co., Ltd. Z390 AORUS ULTRA/Z390 AORUS ULTRA-CF, BIOS F7 03/14/2019
[ 1730.559278] RIP: 0010:__hrtimer_run_queues+0xe2/0x440
[ 1730.559278] Code: 48 8b 43 28 4c 89 ff 48 8b 75 c0 48 89 45 c8 e8 f4 bb 7c 00 0f 1f 44 00 00 65 8b 05 40 31 f0 68 89 c0 48 0f a3 05 3e 5c 25 01 <0f> 82 fc 01 00 00 48 8b 45 c8 48 89 df ff d0 89 45 c8 0f 1f 44 00
[ 1730.559279] RSP: 0018:ffff9970802d8f10 EFLAGS: 00000083
[ 1730.559279] RAX: 0000000000000006 RBX: ffff8b31645bff38 RCX: 0000000000000000
[ 1730.559280] RDX: 0000000000000000 RSI: ffffffff9710f2ec RDI: ffffffff978daf0e
[ 1730.559280] RBP: ffff9970802d8f68 R08: 0000000000000000 R09: 0000000000000000
[ 1730.559280] R10: 0000018336d7944e R11: 0000000000000001 R12: ffff8b316e39f9c0
[ 1730.559281] R13: ffff8b316e39f940 R14: ffff8b316e39f998 R15: ffff8b316e39f7c0
[ 1730.559281] FS:  0000000000000000(0000) GS:ffff8b316e380000(0000) knlGS:0000000000000000
[ 1730.559281] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 1730.559281] CR2: 00007f1105303760 CR3: 0000000227210005 CR4: 00000000003606e0
[ 1730.559282] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 1730.559282] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 1730.559282] Call Trace:
[ 1730.559282]  <IRQ>
[ 1730.559283]  ? taprio_dequeue_soft+0x2d0/0x2d0 [sch_taprio]
[ 1730.559283]  hrtimer_interrupt+0x104/0x220
[ 1730.559283]  ? irqtime_account_irq+0x34/0xa0
[ 1730.559283]  smp_apic_timer_interrupt+0x6d/0x230
[ 1730.559284]  apic_timer_interrupt+0xf/0x20
[ 1730.559284]  </IRQ>
[ 1730.559284] RIP: 0010:cpu_idle_poll+0x35/0x1a0
[ 1730.559285] Code: 88 82 ff 65 44 8b 25 12 7d 73 68 0f 1f 44 00 00 e8 90 c3 89 ff fb 65 48 8b 1c 25 c0 7e 01 00 48 8b 03 a8 08 74 0b eb 1c f3 90 <48> 8b 03 a8 08 75 13 8b 05 be a8 a8 00 85 c0 75 ed e8 75 48 84 ff
[ 1730.559285] RSP: 0018:ffff997080137ea8 EFLAGS: 00000202 ORIG_RAX: ffffffffffffff13
[ 1730.559285] RAX: 0000000000000001 RBX: ffff8b316bc3c580 RCX: 0000000000000000
[ 1730.559286] RDX: 0000000000000001 RSI: 000000002819aad9 RDI: ffffffff978da730
[ 1730.559286] RBP: ffff997080137ec0 R08: 0000018324a6d387 R09: 0000000000000000
[ 1730.559286] R10: 0000000000000400 R11: 0000000000000001 R12: 0000000000000006
[ 1730.559286] R13: ffff8b316bc3c580 R14: 0000000000000000 R15: 0000000000000000
[ 1730.559287]  ? cpu_idle_poll+0x20/0x1a0
[ 1730.559287]  ? cpu_idle_poll+0x20/0x1a0
[ 1730.559287]  do_idle+0x4d/0x1f0
[ 1730.559287]  ? complete+0x44/0x50
[ 1730.559288]  cpu_startup_entry+0x1b/0x20
[ 1730.559288]  start_secondary+0x142/0x180
[ 1730.559288]  secondary_startup_64+0xb6/0xc0
[ 1776.686313] nvme nvme0: I/O 96 QID 1 timeout, completion polled

Fixes: 4cfd5779bd6e ("taprio: Add support for txtime-assist mode")
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
 net/sched/sch_taprio.c | 47 ++++++++++++++++++++++++++++++------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index 110143fba114..7a13a144615b 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -31,6 +31,7 @@ static DEFINE_SPINLOCK(taprio_list_lock);
 
 #define TXTIME_ASSIST_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_TXTIME_ASSIST)
 #define FULL_OFFLOAD_IS_ENABLED(flags) ((flags) & TCA_TAPRIO_ATTR_FLAG_FULL_OFFLOAD)
+#define TAPRIO_FLAGS_INVALID U32_MAX
 
 struct sched_entry {
 	struct list_head list;
@@ -1367,6 +1368,33 @@ static int taprio_mqprio_cmp(const struct net_device *dev,
 	return 0;
 }
 
+static int taprio_new_flags(const struct nlattr *attr, u32 old,
+			    struct netlink_ext_ack *extack)
+{
+	u32 new = 0;
+
+	if (attr) {
+		new = nla_get_u32(attr);
+	} else if (old != TAPRIO_FLAGS_INVALID) {
+		/* If the user didn't specify new flags and the old
+		 * one is valid, use the old value.
+		 */
+		return old;
+	}
+
+	if (old != TAPRIO_FLAGS_INVALID && old != new) {
+		NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
+		return -ENOTSUPP;
+	}
+
+	if (!taprio_flags_valid(new)) {
+		NL_SET_ERR_MSG_MOD(extack, "Specified 'flags' are not valid");
+		return -EINVAL;
+	}
+
+	return new;
+}
+
 static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 			 struct netlink_ext_ack *extack)
 {
@@ -1375,7 +1403,6 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	struct taprio_sched *q = qdisc_priv(sch);
 	struct net_device *dev = qdisc_dev(sch);
 	struct tc_mqprio_qopt *mqprio = NULL;
-	u32 taprio_flags = 0;
 	unsigned long flags;
 	ktime_t start;
 	int i, err;
@@ -1388,19 +1415,12 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
 	if (tb[TCA_TAPRIO_ATTR_PRIOMAP])
 		mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
 
-	if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
-		taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
-
-		if (q->flags != 0 && q->flags != taprio_flags) {
-			NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
-			return -EOPNOTSUPP;
-		} else if (!taprio_flags_valid(taprio_flags)) {
-			NL_SET_ERR_MSG_MOD(extack, "Specified 'flags' are not valid");
-			return -EINVAL;
-		}
+	err = taprio_new_flags(tb[TCA_TAPRIO_ATTR_FLAGS],
+			       q->flags, extack);
+	if (err < 0)
+		return err;
 
-		q->flags = taprio_flags;
-	}
+	q->flags = err;
 
 	err = taprio_parse_mqprio_opt(dev, mqprio, extack, q->flags);
 	if (err < 0)
@@ -1597,6 +1617,7 @@ static int taprio_init(struct Qdisc *sch, struct nlattr *opt,
 	 * and get the valid one on taprio_change().
 	 */
 	q->clockid = -1;
+	q->flags = TAPRIO_FLAGS_INVALID;
 
 	spin_lock(&taprio_list_lock);
 	list_add(&q->taprio_list, &taprio_list);
-- 
2.25.0