Skip to content

Commit bfcc998

Browse files
edumazetgregkh
edumazet
authored and
gregkh
committed
net: sched: put back q.qlen into a single location
[ Upstream commit 46b1c18 ] In the series fc8b81a ("Merge branch 'lockless-qdisc-series'") John made the assumption that the data path had no need to read the qdisc qlen (number of packets in the qdisc). It is true when pfifo_fast is used as the root qdisc, or as direct MQ/MQPRIO children. But pfifo_fast can be used as leaf in class full qdiscs, and existing logic needs to access the child qlen in an efficient way. HTB breaks badly, since it uses cl->leaf.q->q.qlen in : htb_activate() -> WARN_ON() htb_dequeue_tree() to decide if a class can be htb_deactivated when it has no more packets. HFSC, DRR, CBQ, QFQ have similar issues, and some calls to qdisc_tree_reduce_backlog() also read q.qlen directly. Using qdisc_qlen_sum() (which iterates over all possible cpus) in the data path is a non starter. It seems we have to put back qlen in a central location, at least for stable kernels. For all qdisc but pfifo_fast, qlen is guarded by the qdisc lock, so the existing q.qlen{++|--} are correct. For 'lockless' qdisc (pfifo_fast so far), we need to use atomic_{inc|dec}() because the spinlock might be not held (for example from pfifo_fast_enqueue() and pfifo_fast_dequeue()) This patch adds atomic_qlen (in the same location than qlen) and renames the following helpers, since we want to express they can be used without qdisc lock, and that qlen is no longer percpu. - qdisc_qstats_cpu_qlen_dec -> qdisc_qstats_atomic_qlen_dec() - qdisc_qstats_cpu_qlen_inc -> qdisc_qstats_atomic_qlen_inc() Later (net-next) we might revert this patch by tracking all these qlen uses and replace them by a more efficient method (not having to access a precise qlen, but an empty/non_empty status that might be less expensive to maintain/track). Another possibility is to have a legacy pfifo_fast version that would be used when used a a child qdisc, since the parent qdisc needs a spinlock anyway. But then, future lockless qdiscs would also have the same problem. Fixes: 7e66016 ("net: sched: helpers to sum qlen and qlen for per cpu logic") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: John Fastabend <john.fastabend@gmail.com> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Cong Wang <xiyou.wangcong@gmail.com> Cc: Jiri Pirko <jiri@resnulli.us> Signed-off-by: David S. Miller <davem@davemloft.net> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent 5a273ea commit bfcc998

3 files changed

Lines changed: 19 additions & 27 deletions

File tree

‎include/net/sch_generic.h‎

Lines changed: 13 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,10 @@ struct qdisc_size_table {
4848
struct qdisc_skb_head {
4949
struct sk_buff *head;
5050
struct sk_buff *tail;
51-
__u32 qlen;
51+
union {
52+
u32 qlen;
53+
atomic_t atomic_qlen;
54+
};
5255
spinlock_t lock;
5356
};
5457

@@ -405,27 +408,19 @@ static inline void qdisc_cb_private_validate(const struct sk_buff *skb, int sz)
405408
BUILD_BUG_ON(sizeof(qcb->data) < sz);
406409
}
407410

408-
static inline int qdisc_qlen_cpu(const struct Qdisc *q)
409-
{
410-
return this_cpu_ptr(q->cpu_qstats)->qlen;
411-
}
412-
413411
static inline int qdisc_qlen(const struct Qdisc *q)
414412
{
415413
return q->q.qlen;
416414
}
417415

418-
static inline int qdisc_qlen_sum(const struct Qdisc *q)
416+
static inline u32 qdisc_qlen_sum(const struct Qdisc *q)
419417
{
420-
__u32 qlen = q->qstats.qlen;
421-
int i;
418+
u32 qlen = q->qstats.qlen;
422419

423-
if (q->flags & TCQ_F_NOLOCK) {
424-
for_each_possible_cpu(i)
425-
qlen += per_cpu_ptr(q->cpu_qstats, i)->qlen;
426-
} else {
420+
if (q->flags & TCQ_F_NOLOCK)
421+
qlen += atomic_read(&q->q.atomic_qlen);
422+
else
427423
qlen += q->q.qlen;
428-
}
429424

430425
return qlen;
431426
}
@@ -798,14 +793,14 @@ static inline void qdisc_qstats_cpu_backlog_inc(struct Qdisc *sch,
798793
this_cpu_add(sch->cpu_qstats->backlog, qdisc_pkt_len(skb));
799794
}
800795

801-
static inline void qdisc_qstats_cpu_qlen_inc(struct Qdisc *sch)
796+
static inline void qdisc_qstats_atomic_qlen_inc(struct Qdisc *sch)
802797
{
803-
this_cpu_inc(sch->cpu_qstats->qlen);
798+
atomic_inc(&sch->q.atomic_qlen);
804799
}
805800

