[PATCH net-next 0/3] net/sched: changes around qdisc_qstats_qlen_backlog()

[PATCH net-next 0/3] net/sched: changes around qdisc_qstats_qlen_backlog()

[Eric Dumazet]

First patch makes clear qdisc_qstats_qlen_backlog() runs locklessly.

Second patch changes ioam6 to not acquire qdisc spinlock anymore.

Third pach adds missing READ_ONCE()/WRITE_ONCE() annotations in hfsc.

htb still needs fixes, I will take care of them separately.

Eric Dumazet (3):
  net/sched: qdisc_qstats_qlen_backlog() runs locklessly
  net: ioam6: no longer acquire qdisc spinlock while calling
    qdisc_qstats_qlen_backlog()
  net/sched: sch_hfsc: annotate data-races in hfsc_dump_class_stats()

 include/net/sch_generic.h |  6 +++---
 net/ipv6/ioam6.c          |  4 +---
 net/sched/sch_hfsc.c      | 22 +++++++++++-----------
 3 files changed, 15 insertions(+), 17 deletions(-)

-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 1/3] net/sched: qdisc_qstats_qlen_backlog() runs locklessly

[Eric Dumazet]

qdisc_qstats_qlen_backlog() can be called without qdisc spinlock being held.

Use qdisc_qlen_lockless() instead of qdisc_qlen().

Add a const qualifier to its first parameter (struct Qdisc *sch).

Fixes: edb09eb17ed8 ("net: sched: do not acquire qdisc spinlock in qdisc/class stats dump")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/sch_generic.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 9e6ed92729d282642e17a72cc578f25e1d22e4d9..d0ca932b1871c086f0ba7aad11241742bba0a9d8 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1070,13 +1070,13 @@ static inline int qdisc_qstats_copy(struct gnet_dump *d, const struct Qdisc *sch
 	return gnet_stats_copy_queue(d, sch->cpu_qstats, &sch->qstats, qlen);
 }
 
-static inline void qdisc_qstats_qlen_backlog(struct Qdisc *sch,  __u32 *qlen,
-					     __u32 *backlog)
+static inline void qdisc_qstats_qlen_backlog(const struct Qdisc *sch,
+					     u32 *qlen, u32 *backlog)
 {
 	struct gnet_stats_queue qstats = { 0 };
 
 	gnet_stats_add_queue(&qstats, sch->cpu_qstats, &sch->qstats);
-	*qlen = qstats.qlen + qdisc_qlen(sch);
+	*qlen = qstats.qlen + qdisc_qlen_lockless(sch);
 	*backlog = qstats.backlog;
 }
 
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 2/3] net: ioam6: no longer acquire qdisc spinlock while calling qdisc_qstats_qlen_backlog()

[Eric Dumazet]

Recent changes in qdisc_qstats_qlen_backlog() and friends make it safe
to use locklessly.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/ioam6.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/net/ipv6/ioam6.c b/net/ipv6/ioam6.c
index e963a71858a78c0e53db31d98ab317bbf64ea2a2..943861a5581ee6e9334c29b9daf888a9d2bf1de0 100644
--- a/net/ipv6/ioam6.c
+++ b/net/ipv6/ioam6.c
@@ -800,7 +800,7 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
 	/* queue depth */
 	if (trace->type.bit6) {
 		struct netdev_queue *queue;
-		struct Qdisc *qdisc;
+		const struct Qdisc *qdisc;
 		__u32 qlen, backlog;
 
 		if (dev->flags & IFF_LOOPBACK ||
@@ -810,9 +810,7 @@ static void __ioam6_fill_trace_data(struct sk_buff *skb,
 			queue = skb_get_tx_queue(dev, skb);
 			qdisc = rcu_dereference(queue->qdisc);
 
-			spin_lock_bh(qdisc_lock(qdisc));
 			qdisc_qstats_qlen_backlog(qdisc, &qlen, &backlog);
-			spin_unlock_bh(qdisc_lock(qdisc));
 
 			*(__be32 *)data = cpu_to_be32(backlog);
 		}
-- 
2.54.0.563.g4f69b47b94-goog

[PATCH net-next 3/3] net/sched: sch_hfsc: annotate data-races in hfsc_dump_class_stats()

[Eric Dumazet]

hfsc_dump_class_stats() runs without qdisc spinlock being held.

Add READ_ONCE()/WRITE_ONCE() annotations around:

- cl->level
- cl->cl_vtperiod
- cl->cl_total
- cl->cl_cumul

Fixes: edb09eb17ed8 ("net: sched: do not acquire qdisc spinlock in qdisc/class stats dump")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/sched/sch_hfsc.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/net/sched/sch_hfsc.c b/net/sched/sch_hfsc.c
index 59409ee2d2ff9279d7439b744030c0e845386de0..c06d2761a1fb9dbc711b18d61cfa8c28a7de0bd8 100644
--- a/net/sched/sch_hfsc.c
+++ b/net/sched/sch_hfsc.c
@@ -715,7 +715,7 @@ init_vf(struct hfsc_class *cl, unsigned int len)
 			rtsc_min(&cl->cl_virtual, &cl->cl_fsc, cl->cl_vt, cl->cl_total);
 			cl->cl_vtadj = 0;
 
-			cl->cl_vtperiod++;  /* increment vt period */
+			WRITE_ONCE(cl->cl_vtperiod, cl->cl_vtperiod + 1);  /* increment vt period */
 			cl->cl_parentperiod = cl->cl_parent->cl_vtperiod;
 			if (cl->cl_parent->cl_nactive == 0)
 				cl->cl_parentperiod++;
@@ -757,7 +757,7 @@ update_vf(struct hfsc_class *cl, unsigned int len, u64 cur_time)
 		go_passive = 1;
 
 	for (; cl->cl_parent != NULL; cl = cl->cl_parent) {
-		cl->cl_total += len;
+		WRITE_ONCE(cl->cl_total, cl->cl_total + len);
 
 		if (!(cl->cl_flags & HFSC_FSC) || cl->cl_nactive == 0)
 			continue;
@@ -847,7 +847,7 @@ hfsc_adjust_levels(struct hfsc_class *cl)
 			if (p->level >= level)
 				level = p->level + 1;
 		}
-		cl->level = level;
+		WRITE_ONCE(cl->level, level);
 	} while ((cl = cl->cl_parent) != NULL);
 }
 
