[PATCH 0/4] mqprio fixup and simplify code.

I've been looking at the statistics code and stumbled upon something
that looked like bug. The following patches is an attempt to simplify
the code so that the else part can be removed. I'm not 100% sure
regarding the qlen usage in 4/4.

Sebastian

It looks like with the introduction of subqueus the statics broke.
Before the change `bstats' and `qstats' on stack was fed and later this
was copied over to struct gnet_dump.

After the change the `bstats' and `qstats' are only set to 0 and no
longer updated and that is then fed to gnet_dump. Additionally
qdisc->cpu_bstats and qdisc->cpu_qstats is destroeyd for global
stats. For per-CPU stats both __gnet_stats_copy_basic() and
__gnet_stats_copy_queue() add the values but for global stats the value
set and so the previous value is lost and only the last value from the
loop ends up in sch->[bq]stats.

Use the on-stack [bq]stats variables again and add the stats manually in
the global case.

Fixes: ce679e8df7ed2 ("net: sched: add support for TCQ_F_NOLOCK subqueues to sch_mqprio")
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/sched/sch_mqprio.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 8766ab5b87880..5eb3b1b7ae5e7 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c

@@ -529,22 +529,28 @@ static int mqprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 		for (i = tc.offset; i < tc.offset + tc.count; i++) {
 			struct netdev_queue *q = netdev_get_tx_queue(dev, i);
 			struct Qdisc *qdisc = rtnl_dereference(q->qdisc);
-			struct gnet_stats_basic_cpu __percpu *cpu_bstats = NULL;
-			struct gnet_stats_queue __percpu *cpu_qstats = NULL;
 
 			spin_lock_bh(qdisc_lock(qdisc));
-			if (qdisc_is_percpu_stats(qdisc)) {
-				cpu_bstats = qdisc->cpu_bstats;
-				cpu_qstats = qdisc->cpu_qstats;
-			}
 
-			qlen = qdisc_qlen_sum(qdisc);
-			__gnet_stats_copy_basic(NULL, &sch->bstats,
-						cpu_bstats, &qdisc->bstats);
-			__gnet_stats_copy_queue(&sch->qstats,
-						cpu_qstats,
-						&qdisc->qstats,
-						qlen);
+			if (qdisc_is_percpu_stats(qdisc)) {
+				qlen = qdisc_qlen_sum(qdisc);
+
+				__gnet_stats_copy_basic(NULL, &bstats,
+							qdisc->cpu_bstats,
+							&qdisc->bstats);
+				__gnet_stats_copy_queue(&qstats,
+							qdisc->cpu_qstats,
+							&qdisc->qstats,
+							qlen);
+			} else {
+				qlen		+= qdisc->q.qlen;
+				bstats.bytes	+= qdisc->bstats.bytes;
+				bstats.packets	+= qdisc->bstats.packets;
+				qstats.backlog	+= qdisc->qstats.backlog;
+				qstats.drops	+= qdisc->qstats.drops;
+				qstats.requeues	+= qdisc->qstats.requeues;
+				qstats.overlimits += qdisc->qstats.overlimits;
+			}
 			spin_unlock_bh(qdisc_lock(qdisc));
 		}
 

-- 
2.33.0

Based on review there are five users of __gnet_stats_copy_queue as of
today:
- qdisc_qstats_qlen_backlog(), gnet_stats_copy_queue(),
  memsets() bstats to zero, single invocation.

- mq_dump(), mqprio_dump(), mqprio_dump_class_stats(),
  memsets() bstats to zero, multiple invocation but does not use the
  function due to !qdisc_is_percpu_stats().

It will probably simplify in percpu stats case if the value would be
added and not just stored.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/core/gen_stats.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index e12e544a7ab0f..76dbae98c83fd 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c

@@ -312,14 +312,14 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats,
 	if (cpu) {
 		__gnet_stats_copy_queue_cpu(qstats, cpu);
 	} else {
-		qstats->qlen = q->qlen;
-		qstats->backlog = q->backlog;
-		qstats->drops = q->drops;
-		qstats->requeues = q->requeues;
-		qstats->overlimits = q->overlimits;
+		qstats->qlen += q->qlen;
+		qstats->backlog += q->backlog;
+		qstats->drops += q->drops;
+		qstats->requeues += q->requeues;
+		qstats->overlimits += q->overlimits;
 	}
 
-	qstats->qlen = qlen;
+	qstats->qlen += qlen;
 }
 EXPORT_SYMBOL(__gnet_stats_copy_queue);
 

