[PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting

[PATCH net-next 0/5] net: sched: move back qlen to per CPU accounting

[Paolo Abeni]

The commit 46b1c18f9deb ("net: sched: put back q.qlen into a single location")
introduced some measurable regression in the contended scenarios for
lock qdisc.

As Eric suggested we could replace q.qlen access with calls to qdisc_is_empty()
in the datapath and revert the above commit. The TC subsystem updates 
qdisc->is_empty in a somewhat loose way: notably is_empty is set only when
the qdisc dequeue() calls return a NULL ptr. That is, the invocation after
the last packet is dequeued.

The above is good enough for BYPASS implementation - the only downside is that
we end up avoiding the optimization for a very small time-frame - but will
break hard things when internal structures consistency for classful qdisc
relies on child qdisc_is_empty().

A more strict is_empty update adds a relevant complexity to its life-cycle, so
this series takes a different approach: we allow lockless qdisc to switch from
per CPU accounting to global stats accounting when the NOLOCK bit is cleared.
Since most pieces of infrastructure are already in place, this requires very
little changes to the pfifo_fast qdisc, and any later NOLOCK qdisc can hook
there with little effort - no need to maintain two different implementations.

The first 2 patches removes direct qlen access from non core TC code, the 3rd
and 4th patches place and use the infrastructure to allow stats account
switching and the 5th patch is the actual revert.

Paolo Abeni (5):
  net: caif: avoid using qdisc_qlen()
  net: sched: prefer qdisc_is_empty() over direct qlen access
  net: sched: always do stats accounting according to TCQ_F_CPUSTATS
  net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too
  Revert: "net: sched: put back q.qlen into a single location"

 include/net/sch_generic.h | 82 ++++++++++++++++++++++++++++-----------
 net/caif/caif_dev.c       | 12 ++++--
 net/core/gen_stats.c      |  2 +
 net/sched/sch_api.c       | 15 ++++++-
 net/sched/sch_generic.c   | 67 +++++++++++---------------------
 5 files changed, 106 insertions(+), 72 deletions(-)

-- 
2.20.1

[PATCH net-next 1/5] net: caif: avoid using qdisc_qlen()

[Paolo Abeni]

Such helper does not cope correctly with NOLOCK qdiscs.
In the following patches we will move back qlen to per CPU
values for such qdiscs, so qdisc_qlen_sum() is not an option,
too.
Instead, use qlen only for lock qdiscs, and always set
flow off for NOLOCK qdiscs with a not empty tx queue.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/caif/caif_dev.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/net/caif/caif_dev.c b/net/caif/caif_dev.c
index 711d7156efd8..6c6e01963aac 100644
--- a/net/caif/caif_dev.c
+++ b/net/caif/caif_dev.c
@@ -186,15 +186,19 @@ static int transmit(struct cflayer *layer, struct cfpkt *pkt)
 		goto noxoff;
 
 	if (likely(!netif_queue_stopped(caifd->netdev))) {
+		struct Qdisc *sch;
+
 		/* If we run with a TX queue, check if the queue is too long*/
 		txq = netdev_get_tx_queue(skb->dev, 0);
-		qlen = qdisc_qlen(rcu_dereference_bh(txq->qdisc));
-
-		if (likely(qlen == 0))
+		sch = rcu_dereference_bh(txq->qdisc);
+		if (likely(qdisc_is_empty(sch)))
 			goto noxoff;
 
+		/* can check for explicit qdisc len value only !NOLOCK,
+		 * always set flow off otherwise
+		 */
 		high = (caifd->netdev->tx_queue_len * q_high) / 100;
-		if (likely(qlen < high))
+		if (!(sch->flags & TCQ_F_NOLOCK) && likely(sch->q.qlen < high))
 			goto noxoff;
 	}
 
-- 
2.20.1

[PATCH net-next 2/5] net: sched: prefer qdisc_is_empty() over direct qlen access

[Paolo Abeni]

When checking for root qdisc queue length, do not access directly q.qlen.
In the following patches we will move back qlen accounting to per CPU
values for NOLOCK qdiscs.

Instead, prefer the qdisc_is_empty() helper usage.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sch_generic.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 83364fac7cf1..19206d669655 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -747,7 +747,7 @@ static inline bool qdisc_all_tx_empty(const struct net_device *dev)
 		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
 		const struct Qdisc *q = rcu_dereference(txq->qdisc);
 
-		if (q->q.qlen) {
+		if (!qdisc_is_empty(q)) {
 			rcu_read_unlock();
 			return false;
 		}
-- 
2.20.1

[PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS

[Paolo Abeni]

The core sched implementation checks independently for NOLOCK flag
to acquire/release the root spin lock and for qdisc_is_percpu_stats()
to account per CPU values in many places.

This change update the last few places checking the TCQ_F_NOLOCK to
do per CPU stats accounting according to qdisc_is_percpu_stats()
value.

The above allows to clean dev_requeue_skb() implementation a bit
and makes stats update always consistent with a single flag.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sch_generic.h | 25 ++++++++++++--------
 net/sched/sch_generic.c   | 50 +++++++++++++--------------------------
 2 files changed, 32 insertions(+), 43 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 19206d669655..249817d97bf7 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -146,13 +146,6 @@ static inline bool qdisc_is_running(struct Qdisc *qdisc)
 	return (raw_read_seqcount(&qdisc->running) & 1) ? true : false;
 }
 
-static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
-{
-	if (qdisc->flags & TCQ_F_NOLOCK)
-		return qdisc->empty;
-	return !qdisc->q.qlen;
-}
-
 static inline bool qdisc_run_begin(struct Qdisc *qdisc)
 {
 	if (qdisc->flags & TCQ_F_NOLOCK) {
@@ -490,7 +483,7 @@ static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
 {
 	u32 qlen = q->qstats.qlen;
 
-	if (q->flags & TCQ_F_NOLOCK)
+	if (q->flags & TCQ_F_CPUSTATS)
 		qlen += atomic_read(&q->q.atomic_qlen);
 	else
 		qlen += q->q.qlen;
@@ -822,6 +815,13 @@ static inline bool qdisc_is_percpu_stats(const struct Qdisc *q)
 	return q->flags & TCQ_F_CPUSTATS;
 }
 
+static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
+{
+	if (qdisc_is_percpu_stats(qdisc))
+		return qdisc->empty;
+	return !qdisc->q.qlen;
+}
+
 static inline void _bstats_update(struct gnet_stats_basic_packed *bstats,
 				  __u64 bytes, __u32 packets)
 {
@@ -1113,8 +1113,13 @@ static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
 
 	if (skb) {
 		skb = __skb_dequeue(&sch->gso_skb);
-		qdisc_qstats_backlog_dec(sch, skb);
-		sch->q.qlen--;
+		if (qdisc_is_percpu_stats(sch)) {
+			qdisc_qstats_cpu_backlog_dec(sch, skb);
+			qdisc_qstats_atomic_qlen_dec(sch);
+		} else {
+			qdisc_qstats_backlog_dec(sch, skb);
+			sch->q.qlen--;
+		}
 	} else {
 		skb = sch->dequeue(sch);
 	}
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 81356ef38d1d..ddff2952be87 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -118,52 +118,36 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
 		spin_unlock(lock);
 }
 
-static inline int __dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
+static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 {
-	while (skb) {
-		struct sk_buff *next = skb->next;
-
-		__skb_queue_tail(&q->gso_skb, skb);
-		q->qstats.requeues++;
-		qdisc_qstats_backlog_inc(q, skb);
-		q->q.qlen++;	/* it's still part of the queue */
+	spinlock_t *lock = NULL;
 
-		skb = next;
+	if (q->flags & TCQ_F_NOLOCK) {
+		lock = qdisc_lock(q);
+		spin_lock(lock);
 	}
-	__netif_schedule(q);
-
-	return 0;
-}
 
-static inline int dev_requeue_skb_locked(struct sk_buff *skb, struct Qdisc *q)
-{
-	spinlock_t *lock = qdisc_lock(q);
-
-	spin_lock(lock);
 	while (skb) {
 		struct sk_buff *next = skb->next;
 
 		__skb_queue_tail(&q->gso_skb, skb);
 
-		qdisc_qstats_cpu_requeues_inc(q);
-		qdisc_qstats_cpu_backlog_inc(q, skb);
-		qdisc_qstats_atomic_qlen_inc(q);
+		/* it's still part of the queue */
+		if (qdisc_is_percpu_stats(q)) {
+			qdisc_qstats_cpu_requeues_inc(q);
+			qdisc_qstats_cpu_backlog_inc(q, skb);
+			qdisc_qstats_atomic_qlen_inc(q);
+		} else {
+			q->qstats.requeues++;
+			qdisc_qstats_backlog_inc(q, skb);
+			q->q.qlen++;
+		}
 
 		skb = next;
 	}
-	spin_unlock(lock);
-
+	if (lock)
+		spin_unlock(lock);
 	__netif_schedule(q);
-
-	return 0;
-}
-
-static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
-{
-	if (q->flags & TCQ_F_NOLOCK)
-		return dev_requeue_skb_locked(skb, q);
-	else
-		return __dev_requeue_skb(skb, q);
 }
 
 static void try_bulk_dequeue_skb(struct Qdisc *q,
-- 
2.20.1

[PATCH net-next 4/5] net: sched: when clearing NOLOCK, clear TCQ_F_CPUSTATS, too

[Paolo Abeni]

Since stats updating is always consistent with TCQ_F_CPUSTATS flag,
we can disable it at qdisc creation time flipping such bit.

In my experiments, if the NOLOCK flag is cleared, per CPU stats
accounting does not give any measurable performance gain, but it
waste some memory.

Let's clear TCQ_F_CPUSTATS together with NOLOCK, when enslaving
a NOLOCK qdisc to 'lock' one.

Use stats update helper inside pfifo_fast, to cope correctly with
TCQ_F_CPUSTATS flag change.

As a side effect, q.qlen value for any child qdiscs is always
consistent for all lock classfull qdiscs.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sch_generic.h | 26 ++++++++++++++++++++++++++
 net/sched/sch_api.c       | 15 ++++++++++++++-
 net/sched/sch_generic.c   | 10 ++--------
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 249817d97bf7..cdd1671bdd22 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -1106,6 +1106,32 @@ static inline struct sk_buff *qdisc_peek_dequeued(struct Qdisc *sch)
 	return skb;
 }
 
+static inline void qdisc_update_stats_at_dequeue(struct Qdisc *sch,
+						 struct sk_buff *skb)
+{
+	if (qdisc_is_percpu_stats(sch)) {
+		qdisc_qstats_cpu_backlog_dec(sch, skb);
+		qdisc_bstats_cpu_update(sch, skb);
+		qdisc_qstats_atomic_qlen_dec(sch);
+	} else {
+		qdisc_qstats_backlog_dec(sch, skb);
+		qdisc_bstats_update(sch, skb);
+		sch->q.qlen--;
+	}
+}
+
+static inline void qdisc_update_stats_at_enqueue(struct Qdisc *sch,
+						 unsigned pkt_len)
+{
+	if (qdisc_is_percpu_stats(sch)) {
+		qdisc_qstats_atomic_qlen_inc(sch);
+		this_cpu_add(sch->cpu_qstats->backlog, pkt_len);
+	} else {
+		sch->qstats.backlog += pkt_len;
+		sch->q.qlen++;
+	}
+}
+
 /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
 static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
 {
diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c
index fb8f138b9776..c126b9f78d6e 100644
--- a/net/sched/sch_api.c
+++ b/net/sched/sch_api.c
@@ -998,6 +998,19 @@ static void notify_and_destroy(struct net *net, struct sk_buff *skb,
 		qdisc_put(old);
 }
 
+static void qdisc_clear_nolock(struct Qdisc *sch)
+{
+	sch->flags &= ~TCQ_F_NOLOCK;
+	if (!(sch->flags & TCQ_F_CPUSTATS))
+		return;
+
+	free_percpu(sch->cpu_bstats);
+	free_percpu(sch->cpu_qstats);
+	sch->cpu_bstats = NULL;
+	sch->cpu_qstats = NULL;
+	sch->flags &= ~TCQ_F_CPUSTATS;
+}
+
 /* Graft qdisc "new" to class "classid" of qdisc "parent" or
  * to device "dev".
  *
@@ -1076,7 +1089,7 @@ static int qdisc_graft(struct net_device *dev, struct Qdisc *parent,
 		/* Only support running class lockless if parent is lockless */
 		if (new && (new->flags & TCQ_F_NOLOCK) &&
 		    parent && !(parent->flags & TCQ_F_NOLOCK))
-			new->flags &= ~TCQ_F_NOLOCK;
+			qdisc_clear_nolock(new);
 
 		if (!cops || !cops->graft)
 			return -EOPNOTSUPP;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ddff2952be87..12a6e1a39fa0 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -629,11 +629,7 @@ static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc *qdisc,
 	if (unlikely(err))
 		return qdisc_drop_cpu(skb, qdisc, to_free);
 
-	qdisc_qstats_atomic_qlen_inc(qdisc);
-	/* Note: skb can not be used after skb_array_produce(),
-	 * so we better not use qdisc_qstats_cpu_backlog_inc()
-	 */
-	this_cpu_add(qdisc->cpu_qstats->backlog, pkt_len);
+	qdisc_update_stats_at_enqueue(qdisc, pkt_len);
 	return NET_XMIT_SUCCESS;
 }
 
@@ -652,9 +648,7 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
 		skb = __skb_array_consume(q);
 	}
 	if (likely(skb)) {
-		qdisc_qstats_cpu_backlog_dec(qdisc, skb);
-		qdisc_bstats_cpu_update(qdisc, skb);
-		qdisc_qstats_atomic_qlen_dec(qdisc);
+		qdisc_update_stats_at_dequeue(qdisc, skb);
 	} else {
 		qdisc->empty = true;
 	}
-- 
2.20.1

[PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"

[Paolo Abeni]

This revert commit 46b1c18f9deb ("net: sched: put back q.qlen
into a single location").
After the previous patch nobody accesses directly qlen for a child
qdisc when such qdisc does per CPU stats accounting.
In the control path nobody uses directly qlen since commit
677f1bc207c ("net: sched: introduce and use qdisc tree flush/purge
helpers"), so we can remove the contented atomic ops from the
datapath.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sch_generic.h | 35 ++++++++++++++++++++---------------
 net/core/gen_stats.c      |  2 ++
 net/sched/sch_generic.c   |  9 +++++----
 3 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index cdd1671bdd22..bee9d9eac9fa 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -52,10 +52,7 @@ struct qdisc_size_table {
 struct qdisc_skb_head {
 	struct sk_buff	*head;
 	struct sk_buff	*tail;
-	union {
-		u32		qlen;
-		atomic_t	atomic_qlen;
-	};
+	__u32		qlen;
 	spinlock_t	lock;
 };
 
@@ -474,19 +471,27 @@ static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
 	BUILD_BUG_ON(sizeof(qcb->data) < sz);
 }
 
+static inline int qdisc_qlen_cpu(const struct Qdisc *q)
+{
+	return this_cpu_ptr(q->cpu_qstats)->qlen;
+}
+
 static inline int qdisc_qlen(const struct Qdisc *q)
 {
 	return q->q.qlen;
 }
 
-static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
+static inline int qdisc_qlen_sum(const struct Qdisc *q)
 {
-	u32 qlen = q->qstats.qlen;
+	__u32 qlen = q->qstats.qlen;
+	int i;
 
-	if (q->flags & TCQ_F_CPUSTATS)
-		qlen += atomic_read(&q->q.atomic_qlen);
-	else
+	if (q->flags & TCQ_F_CPUSTATS) {
+		for_each_possible_cpu(i)
+			qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen;
+	} else {
 		qlen += q->q.qlen;
+	}
 
 	return qlen;
 }
@@ -889,14 +894,14 @@ static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
 	this_cpu_add(sch->cpu_qstats->backlog, qdisc_pkt_len(skb));
 }
 
-static inline void qdisc_qstats_atomic_qlen_inc(struct Qdisc *sch)
+static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
 {
-	atomic_inc(&sch->q.atomic_qlen);
+	this_cpu_inc(sch->cpu_qstats->qlen);
 }
 
-static inline void qdisc_qstats_atomic_qlen_dec(struct Qdisc *sch)
+static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
 {
-	atomic_dec(&sch->q.atomic_qlen);
+	this_cpu_dec(sch->cpu_qstats->qlen);
 }
 
 static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)
@@ -1112,7 +1117,7 @@ static inline void qdisc_update_stats_at_dequeue(struct Qdisc *sch,
 	if (qdisc_is_percpu_stats(sch)) {
 		qdisc_qstats_cpu_backlog_dec(sch, skb);
 		qdisc_bstats_cpu_update(sch, skb);
-		qdisc_qstats_atomic_qlen_dec(sch);
+		qdisc_qstats_cpu_qlen_dec(sch);
 	} else {
 		qdisc_qstats_backlog_dec(sch, skb);
 		qdisc_bstats_update(sch, skb);
@@ -1124,7 +1129,7 @@ static inline void qdisc_update_stats_at_enqueue(struct Qdisc *sch,
 						 unsigned pkt_len)
 {
 	if (qdisc_is_percpu_stats(sch)) {
-		qdisc_qstats_atomic_qlen_inc(sch);
+		qdisc_qstats_cpu_qlen_inc(sch);
 		this_cpu_add(sch->cpu_qstats->backlog, pkt_len);
 	} else {
 		sch->qstats.backlog += pkt_len;
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index ac679f74ba47..9bf1b9ad1780 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -291,6 +291,7 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
 	for_each_possible_cpu(i) {
 		const struct gnet_stats_queue *qcpu = per_cpu_ptr(q, i);
 
+		qstats->qlen = 0;
 		qstats->backlog += qcpu->backlog;
 		qstats->drops += qcpu->drops;
 		qstats->requeues += qcpu->requeues;
@@ -306,6 +307,7 @@ 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;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 12a6e1a39fa0..848aab3693bd 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -68,7 +68,7 @@ static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
 			skb = __skb_dequeue(&q->skb_bad_txq);
 			if (qdisc_is_percpu_stats(q)) {
 				qdisc_qstats_cpu_backlog_dec(q, skb);
-				qdisc_qstats_atomic_qlen_dec(q);
+				qdisc_qstats_cpu_qlen_dec(q);
 			} else {
 				qdisc_qstats_backlog_dec(q, skb);
 				q->q.qlen--;
@@ -108,7 +108,7 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
 
 	if (qdisc_is_percpu_stats(q)) {
 		qdisc_qstats_cpu_backlog_inc(q, skb);
-		qdisc_qstats_atomic_qlen_inc(q);
+		qdisc_qstats_cpu_qlen_inc(q);
 	} else {
 		qdisc_qstats_backlog_inc(q, skb);
 		q->q.qlen++;
@@ -136,7 +136,7 @@ static inline void dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 		if (qdisc_is_percpu_stats(q)) {
 			qdisc_qstats_cpu_requeues_inc(q);
 			qdisc_qstats_cpu_backlog_inc(q, skb);
-			qdisc_qstats_atomic_qlen_inc(q);
+			qdisc_qstats_cpu_qlen_inc(q);
 		} else {
 			q->qstats.requeues++;
 			qdisc_qstats_backlog_inc(q, skb);
@@ -236,7 +236,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
 			skb = __skb_dequeue(&q->gso_skb);
 			if (qdisc_is_percpu_stats(q)) {
 				qdisc_qstats_cpu_backlog_dec(q, skb);
-				qdisc_qstats_atomic_qlen_dec(q);
+				qdisc_qstats_cpu_qlen_dec(q);
 			} else {
 				qdisc_qstats_backlog_dec(q, skb);
 				q->q.qlen--;
@@ -694,6 +694,7 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
 		struct gnet_stats_queue *q = per_cpu_ptr(qdisc->cpu_qstats, i);
 
 		q->backlog = 0;
+		q->qlen = 0;
 	}
 }
 
-- 
2.20.1

Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"

[Eric Dumazet]

On 04/08/2019 09:35 AM, Paolo Abeni wrote:
> This revert commit 46b1c18f9deb ("net: sched: put back q.qlen
> into a single location").
> After the previous patch nobody accesses directly qlen for a child
> qdisc when such qdisc does per CPU stats accounting.
> In the control path nobody uses directly qlen since commit
> 677f1bc207c ("net: sched: introduce and use qdisc tree flush/purge
> helpers"), so we can remove the contented atomic ops from the
> datapath.
>

Have you tested HTB with a pfifo_fast on a throttled class ?

I do not see any changes in HTB in your patch series, so it is not
clear why your patch series is not adding back the issue.

Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"

[Paolo Abeni]

Hi,

On Mon, 2019-04-08 at 14:17 -0700, Eric Dumazet wrote:
> On 04/08/2019 09:35 AM, Paolo Abeni wrote:
> > This revert commit 46b1c18f9deb ("net: sched: put back q.qlen
> > into a single location").
> > After the previous patch nobody accesses directly qlen for a child
> > qdisc when such qdisc does per CPU stats accounting.
> > In the control path nobody uses directly qlen since commit
> > 677f1bc207c ("net: sched: introduce and use qdisc tree flush/purge
> > helpers"), so we can remove the contented atomic ops from the
> > datapath.
> > 
> 
> Have you tested HTB with a pfifo_fast on a throttled class ?
> 
> I do not see any changes in HTB in your patch series, so it is not
> clear why your patch series is not adding back the issue.

Thank you for the feedback.

I tested this series enslaving pfifo_fast to each classful qdiscs -
including HTB - sending traffic through the pfifo_fast qdisc, and
checking correct accounting.

When pfifo_fast is enslaved to HTB, the NOLOCK flag is cleared -  by
qdisc_graft(), as HTB is a lock qdisc. As per patch 4/5, TCQ_F_CPUSTATS
is cleared, too, so pfifo_fast switches to global accounting, under
root lock protection.

In HTB context, <child pfifo_fast>->q.qlen should be always valid, no
changes required there, nor to any other classful qdisc - until we will
have lockless classful qdiscs: they should not access <child>->q.qlen
directly.

Please let me know if the above is somewhat clear. Have I missed
something?

Thanks,

Paolo

Re: [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 9347 bytes --]

Hi Paolo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: i386-randconfig-x077-201914 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from net//vmw_vsock/af_vsock.c:118:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
>> include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
           ida_is_empty
   include/net/sch_generic.h: At top level:
>> include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^~~~~~~~~~~~~~
   include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
   cc1: some warnings being treated as errors
--
   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from include/net/inet_sock.h:26,
                    from include/net/inet_connection_sock.h:24,
                    from include/linux/dccp.h:13,
                    from net//dccp/proto.c:12:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
>> include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
           ida_is_empty
   include/net/sch_generic.h: At top level:
>> include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^~~~~~~~~~~~~~
   include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
   In file included from net//dccp/trace.h:84:0,
                    from net//dccp/proto.c:42:
   include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
    #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
                                             ^
   cc1: some warnings being treated as errors
   compilation terminated.

vim +743 include/net/sch_generic.h

5aa709954 David S. Miller 2008-07-08  732  
3e745dd69 David S. Miller 2008-07-08  733  /* Are all TX queues of the device empty?  */
3e745dd69 David S. Miller 2008-07-08  734  static inline bool qdisc_all_tx_empty(const struct net_device *dev)
3e745dd69 David S. Miller 2008-07-08  735  {
e8a0464cc David S. Miller 2008-07-17  736  	unsigned int i;
46e5da40a John Fastabend  2014-09-12  737  
46e5da40a John Fastabend  2014-09-12  738  	rcu_read_lock();
e8a0464cc David S. Miller 2008-07-17  739  	for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17  740  		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend  2014-09-12  741  		const struct Qdisc *q = rcu_dereference(txq->qdisc);
3e745dd69 David S. Miller 2008-07-08  742  
630dee66e Paolo Abeni     2019-04-08 @743  		if (!qdisc_is_empty(q)) {
46e5da40a John Fastabend  2014-09-12  744  			rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17  745  			return false;
e8a0464cc David S. Miller 2008-07-17  746  		}
46e5da40a John Fastabend  2014-09-12  747  	}
46e5da40a John Fastabend  2014-09-12  748  	rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17  749  	return true;
3e745dd69 David S. Miller 2008-07-08  750  }
3e745dd69 David S. Miller 2008-07-08  751  
6fa9864b5 David S. Miller 2008-07-08  752  /* Are any of the TX qdiscs changing?  */
05bdd2f14 Eric Dumazet    2011-10-20  753  static inline bool qdisc_tx_changing(const struct net_device *dev)
6fa9864b5 David S. Miller 2008-07-08  754  {
e8a0464cc David S. Miller 2008-07-17  755  	unsigned int i;
46e5da40a John Fastabend  2014-09-12  756  
e8a0464cc David S. Miller 2008-07-17  757  	for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17  758  		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend  2014-09-12  759  		if (rcu_access_pointer(txq->qdisc) != txq->qdisc_sleeping)
e8a0464cc David S. Miller 2008-07-17  760  			return true;
e8a0464cc David S. Miller 2008-07-17  761  	}
e8a0464cc David S. Miller 2008-07-17  762  	return false;
6fa9864b5 David S. Miller 2008-07-08  763  }
6fa9864b5 David S. Miller 2008-07-08  764  
e8a0464cc David S. Miller 2008-07-17  765  /* Is the device using the noop qdisc on all queues?  */
052979499 David S. Miller 2008-07-08  766  static inline bool qdisc_tx_is_noop(const struct net_device *dev)
052979499 David S. Miller 2008-07-08  767  {
e8a0464cc David S. Miller 2008-07-17  768  	unsigned int i;
46e5da40a John Fastabend  2014-09-12  769  
e8a0464cc David S. Miller 2008-07-17  770  	for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17  771  		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend  2014-09-12  772  		if (rcu_access_pointer(txq->qdisc) != &noop_qdisc)
e8a0464cc David S. Miller 2008-07-17  773  			return false;
e8a0464cc David S. Miller 2008-07-17  774  	}
e8a0464cc David S. Miller 2008-07-17  775  	return true;
052979499 David S. Miller 2008-07-08  776  }
052979499 David S. Miller 2008-07-08  777  
bfe0d0298 Eric Dumazet    2011-01-09  778  static inline unsigned int qdisc_pkt_len(const struct sk_buff *skb)
0abf77e55 Jussi Kivilinna 2008-07-20  779  {
175f9c1bb Jussi Kivilinna 2008-07-20  780  	return qdisc_skb_cb(skb)->pkt_len;
0abf77e55 Jussi Kivilinna 2008-07-20  781  }
0abf77e55 Jussi Kivilinna 2008-07-20  782  
c27f339af Jarek Poplawski 2008-08-04  783  /* additional qdisc xmit flags (NET_XMIT_MASK in linux/netdevice.h) */
378a2f090 Jarek Poplawski 2008-08-04  784  enum net_xmit_qdisc_t {
378a2f090 Jarek Poplawski 2008-08-04  785  	__NET_XMIT_STOLEN = 0x00010000,
c27f339af Jarek Poplawski 2008-08-04  786  	__NET_XMIT_BYPASS = 0x00020000,
378a2f090 Jarek Poplawski 2008-08-04  787  };
378a2f090 Jarek Poplawski 2008-08-04  788  
c27f339af Jarek Poplawski 2008-08-04  789  #ifdef CONFIG_NET_CLS_ACT
378a2f090 Jarek Poplawski 2008-08-04  790  #define net_xmit_drop_count(e)	((e) & __NET_XMIT_STOLEN ? 0 : 1)
378a2f090 Jarek Poplawski 2008-08-04  791  #else
378a2f090 Jarek Poplawski 2008-08-04  792  #define net_xmit_drop_count(e)	(1)
378a2f090 Jarek Poplawski 2008-08-04  793  #endif
378a2f090 Jarek Poplawski 2008-08-04  794  
a2da570d6 Eric Dumazet    2011-01-20  795  static inline void qdisc_calculate_pkt_len(struct sk_buff *skb,
a2da570d6 Eric Dumazet    2011-01-20  796  					   const struct Qdisc *sch)
5f86173bd Jussi Kivilinna 2008-07-20  797  {
3a682fbd7 David S. Miller 2008-07-20  798  #ifdef CONFIG_NET_SCHED
a2da570d6 Eric Dumazet    2011-01-20  799  	struct qdisc_size_table *stab = rcu_dereference_bh(sch->stab);
a2da570d6 Eric Dumazet    2011-01-20  800  
a2da570d6 Eric Dumazet    2011-01-20  801  	if (stab)
a2da570d6 Eric Dumazet    2011-01-20  802  		__qdisc_calculate_pkt_len(skb, stab);
3a682fbd7 David S. Miller 2008-07-20  803  #endif
a2da570d6 Eric Dumazet    2011-01-20  804  }
a2da570d6 Eric Dumazet    2011-01-20  805  
520ac30f4 Eric Dumazet    2016-06-21  806  static inline int qdisc_enqueue(struct sk_buff *skb, struct Qdisc *sch,
520ac30f4 Eric Dumazet    2016-06-21  807  				struct sk_buff **to_free)
a2da570d6 Eric Dumazet    2011-01-20  808  {
a2da570d6 Eric Dumazet    2011-01-20  809  	qdisc_calculate_pkt_len(skb, sch);
520ac30f4 Eric Dumazet    2016-06-21  810  	return sch->enqueue(skb, sch, to_free);
5f86173bd Jussi Kivilinna 2008-07-20  811  }
5f86173bd Jussi Kivilinna 2008-07-20  812  
22e0f8b93 John Fastabend  2014-09-28  813  static inline bool qdisc_is_percpu_stats(const struct Qdisc *q)
22e0f8b93 John Fastabend  2014-09-28  814  {
22e0f8b93 John Fastabend  2014-09-28  815  	return q->flags & TCQ_F_CPUSTATS;
22e0f8b93 John Fastabend  2014-09-28  816  }
bfe0d0298 Eric Dumazet    2011-01-09  817  
9cda4ff7e Paolo Abeni     2019-04-08 @818  static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
9cda4ff7e Paolo Abeni     2019-04-08  819  {
9cda4ff7e Paolo Abeni     2019-04-08  820  	if (qdisc_is_percpu_stats(qdisc))
9cda4ff7e Paolo Abeni     2019-04-08  821  		return qdisc->empty;
9cda4ff7e Paolo Abeni     2019-04-08  822  	return !qdisc->q.qlen;
9cda4ff7e Paolo Abeni     2019-04-08  823  }
9cda4ff7e Paolo Abeni     2019-04-08  824  

:::::: The code at line 743 was first introduced by commit
:::::: 630dee66e06a4ec68b87f0d1a9959b54b42f8959 net: sched: prefer qdisc_is_empty() over direct qlen access

:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32236 bytes --]

Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 5434 bytes --]

Hi Paolo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: i386-randconfig-x077-201914 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from include/net/cls_cgroup.h:19,
                    from net/socket.c:99:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
   include/net/sch_generic.h:748:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
           ida_is_empty
   include/net/sch_generic.h: At top level:
   include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^~~~~~~~~~~~~~
   include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
   include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec'; did you mean 'qdisc_qstats_cpu_qlen_dec'? [-Werror=implicit-function-declaration]
       qdisc_qstats_atomic_qlen_dec(sch);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
       qdisc_qstats_cpu_qlen_dec
   cc1: some warnings being treated as errors
--
   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from include/net/inet_sock.h:26,
                    from include/net/inet_connection_sock.h:24,
                    from include/linux/dccp.h:13,
                    from net//dccp/proto.c:12:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
   include/net/sch_generic.h:748:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
           ida_is_empty
   include/net/sch_generic.h: At top level:
   include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^~~~~~~~~~~~~~
   include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^~~~~~~~~~~~~~
   include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec'; did you mean 'qdisc_qstats_cpu_qlen_dec'? [-Werror=implicit-function-declaration]
       qdisc_qstats_atomic_qlen_dec(sch);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
       qdisc_qstats_cpu_qlen_dec
   In file included from net//dccp/trace.h:84:0,
                    from net//dccp/proto.c:42:
   include/trace/define_trace.h: At top level:
   include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
    #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
                                             ^
   cc1: some warnings being treated as errors
   compilation terminated.

vim +1149 include/net/sch_generic.h

fa70e3d2 Paolo Abeni     2019-04-08  1139  
77be155c Jarek Poplawski 2008-10-31  1140  /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
77be155c Jarek Poplawski 2008-10-31  1141  static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
77be155c Jarek Poplawski 2008-10-31  1142  {
a53851e2 John Fastabend  2017-12-07  1143  	struct sk_buff *skb = skb_peek(&sch->gso_skb);
77be155c Jarek Poplawski 2008-10-31  1144  
61c9eaf9 Jarek Poplawski 2008-11-05  1145  	if (skb) {
a53851e2 John Fastabend  2017-12-07  1146  		skb = __skb_dequeue(&sch->gso_skb);
9cda4ff7 Paolo Abeni     2019-04-08  1147  		if (qdisc_is_percpu_stats(sch)) {
9cda4ff7 Paolo Abeni     2019-04-08  1148  			qdisc_qstats_cpu_backlog_dec(sch, skb);
9cda4ff7 Paolo Abeni     2019-04-08 @1149  			qdisc_qstats_atomic_qlen_dec(sch);
9cda4ff7 Paolo Abeni     2019-04-08  1150  		} else {
a27758ff WANG Cong       2016-06-03  1151  			qdisc_qstats_backlog_dec(sch, skb);
61c9eaf9 Jarek Poplawski 2008-11-05  1152  			sch->q.qlen--;
9cda4ff7 Paolo Abeni     2019-04-08  1153  		}
61c9eaf9 Jarek Poplawski 2008-11-05  1154  	} else {
77be155c Jarek Poplawski 2008-10-31  1155  		skb = sch->dequeue(sch);
61c9eaf9 Jarek Poplawski 2008-11-05  1156  	}
77be155c Jarek Poplawski 2008-10-31  1157  
77be155c Jarek Poplawski 2008-10-31  1158  	return skb;
77be155c Jarek Poplawski 2008-10-31  1159  }
77be155c Jarek Poplawski 2008-10-31  1160  

:::::: The code at line 1149 was first introduced by commit
:::::: 9cda4ff7ed51bb469cb19e03c9fe4972408edb63 net: sched: always do stats accounting according to TCQ_F_CPUSTATS

:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32236 bytes --]

Re: [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6834 bytes --]

Hi Paolo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: x86_64-randconfig-x001-201914 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bug.h:5:0,
                    from arch/x86/include/asm/bug.h:83,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from net/socket.c:61:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
   include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
           ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
      if (!qdisc_is_empty(q)) {
      ^~
   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from include/net/cls_cgroup.h:19,
                    from net/socket.c:99:
   include/net/sch_generic.h: At top level:
   include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^~~~~~~~~~~~~~
   In file included from include/asm-generic/bug.h:5:0,
                    from arch/x86/include/asm/bug.h:83,
                    from include/linux/bug.h:5,
                    from include/linux/mmdebug.h:5,
                    from include/linux/mm.h:9,
                    from net/socket.c:61:
   include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
      if (!qdisc_is_empty(q)) {
      ^~
   cc1: some warnings being treated as errors
--
   In file included from include/linux/kernel.h:11:0,
                    from include/linux/uio.h:12,
                    from include/linux/socket.h:8,
                    from include/uapi/linux/in.h:24,
                    from include/linux/in.h:23,
                    from include/linux/dccp.h:6,
                    from net//dccp/proto.c:12:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
   include/net/sch_generic.h:743:8: error: implicit declaration of function 'qdisc_is_empty'; did you mean 'ida_is_empty'? [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
           ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
      if (!qdisc_is_empty(q)) {
      ^~
   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from include/net/inet_sock.h:26,
                    from include/net/inet_connection_sock.h:24,
                    from include/linux/dccp.h:13,
                    from net//dccp/proto.c:12:
   include/net/sch_generic.h: At top level:
   include/net/sch_generic.h:818:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^~~~~~~~~~~~~~
   In file included from include/linux/kernel.h:11:0,
                    from include/linux/uio.h:12,
                    from include/linux/socket.h:8,
                    from include/uapi/linux/in.h:24,
                    from include/linux/in.h:23,
                    from include/linux/dccp.h:6,
                    from net//dccp/proto.c:12:
   include/net/sch_generic.h:743:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^
   include/linux/compiler.h:58:30: note: in definition of macro '__trace_if'
     if (__builtin_constant_p(!!(cond)) ? !!(cond) :   \
                                 ^~~~
>> include/net/sch_generic.h:743:3: note: in expansion of macro 'if'
      if (!qdisc_is_empty(q)) {
      ^~
   In file included from net//dccp/trace.h:84:0,
                    from net//dccp/proto.c:42:
   include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
    #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
                                             ^
   cc1: some warnings being treated as errors
   compilation terminated.

vim +/if +743 include/net/sch_generic.h

5aa709954 David S. Miller 2008-07-08  732  
3e745dd69 David S. Miller 2008-07-08  733  /* Are all TX queues of the device empty?  */
3e745dd69 David S. Miller 2008-07-08  734  static inline bool qdisc_all_tx_empty(const struct net_device *dev)
3e745dd69 David S. Miller 2008-07-08  735  {
e8a0464cc David S. Miller 2008-07-17  736  	unsigned int i;
46e5da40a John Fastabend  2014-09-12  737  
46e5da40a John Fastabend  2014-09-12  738  	rcu_read_lock();
e8a0464cc David S. Miller 2008-07-17  739  	for (i = 0; i < dev->num_tx_queues; i++) {
e8a0464cc David S. Miller 2008-07-17  740  		struct netdev_queue *txq = netdev_get_tx_queue(dev, i);
46e5da40a John Fastabend  2014-09-12  741  		const struct Qdisc *q = rcu_dereference(txq->qdisc);
3e745dd69 David S. Miller 2008-07-08  742  
630dee66e Paolo Abeni     2019-04-08 @743  		if (!qdisc_is_empty(q)) {
46e5da40a John Fastabend  2014-09-12  744  			rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17  745  			return false;
e8a0464cc David S. Miller 2008-07-17  746  		}
46e5da40a John Fastabend  2014-09-12  747  	}
46e5da40a John Fastabend  2014-09-12  748  	rcu_read_unlock();
e8a0464cc David S. Miller 2008-07-17  749  	return true;
3e745dd69 David S. Miller 2008-07-08  750  }
3e745dd69 David S. Miller 2008-07-08  751  

:::::: The code at line 743 was first introduced by commit
:::::: 630dee66e06a4ec68b87f0d1a9959b54b42f8959 net: sched: prefer qdisc_is_empty() over direct qlen access

:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32103 bytes --]

Re: [PATCH net-next 3/5] net: sched: always do stats accounting according to TCQ_F_CPUSTATS

[Paolo Abeni]

On Tue, 2019-04-09 at 17:50 +0800, kbuild test robot wrote:
> Hi Paolo,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on net-next/master]
> 
> url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
> config: i386-randconfig-x077-201914 (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
>         # save the attached .config to linux build tree
>         make ARCH=i386 

I apologize, a last bunch of local changes did not land into the
submitted patches due to PEBKAC. 

(qdisc_is_empty() declaration in the correct place and full replacement
of qdisc_qstats_atomic_qlen_dec in patch 5/5).

I'll fix in v2,

Thanks,

Paolo

Re: [PATCH net-next 5/5] Revert: "net: sched: put back q.qlen into a single location"

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 4995 bytes --]

Hi Paolo,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-caif-avoid-using-qdisc_qlen/20190409-164620
config: i386-randconfig-b0-04091710 (attached as .config)
compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from include/net/cls_cgroup.h:19,
                    from net/socket.c:99:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
   include/net/sch_generic.h:748:3: error: implicit declaration of function 'qdisc_is_empty' [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
      ^
   include/net/sch_generic.h: At top level:
   include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^
   include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^
   include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec' [-Werror=implicit-function-declaration]
       qdisc_qstats_atomic_qlen_dec(sch);
       ^
   cc1: some warnings being treated as errors
--
   In file included from include/linux/filter.h:24:0,
                    from include/net/sock.h:64,
                    from net//tipc/socket.h:38,
                    from net//tipc/trace.h:45,
                    from net//tipc/trace.c:37:
   include/net/sch_generic.h: In function 'qdisc_all_tx_empty':
   include/net/sch_generic.h:748:3: error: implicit declaration of function 'qdisc_is_empty' [-Werror=implicit-function-declaration]
      if (!qdisc_is_empty(q)) {
      ^
   include/net/sch_generic.h: At top level:
   include/net/sch_generic.h:823:20: error: conflicting types for 'qdisc_is_empty'
    static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
                       ^
   include/net/sch_generic.h:748:8: note: previous implicit declaration of 'qdisc_is_empty' was here
      if (!qdisc_is_empty(q)) {
           ^
   include/net/sch_generic.h: In function 'qdisc_dequeue_peeked':
>> include/net/sch_generic.h:1149:4: error: implicit declaration of function 'qdisc_qstats_atomic_qlen_dec' [-Werror=implicit-function-declaration]
       qdisc_qstats_atomic_qlen_dec(sch);
       ^
   In file included from net//tipc/trace.h:431:0,
                    from net//tipc/trace.c:37:
   include/trace/define_trace.h: At top level:
   include/trace/define_trace.h:89:43: fatal error: ./trace.h: No such file or directory
    #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
                                              ^
   cc1: some warnings being treated as errors
   compilation terminated.

vim +/qdisc_qstats_atomic_qlen_dec +1149 include/net/sch_generic.h

fa70e3d2 Paolo Abeni     2019-04-08  1139  
77be155c Jarek Poplawski 2008-10-31  1140  /* use instead of qdisc->dequeue() for all qdiscs queried with ->peek() */
77be155c Jarek Poplawski 2008-10-31  1141  static inline struct sk_buff *qdisc_dequeue_peeked(struct Qdisc *sch)
77be155c Jarek Poplawski 2008-10-31  1142  {
a53851e2 John Fastabend  2017-12-07  1143  	struct sk_buff *skb = skb_peek(&sch->gso_skb);
77be155c Jarek Poplawski 2008-10-31  1144  
61c9eaf9 Jarek Poplawski 2008-11-05  1145  	if (skb) {
a53851e2 John Fastabend  2017-12-07  1146  		skb = __skb_dequeue(&sch->gso_skb);
9cda4ff7 Paolo Abeni     2019-04-08  1147  		if (qdisc_is_percpu_stats(sch)) {
9cda4ff7 Paolo Abeni     2019-04-08  1148  			qdisc_qstats_cpu_backlog_dec(sch, skb);
9cda4ff7 Paolo Abeni     2019-04-08 @1149  			qdisc_qstats_atomic_qlen_dec(sch);
9cda4ff7 Paolo Abeni     2019-04-08  1150  		} else {
a27758ff WANG Cong       2016-06-03  1151  			qdisc_qstats_backlog_dec(sch, skb);
61c9eaf9 Jarek Poplawski 2008-11-05  1152  			sch->q.qlen--;
9cda4ff7 Paolo Abeni     2019-04-08  1153  		}
61c9eaf9 Jarek Poplawski 2008-11-05  1154  	} else {
77be155c Jarek Poplawski 2008-10-31  1155  		skb = sch->dequeue(sch);
61c9eaf9 Jarek Poplawski 2008-11-05  1156  	}
77be155c Jarek Poplawski 2008-10-31  1157  
77be155c Jarek Poplawski 2008-10-31  1158  	return skb;
77be155c Jarek Poplawski 2008-10-31  1159  }
77be155c Jarek Poplawski 2008-10-31  1160  

:::::: The code at line 1149 was first introduced by commit
:::::: 9cda4ff7ed51bb469cb19e03c9fe4972408edb63 net: sched: always do stats accounting according to TCQ_F_CPUSTATS

:::::: TO: Paolo Abeni <pabeni@redhat.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35508 bytes --]