@@ -1338,10 +1338,10 @@ hfsc_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	__u32 qlen;
 
 	qdisc_qstats_qlen_backlog(cl->qdisc, &qlen, &cl->qstats.backlog);
-	xstats.level   = cl->level;
-	xstats.period  = cl->cl_vtperiod;
-	xstats.work    = cl->cl_total;
-	xstats.rtwork  = cl->cl_cumul;
+	xstats.level   = READ_ONCE(cl->level);
+	xstats.period  = READ_ONCE(cl->cl_vtperiod);
+	xstats.work    = READ_ONCE(cl->cl_total);
+	xstats.rtwork  = READ_ONCE(cl->cl_cumul);
 
 	if (gnet_stats_copy_basic(d, NULL, &cl->bstats, true) < 0 ||
 	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
@@ -1452,15 +1452,15 @@ hfsc_change_qdisc(struct Qdisc *sch, struct nlattr *opt,
 static void
 hfsc_reset_class(struct hfsc_class *cl)
 {
-	cl->cl_total        = 0;
-	cl->cl_cumul        = 0;
+	WRITE_ONCE(cl->cl_total, 0);
+	WRITE_ONCE(cl->cl_cumul, 0);
 	cl->cl_d            = 0;
 	cl->cl_e            = 0;
 	cl->cl_vt           = 0;
 	cl->cl_vtadj        = 0;
 	cl->cl_cvtmin       = 0;
 	cl->cl_cvtoff       = 0;
-	cl->cl_vtperiod     = 0;
+	WRITE_ONCE(cl->cl_vtperiod, 0);
 	cl->cl_parentperiod = 0;
 	cl->cl_f            = 0;
 	cl->cl_myf          = 0;
@@ -1626,7 +1626,7 @@ hfsc_dequeue(struct Qdisc *sch)
 	bstats_update(&cl->bstats, skb);
 	update_vf(cl, qdisc_pkt_len(skb), cur_time);
 	if (realtime)
-		cl->cl_cumul += qdisc_pkt_len(skb);
+		WRITE_ONCE(cl->cl_cumul, cl->cl_cumul + qdisc_pkt_len(skb));
 
 	if (cl->cl_flags & HFSC_RSC) {
 		if (cl->qdisc->q.qlen != 0) {
-- 
2.54.0.563.g4f69b47b94-goog

Re: [PATCH net-next 0/3] net/sched: changes around qdisc_qstats_qlen_backlog()

[Toke Høiland-Jørgensen]

Eric Dumazet <edumazet@google.com> writes:

> First patch makes clear qdisc_qstats_qlen_backlog() runs locklessly.
>
> Second patch changes ioam6 to not acquire qdisc spinlock anymore.
>
> Third pach adds missing READ_ONCE()/WRITE_ONCE() annotations in hfsc.
>
> htb still needs fixes, I will take care of them separately.
>
> Eric Dumazet (3):
>   net/sched: qdisc_qstats_qlen_backlog() runs locklessly
>   net: ioam6: no longer acquire qdisc spinlock while calling
>     qdisc_qstats_qlen_backlog()
>   net/sched: sch_hfsc: annotate data-races in hfsc_dump_class_stats()
>
>  include/net/sch_generic.h |  6 +++---
>  net/ipv6/ioam6.c          |  4 +---
>  net/sched/sch_hfsc.c      | 22 +++++++++++-----------
>  3 files changed, 15 insertions(+), 17 deletions(-)

For the series:

Reviewed-by: Toke Høiland-Jørgensen <toke@toke.dk>