-- 
2.33.0

Since day one __gnet_stats_copy_basic() always assigned the value to the
bstats argument overwriting the previous value.

Based on review there are five users of that function as of today:
- est_fetch_counters(), ___gnet_stats_copy_basic()
  memsets() bstats to zero, single invocation.

- mq_dump(), mqprio_dump(), mqprio_dump_class_stats()
  memsets() bstats to zero, multiple invocation but does not use the
  function due to !qdisc_is_percpu_stats().

It will probably simplify in percpu stats case if the value would be
added and not just stored.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 net/core/gen_stats.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index e491b083b3485..e12e544a7ab0f 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c

@@ -143,6 +143,8 @@ __gnet_stats_copy_basic(const seqcount_t *running,
 			struct gnet_stats_basic_packed *b)
 {
 	unsigned int seq;
+	__u64 bytes = 0;
+	__u64 packets = 0;
 
 	if (cpu) {
 		__gnet_stats_copy_basic_cpu(bstats, cpu);

@@ -151,9 +153,12 @@ __gnet_stats_copy_basic(const seqcount_t *running,
 	do {
 		if (running)
 			seq = read_seqcount_begin(running);
-		bstats->bytes = b->bytes;
-		bstats->packets = b->packets;
+		bytes = b->bytes;
+		packets = b->packets;
 	} while (running && read_seqcount_retry(running, seq));
+
+	bstats->bytes += bytes;
+	bstats->packets += packets;
 }
 EXPORT_SYMBOL(__gnet_stats_copy_basic);
 

-- 
2.33.0

__gnet_stats_copy_basic() and __gnet_stats_copy_queue() update the
statistics and don't overwritte them for both: global and per-CPU
statistics.

Simplify the code by removing the else case.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

Could someone please double check if the qdisc_qlen_sum() for
!qdisc_is_percpu_stats() is correct here? I would lke not break it again
in another way ;)

 net/sched/sch_mq.c     | 27 ++++++---------------
 net/sched/sch_mqprio.c | 55 +++++++++++++-----------------------------
 2 files changed, 25 insertions(+), 57 deletions(-)

diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index e79f1afe0cfd6..9aa9534f62ff5 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c

@@ -145,26 +145,15 @@ static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
 		qdisc = netdev_get_tx_queue(dev, ntx)->qdisc_sleeping;
 		spin_lock_bh(qdisc_lock(qdisc));
 
-		if (qdisc_is_percpu_stats(qdisc)) {
-			qlen = qdisc_qlen_sum(qdisc);
-			__gnet_stats_copy_basic(NULL, &sch->bstats,
-						qdisc->cpu_bstats,
-						&qdisc->bstats);
-			__gnet_stats_copy_queue(&sch->qstats,
-						qdisc->cpu_qstats,
-						&qdisc->qstats, qlen);
-			sch->q.qlen		+= qlen;
-		} else {
-			sch->q.qlen		+= qdisc->q.qlen;
-			sch->bstats.bytes	+= qdisc->bstats.bytes;
-			sch->bstats.packets	+= qdisc->bstats.packets;
-			sch->qstats.qlen	+= qdisc->qstats.qlen;
-			sch->qstats.backlog	+= qdisc->qstats.backlog;
-			sch->qstats.drops	+= qdisc->qstats.drops;
-			sch->qstats.requeues	+= qdisc->qstats.requeues;
-			sch->qstats.overlimits	+= qdisc->qstats.overlimits;
-		}
+		qlen = qdisc_qlen_sum(qdisc);
 
+		__gnet_stats_copy_basic(NULL, &sch->bstats,
+					qdisc->cpu_bstats,
+					&qdisc->bstats);
+		__gnet_stats_copy_queue(&sch->qstats,
+					qdisc->cpu_qstats,
+					&qdisc->qstats, qlen);
+		sch->q.qlen		+= qlen;
 		spin_unlock_bh(qdisc_lock(qdisc));
 	}
 

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 5eb3b1b7ae5e7..8e57c4a3545ee 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c

