* [PATCH net v1 0/3] taprio: Some fixes
@ 2020-01-25 0:53 Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes Vinicius Costa Gomes
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Vinicius Costa Gomes @ 2020-01-25 0:53 UTC (permalink / raw)
To: netdev
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem,
vladimir.oltean, po.liu
Hi,
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: Changing the "flags" parameter during "runtime" was never
supposed to work, but if the user didn't set it at the beginning, we
were still allowing it.
Patch 3/3: Improves the code, so a similar situation fixed by 2/3 is
harder to happen again, and has the intended side effect that users do
not need to specify the "flags" argument when changing schedules when
some kind of offloading is enabled.
Cheers,
--
Vinicius
Vinicius Costa Gomes (3):
taprio: Fix enabling offload with wrong number of traffic classes
taprio: Fix still allowing changing the flags during runtime
taprio: Allow users not to specify "flags" when changing schedules
net/sched/sch_taprio.c | 45 +++++++++++++++++++++---------------------
1 file changed, 22 insertions(+), 23 deletions(-)
--
2.25.0
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes
2020-01-25 0:53 [PATCH net v1 0/3] taprio: Some fixes Vinicius Costa Gomes
@ 2020-01-25 0:53 ` Vinicius Costa Gomes
2020-01-25 16:53 ` Vladimir Oltean
2020-01-27 19:02 ` Andre Guedes
2020-01-25 0:53 ` [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules Vinicius Costa Gomes
2 siblings, 2 replies; 10+ messages in thread
From: Vinicius Costa Gomes @ 2020-01-25 0:53 UTC (permalink / raw)
To: netdev
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem,
vladimir.oltean, po.liu
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>
---
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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime
2020-01-25 0:53 [PATCH net v1 0/3] taprio: Some fixes Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes Vinicius Costa Gomes
@ 2020-01-25 0:53 ` Vinicius Costa Gomes
2020-01-25 16:44 ` Vladimir Oltean
2020-01-25 0:53 ` [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules Vinicius Costa Gomes
2 siblings, 1 reply; 10+ messages in thread
From: Vinicius Costa Gomes @ 2020-01-25 0:53 UTC (permalink / raw)
To: netdev
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem,
vladimir.oltean, po.liu
The check for 'q->flags' being zero allows that, if the taprio
instance was created with no 'flags' specified, for 'flags' to be
changed. By design this should not be permitted, as it might require
changing the enqueue()/dequeue() functions of taprio, which might
already be running, and if the hrtimer is enabled or not.
The solution is to not support changing flags in any way during
"runtime".
As a result of this problem the following RCU stall can be observed:
[ 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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index ad0dadcfcdba..dfbecb9ee2f4 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1391,7 +1391,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
- if (q->flags != 0 && q->flags != taprio_flags) {
+ if (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)) {
--
2.25.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules
2020-01-25 0:53 [PATCH net v1 0/3] taprio: Some fixes Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime Vinicius Costa Gomes
@ 2020-01-25 0:53 ` Vinicius Costa Gomes
2020-01-25 16:52 ` Vladimir Oltean
2 siblings, 1 reply; 10+ messages in thread
From: Vinicius Costa Gomes @ 2020-01-25 0:53 UTC (permalink / raw)
To: netdev
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem,
vladimir.oltean, po.liu
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.
Also move 'taprio_flags' to a smaller scope, so we reduce the region
where we have two variables with the similar concepts ('taprio_flags'
and 'q->flags').
Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
---
net/sched/sch_taprio.c | 17 ++++++++---------
1 file changed, 8 insertions(+), 9 deletions(-)
diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
index dfbecb9ee2f4..65357e2ba04e 100644
--- a/net/sched/sch_taprio.c
+++ b/net/sched/sch_taprio.c
@@ -1375,7 +1375,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;
@@ -1389,7 +1388,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
- taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
+ u32 taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
if (q->flags != taprio_flags) {
NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
@@ -1402,7 +1401,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 +1456,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 +1476,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 +1500,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 +1527,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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime
2020-01-25 0:53 ` [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime Vinicius Costa Gomes
@ 2020-01-25 16:44 ` Vladimir Oltean
2020-01-25 20:25 ` Vinicius Costa Gomes
0 siblings, 1 reply; 10+ messages in thread
From: Vladimir Oltean @ 2020-01-25 16:44 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Vladimir Oltean, Po Liu
Hi Vinicius,
On Sat, 25 Jan 2020 at 02:52, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> The check for 'q->flags' being zero allows that, if the taprio
> instance was created with no 'flags' specified, for 'flags' to be
> changed. By design this should not be permitted, as it might require
> changing the enqueue()/dequeue() functions of taprio, which might
> already be running, and if the hrtimer is enabled or not.
>
> The solution is to not support changing flags in any way during
> "runtime".
>
> As a result of this problem the following RCU stall can be observed:
>
> [ 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index ad0dadcfcdba..dfbecb9ee2f4 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1391,7 +1391,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
> if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
> taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
>
> - if (q->flags != 0 && q->flags != taprio_flags) {
> + if (q->flags != taprio_flags) {
You can't quite do this, since it now breaks plain 'tc qdisc add'
behavior. Can you initialize q->flags to -1 in taprio_init, just below
q->clockid, and keep the "q->flags != -1" check here? You might also
need to validate the taprio_flags to be a positive value.
> NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
> return -EOPNOTSUPP;
> } else if (!taprio_flags_valid(taprio_flags)) {
> --
> 2.25.0
>
Regards,
-Vladimir
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules
2020-01-25 0:53 ` [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules Vinicius Costa Gomes
@ 2020-01-25 16:52 ` Vladimir Oltean
0 siblings, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2020-01-25 16:52 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Vladimir Oltean, Po Liu
On Sat, 25 Jan 2020 at 02:52, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> 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.
>
> Also move 'taprio_flags' to a smaller scope, so we reduce the region
> where we have two variables with the similar concepts ('taprio_flags'
> and 'q->flags').
>
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
> ---
Acked-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> net/sched/sch_taprio.c | 17 ++++++++---------
> 1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/net/sched/sch_taprio.c b/net/sched/sch_taprio.c
> index dfbecb9ee2f4..65357e2ba04e 100644
> --- a/net/sched/sch_taprio.c
> +++ b/net/sched/sch_taprio.c
> @@ -1375,7 +1375,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;
> @@ -1389,7 +1388,7 @@ static int taprio_change(struct Qdisc *sch, struct nlattr *opt,
> mqprio = nla_data(tb[TCA_TAPRIO_ATTR_PRIOMAP]);
>
> if (tb[TCA_TAPRIO_ATTR_FLAGS]) {
> - taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
> + u32 taprio_flags = nla_get_u32(tb[TCA_TAPRIO_ATTR_FLAGS]);
>
> if (q->flags != taprio_flags) {
> NL_SET_ERR_MSG_MOD(extack, "Changing 'flags' of a running schedule is not supported");
> @@ -1402,7 +1401,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 +1456,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 +1476,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 +1500,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 +1527,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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes
2020-01-25 0:53 ` [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes Vinicius Costa Gomes
@ 2020-01-25 16:53 ` Vladimir Oltean
2020-01-27 19:02 ` Andre Guedes
1 sibling, 0 replies; 10+ messages in thread
From: Vladimir Oltean @ 2020-01-25 16:53 UTC (permalink / raw)
To: Vinicius Costa Gomes
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Vladimir Oltean, Po Liu
On Sat, 25 Jan 2020 at 02:53, Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> 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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime
2020-01-25 16:44 ` Vladimir Oltean
@ 2020-01-25 20:25 ` Vinicius Costa Gomes
0 siblings, 0 replies; 10+ messages in thread
From: Vinicius Costa Gomes @ 2020-01-25 20:25 UTC (permalink / raw)
To: Vladimir Oltean
Cc: netdev, Jamal Hadi Salim, Cong Wang, Jiri Pirko, David S. Miller,
Vladimir Oltean, Po Liu
Hi,
Vladimir Oltean <olteanv@gmail.com> writes:
>
> You can't quite do this, since it now breaks plain 'tc qdisc add'
> behavior. Can you initialize q->flags to -1 in taprio_init, just below
> q->clockid, and keep the "q->flags != -1" check here? You might also
> need to validate the taprio_flags to be a positive value.
>
Ugh, thanks for pointing this out. Will re-spin the series fixing this.
Another lesson on not sending patches last thing on a friday.
Cheers,
--
Vinicius
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes
2020-01-25 0:53 ` [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes Vinicius Costa Gomes
2020-01-25 16:53 ` Vladimir Oltean
@ 2020-01-27 19:02 ` Andre Guedes
2020-01-27 19:18 ` Vinicius Costa Gomes
1 sibling, 1 reply; 10+ messages in thread
From: Andre Guedes @ 2020-01-27 19:02 UTC (permalink / raw)
To: Vinicius Costa Gomes, netdev
Cc: Vinicius Costa Gomes, jhs, xiyou.wangcong, jiri, davem,
vladimir.oltean, po.liu
Hi Vinicius,
Quoting Vinicius Costa Gomes (2020-01-24 16:53:18)
> 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
If something goes wrong later within this function (e.g.
taprio_enable_offload() returns error), don't we want to roll back these
changes to the netdev object?
- Andre
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes
2020-01-27 19:02 ` Andre Guedes
@ 2020-01-27 19:18 ` Vinicius Costa Gomes
0 siblings, 0 replies; 10+ messages in thread
From: Vinicius Costa Gomes @ 2020-01-27 19:18 UTC (permalink / raw)
To: Andre Guedes, netdev
Cc: jhs, xiyou.wangcong, jiri, davem, vladimir.oltean, po.liu
Hi Andre,
Andre Guedes <andre.guedes@linux.intel.com> writes:
> Hi Vinicius,
>
> Quoting Vinicius Costa Gomes (2020-01-24 16:53:18)
>> 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
>
> If something goes wrong later within this function (e.g.
> taprio_enable_offload() returns error), don't we want to roll back these
> changes to the netdev object?
If something goes wrong, and change() returns an error, taprio_destroy()
is called, and the changes are undone.
Cheers,
--
Vinicius
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-27 19:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-25 0:53 [PATCH net v1 0/3] taprio: Some fixes Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 1/3] taprio: Fix enabling offload with wrong number of traffic classes Vinicius Costa Gomes
2020-01-25 16:53 ` Vladimir Oltean
2020-01-27 19:02 ` Andre Guedes
2020-01-27 19:18 ` Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 2/3] taprio: Fix still allowing changing the flags during runtime Vinicius Costa Gomes
2020-01-25 16:44 ` Vladimir Oltean
2020-01-25 20:25 ` Vinicius Costa Gomes
2020-01-25 0:53 ` [PATCH net v1 3/3] taprio: Allow users not to specify "flags" when changing schedules Vinicius Costa Gomes
2020-01-25 16:52 ` Vladimir Oltean
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.