806-
static inline void qdisc_qstats_cpu_qlen_dec(struct Qdisc *sch)
801+
static inline void qdisc_qstats_atomic_qlen_dec(struct Qdisc *sch)
807802
{
808-
this_cpu_dec(sch->cpu_qstats->qlen);
803+
atomic_dec(&sch->q.atomic_qlen);
809804
}
810805

811806
static inline void qdisc_qstats_cpu_requeues_inc(struct Qdisc *sch)

‎net/core/gen_stats.c‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,6 @@ __gnet_stats_copy_queue_cpu(struct gnet_stats_queue *qstats,
291291
for_each_possible_cpu(i) {
292292
const struct gnet_stats_queue *qcpu = per_cpu_ptr(q, i);
293293

294-
qstats->qlen = 0;
295294
qstats->backlog += qcpu->backlog;
296295
qstats->drops += qcpu->drops;
297296
qstats->requeues += qcpu->requeues;
@@ -307,7 +306,6 @@ void __gnet_stats_copy_queue(struct gnet_stats_queue *qstats,
307306
if (cpu) {
308307
__gnet_stats_copy_queue_cpu(qstats, cpu);
309308
} else {
310-
qstats->qlen = q->qlen;
311309
qstats->backlog = q->backlog;
312310
qstats->drops = q->drops;
313311
qstats->requeues = q->requeues;

‎net/sched/sch_generic.c‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ static inline struct sk_buff *__skb_dequeue_bad_txq(struct Qdisc *q)
6868
skb = __skb_dequeue(&q->skb_bad_txq);
6969
if (qdisc_is_percpu_stats(q)) {
7070
qdisc_qstats_cpu_backlog_dec(q, skb);
71-
qdisc_qstats_cpu_qlen_dec(q);
71+
qdisc_qstats_atomic_qlen_dec(q);
7272
} else {
7373
qdisc_qstats_backlog_dec(q, skb);
7474
q->q.qlen--;
@@ -108,7 +108,7 @@ static inline void qdisc_enqueue_skb_bad_txq(struct Qdisc *q,
108108

109109
if (qdisc_is_percpu_stats(q)) {
110110
qdisc_qstats_cpu_backlog_inc(q, skb);
111-
qdisc_qstats_cpu_qlen_inc(q);
111+
qdisc_qstats_atomic_qlen_inc(q);
112112
} else {
113113
qdisc_qstats_backlog_inc(q, skb);
114114
q->q.qlen++;
@@ -147,7 +147,7 @@ static inline int dev_requeue_skb_locked(struct sk_buff *skb, struct Qdisc *q)
147147

148148
qdisc_qstats_cpu_requeues_inc(q);
149149
qdisc_qstats_cpu_backlog_inc(q, skb);
150-
qdisc_qstats_cpu_qlen_inc(q);
150+
qdisc_qstats_atomic_qlen_inc(q);
151151

152152
skb = next;
153153
}
@@ -252,7 +252,7 @@ static struct sk_buff *dequeue_skb(struct Qdisc *q, bool *validate,
252252
skb = __skb_dequeue(&q->gso_skb);
253253
if (qdisc_is_percpu_stats(q)) {
254254
qdisc_qstats_cpu_backlog_dec(q, skb);
255-
qdisc_qstats_cpu_qlen_dec(q);
255+
qdisc_qstats_atomic_qlen_dec(q);
256256
} else {
257257
qdisc_qstats_backlog_dec(q, skb);
258258
q->q.qlen--;
@@ -645,7 +645,7 @@ static int pfifo_fast_enqueue(struct sk_buff *skb, struct Qdisc *qdisc,
645645
if (unlikely(err))
646646
return qdisc_drop_cpu(skb, qdisc, to_free);
647647

648-
qdisc_qstats_cpu_qlen_inc(qdisc);
648+
qdisc_qstats_atomic_qlen_inc(qdisc);
649649
/* Note: skb can not be used after skb_array_produce(),
650650
* so we better not use qdisc_qstats_cpu_backlog_inc()
651651
*/
@@ -670,7 +670,7 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
670670
if (likely(skb)) {
671671
qdisc_qstats_cpu_backlog_dec(qdisc, skb);
672672
qdisc_bstats_cpu_update(qdisc, skb);
673-
qdisc_qstats_cpu_qlen_dec(qdisc);
673+
qdisc_qstats_atomic_qlen_dec(qdisc);
674674
}
675675

676676
return skb;
@@ -714,7 +714,6 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
714714
struct gnet_stats_queue *q = per_cpu_ptr(qdisc->cpu_qstats, i);
715715

716716
q->backlog = 0;
717-
q->qlen = 0;
718717
}
719718
}
720719

0 commit comments

Comments
 (0)