@@ -399,28 +399,18 @@ static int mqprio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	 * qdisc totals are added at end.
 	 */
 	for (ntx = 0; ntx < dev->num_tx_queues; ntx++) {
+		u32 qlen = qdisc_qlen_sum(qdisc);
+
 		qdisc = netdev_get_tx_queue(dev, ntx)->qdisc_sleeping;
 		spin_lock_bh(qdisc_lock(qdisc));
 
-		if (qdisc_is_percpu_stats(qdisc)) {
-			__u32 qlen = qdisc_qlen_sum(qdisc);
-
-			__gnet_stats_copy_basic(NULL, &sch->bstats,
-						qdisc->cpu_bstats,
-						&qdisc->bstats);
-			__gnet_stats_copy_queue(&sch->qstats,
-						qdisc->cpu_qstats,
-						&qdisc->qstats, qlen);
-			sch->q.qlen		+= qlen;
-		} else {
-			sch->q.qlen		+= qdisc->q.qlen;
-			sch->bstats.bytes	+= qdisc->bstats.bytes;
-			sch->bstats.packets	+= qdisc->bstats.packets;
-			sch->qstats.backlog	+= qdisc->qstats.backlog;
-			sch->qstats.drops	+= qdisc->qstats.drops;
-			sch->qstats.requeues	+= qdisc->qstats.requeues;
-			sch->qstats.overlimits	+= qdisc->qstats.overlimits;
-		}
+		__gnet_stats_copy_basic(NULL, &sch->bstats,
+					qdisc->cpu_bstats,
+					&qdisc->bstats);
+		__gnet_stats_copy_queue(&sch->qstats,
+					qdisc->cpu_qstats,
+					&qdisc->qstats, qlen);
+		sch->q.qlen		+= qlen;
 
 		spin_unlock_bh(qdisc_lock(qdisc));
 	}

@@ -532,25 +522,14 @@ static int mqprio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
 
 			spin_lock_bh(qdisc_lock(qdisc));
 
-			if (qdisc_is_percpu_stats(qdisc)) {
-				qlen = qdisc_qlen_sum(qdisc);
-
-				__gnet_stats_copy_basic(NULL, &bstats,
-							qdisc->cpu_bstats,
-							&qdisc->bstats);
-				__gnet_stats_copy_queue(&qstats,
-							qdisc->cpu_qstats,
-							&qdisc->qstats,
-							qlen);
-			} else {
-				qlen		+= qdisc->q.qlen;
-				bstats.bytes	+= qdisc->bstats.bytes;
-				bstats.packets	+= qdisc->bstats.packets;
-				qstats.backlog	+= qdisc->qstats.backlog;
-				qstats.drops	+= qdisc->qstats.drops;
-				qstats.requeues	+= qdisc->qstats.requeues;
-				qstats.overlimits += qdisc->qstats.overlimits;
-			}
+			qlen = qdisc_qlen_sum(qdisc);
+			__gnet_stats_copy_basic(NULL, &bstats,
+						qdisc->cpu_bstats,
+						&qdisc->bstats);
+			__gnet_stats_copy_queue(&qstats,
+						qdisc->cpu_qstats,
+						&qdisc->qstats,
+						qlen);
 			spin_unlock_bh(qdisc_lock(qdisc));
 		}
 

-- 
2.33.0

On Thu,  7 Oct 2021 19:49:57 +0200 Sebastian Andrzej Siewior wrote:

It looks like with the introduction of subqueus the statics broke.
Before the change `bstats' and `qstats' on stack was fed and later this
was copied over to struct gnet_dump.

After the change the `bstats' and `qstats' are only set to 0 and no
longer updated and that is then fed to gnet_dump. Additionally
qdisc->cpu_bstats and qdisc->cpu_qstats is destroeyd for global
stats. For per-CPU stats both __gnet_stats_copy_basic() and
__gnet_stats_copy_queue() add the values but for global stats the value
set and so the previous value is lost and only the last value from the
loop ends up in sch->[bq]stats.

Use the on-stack [bq]stats variables again and add the stats manually in
the global case.

Fixes: ce679e8df7ed2 ("net: sched: add support for TCQ_F_NOLOCK subqueues to sch_mqprio")
Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

Applied after significant massaging of the commit message.

Please repost the cleanup in a week (once net gets merged 
into net-next).

Thanks!

On Thu,  7 Oct 2021 19:49:58 +0200 Sebastian Andrzej Siewior wrote:

+	__u64 bytes = 0;
+	__u64 packets = 0;

u64 is fine, no need to perpetuate the over-use of the user space types.

On Thu,  7 Oct 2021 19:49:59 +0200 Sebastian Andrzej Siewior wrote:

Looks like qlen is going to be added twice for the non-per-cpu case?

On 2021-10-08 16:38:51 [-0700], Jakub Kicinski wrote:

On Thu,  7 Oct 2021 19:49:59 +0200 Sebastian Andrzej Siewior wrote:

quoted

--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c

@@ -312,14 +312,14 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats,
 	if (cpu) {
 		__gnet_stats_copy_queue_cpu(qstats, cpu);
 	} else {
-		qstats->qlen = q->qlen;
-		qstats->backlog = q->backlog;
-		qstats->drops = q->drops;
-		qstats->requeues = q->requeues;
-		qstats->overlimits = q->overlimits;
+		qstats->qlen += q->qlen;
+		qstats->backlog += q->backlog;
+		qstats->drops += q->drops;
+		qstats->requeues += q->requeues;
+		qstats->overlimits += q->overlimits;
 	}
 
-	qstats->qlen = qlen;
+	qstats->qlen += qlen;

Looks like qlen is going to be added twice for the non-per-cpu case?

Hmmm. Let me dive into unknown territory…

Yes, it does. Also in the pcpu-case it does not look very straight what
goes on:
	qlen = qdisc_qlen_sum(qdisc); /* sum of per-CPU cpu_qstat.qlen */
	__gnet_stats_copy_queue() /* sch.qstats.qlen = qlen */
	sch->q.qlen     += qlen;

sch->qstats.qlen is qdisc_qlen_sum() of the qdisc from last for loop.
sch->q.qlen contains qdisc_qlen_sum() of all qdiscs from the for loop. I
doubt this is intended.
My guess is that a sum (like in the !pcpu case is intended).

We have
- qdisc.q.qlen qdisc_skb_head::qlen
- qdisc.qstats.qlen aka gnet_stats_queue::qlen.

qdisc_skb_head::qlen is incremented if skbs are added to the qdisc which
are about to be sent. Usually the skb is added to the qdisc_skb_head but
some scheduling classes have their own queues (say sch_sfq) and probably
increment this field just to keep things like qdisc_is_empty() working.

gnet_stats_queue::qlen is the number of skbs either in Qdisc::skb_bad_txq or
Qdisc::gso_skb.

qdisc_update_stats_at_enqueue() increments in percpu case the per-CPU
gnet_stats_queue::qlen but in the !percpu case it increments
qdisc_skb_head::qlen. But there is the qstats member which is also if
type gnet_stats_queue. For backlog, the gnet_stats_queue struct is
always used, either per-CPU or the global one. Not here. Not sure if it
on purpose or not. The same true for qdisc_enqueue_skb_bad_txq() and
dev_requeue_skb() plus their counterpart so it consistent.

This brings me to __gnet_stats_copy_queue(). In the percpu case,
caller's gnet_stats_queue::qlen is set to 0 multiple times. And then
caller's qlen argument is assigned to the qlen member. In !percpu
case it copies gnet_stats_queue::qlen. But I don't see an
increment/decrement of that field here so it has to be zero. Also at the
end it sets the field to caller's qlen. So…

This probably works because callers of __gnet_stats_copy_queue() invoke
qdisc_qlen_sum() which returns the sum of:
  qstats::qlen (0) + 
     per-CPU qstats::qlen || !per-CPU qdisc_skb_head::qlen

So in the end the caller figures out qlen is and passes it as a
argument. The copy process of qlen ist decoy.

But then there is gnet_stats_copy_queue().
sch_fq_codel modifies qdisc_skb_head::qlen on fq_codel_enqueue()/
fq_codel_dequeue(). But fq_codel_dump_class_stats() returns statistics
for a specific flow so it counts the number of skb which is less than
the value qdisc_skb_head.qlen if multiple flows are configured.

So I think I could remove the qlen argument from
__gnet_stats_copy_queue() and just copy what is there. And make a real
copy, not just summing all qlen into qdisc_skb_head.qlen and leaving
gnet_stats_queue.qlen with the last value.

Sebastian

On Thu, Oct 7, 2021 at 10:51 AM Sebastian Andrzej Siewior
[off-list ref] wrote:

Since day one __gnet_stats_copy_basic() always assigned the value to the
bstats argument overwriting the previous value.

Based on review there are five users of that function as of today:
- est_fetch_counters(), ___gnet_stats_copy_basic()
  memsets() bstats to zero, single invocation.

- mq_dump(), mqprio_dump(), mqprio_dump_class_stats()
  memsets() bstats to zero, multiple invocation but does not use the
  function due to !qdisc_is_percpu_stats().

It will probably simplify in percpu stats case if the value would be
added and not just stored

You at least need to rename it before doing so, otherwise "copy"
would be too confusing.