From 4909daba37846317ec7dcba16fba009636f7fe21 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 26 May 2020 21:35:23 -0700 Subject: net_sched: use qdisc_reset() in qdisc_destroy() qdisc_destroy() calls ops->reset() and cleans up qdisc->gso_skb and qdisc->skb_bad_txq, these are nearly same with qdisc_reset(), so just call it directly, and cosolidate the code for the next patch. Cc: Jamal Hadi Salim Cc: Jiri Pirko Signed-off-by: Cong Wang Signed-off-by: David S. Miller --- net/sched/sch_generic.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index ebc55d884247..7a0b06001e48 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -949,7 +949,6 @@ static void qdisc_free_cb(struct rcu_head *head) static void qdisc_destroy(struct Qdisc *qdisc) { const struct Qdisc_ops *ops = qdisc->ops; - struct sk_buff *skb, *tmp; #ifdef CONFIG_NET_SCHED qdisc_hash_del(qdisc); @@ -957,24 +956,15 @@ static void qdisc_destroy(struct Qdisc *qdisc) qdisc_put_stab(rtnl_dereference(qdisc->stab)); #endif gen_kill_estimator(&qdisc->rate_est); - if (ops->reset) - ops->reset(qdisc); + + qdisc_reset(qdisc); + if (ops->destroy) ops->destroy(qdisc); module_put(ops->owner); dev_put(qdisc_dev(qdisc)); - skb_queue_walk_safe(&qdisc->gso_skb, skb, tmp) { - __skb_unlink(skb, &qdisc->gso_skb); - kfree_skb_list(skb); - } - - skb_queue_walk_safe(&qdisc->skb_bad_txq, skb, tmp) { - __skb_unlink(skb, &qdisc->skb_bad_txq); - kfree_skb_list(skb); - } - call_rcu(&qdisc->rcu, qdisc_free_cb); } -- cgit From a34dac0b9055202cf9c64e08d8d8dc5e23029d3a Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 26 May 2020 21:35:24 -0700 Subject: net_sched: add tracepoints for qdisc_reset() and qdisc_destroy() Add two tracepoints for qdisc_reset() and qdisc_destroy() to track qdisc resetting and destroying. Sample output: tc-756 [000] ...3 138.355662: qdisc_reset: dev=ens3 kind=pfifo_fast parent=ffff:ffff handle=0:0 tc-756 [000] ...1 138.355720: qdisc_reset: dev=ens3 kind=pfifo_fast parent=ffff:ffff handle=0:0 tc-756 [000] ...1 138.355867: qdisc_reset: dev=ens3 kind=pfifo_fast parent=ffff:ffff handle=0:0 tc-756 [000] ...1 138.355930: qdisc_destroy: dev=ens3 kind=pfifo_fast parent=ffff:ffff handle=0:0 tc-757 [000] ...2 143.073780: qdisc_reset: dev=ens3 kind=fq_codel parent=ffff:ffff handle=8001:0 tc-757 [000] ...1 143.073878: qdisc_reset: dev=ens3 kind=fq_codel parent=ffff:ffff handle=8001:0 tc-757 [000] ...1 143.074114: qdisc_reset: dev=ens3 kind=fq_codel parent=ffff:ffff handle=8001:0 tc-757 [000] ...1 143.074228: qdisc_destroy: dev=ens3 kind=fq_codel parent=ffff:ffff handle=8001:0 Cc: Jamal Hadi Salim Cc: Jiri Pirko Signed-off-by: Cong Wang Signed-off-by: David S. Miller --- include/trace/events/qdisc.h | 52 ++++++++++++++++++++++++++++++++++++++++++++ net/sched/sch_generic.c | 4 ++++ 2 files changed, 56 insertions(+) diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h index 0d1a9ebf55ba..2b948801afa3 100644 --- a/include/trace/events/qdisc.h +++ b/include/trace/events/qdisc.h @@ -8,6 +8,8 @@ #include #include #include +#include +#include TRACE_EVENT(qdisc_dequeue, @@ -44,6 +46,56 @@ TRACE_EVENT(qdisc_dequeue, __entry->txq_state, __entry->packets, __entry->skbaddr ) ); +TRACE_EVENT(qdisc_reset, + + TP_PROTO(struct Qdisc *q), + + TP_ARGS(q), + + TP_STRUCT__entry( + __string( dev, qdisc_dev(q) ) + __string( kind, q->ops->id ) + __field( u32, parent ) + __field( u32, handle ) + ), + + TP_fast_assign( + __assign_str(dev, qdisc_dev(q)); + __assign_str(kind, q->ops->id); + __entry->parent = q->parent; + __entry->handle = q->handle; + ), + + TP_printk("dev=%s kind=%s parent=%x:%x handle=%x:%x", __get_str(dev), + __get_str(kind), TC_H_MAJ(__entry->parent) >> 16, TC_H_MIN(__entry->parent), + TC_H_MAJ(__entry->handle) >> 16, TC_H_MIN(__entry->handle)) +); + +TRACE_EVENT(qdisc_destroy, + + TP_PROTO(struct Qdisc *q), + + TP_ARGS(q), + + TP_STRUCT__entry( + __string( dev, qdisc_dev(q) ) + __string( kind, q->ops->id ) + __field( u32, parent ) + __field( u32, handle ) + ), + + TP_fast_assign( + __assign_str(dev, qdisc_dev(q)); + __assign_str(kind, q->ops->id); + __entry->parent = q->parent; + __entry->handle = q->handle; + ), + + TP_printk("dev=%s kind=%s parent=%x:%x handle=%x:%x", __get_str(dev), + __get_str(kind), TC_H_MAJ(__entry->parent) >> 16, TC_H_MIN(__entry->parent), + TC_H_MAJ(__entry->handle) >> 16, TC_H_MIN(__entry->handle)) +); + #endif /* _TRACE_QDISC_H */ /* This part must be outside protection */ diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index 7a0b06001e48..abaa446ed01a 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -911,6 +911,8 @@ void qdisc_reset(struct Qdisc *qdisc) const struct Qdisc_ops *ops = qdisc->ops; struct sk_buff *skb, *tmp; + trace_qdisc_reset(qdisc); + if (ops->reset) ops->reset(qdisc); @@ -965,6 +967,8 @@ static void qdisc_destroy(struct Qdisc *qdisc) module_put(ops->owner); dev_put(qdisc_dev(qdisc)); + trace_qdisc_destroy(qdisc); + call_rcu(&qdisc->rcu, qdisc_free_cb); } -- cgit From f5a7833e83628f18c1ee94e6ffcb1d232f029be9 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 26 May 2020 21:35:25 -0700 Subject: net_sched: add a tracepoint for qdisc creation With this tracepoint, we could know when qdisc's are created, especially those default qdisc's. Sample output: tc-736 [001] ...1 56.230107: qdisc_create: dev=ens3 kind=pfifo parent=1:0 tc-736 [001] ...1 56.230113: qdisc_create: dev=ens3 kind=hfsc parent=ffff:ffff tc-738 [001] ...1 56.256816: qdisc_create: dev=ens3 kind=pfifo parent=1:100 tc-739 [001] ...1 56.267584: qdisc_create: dev=ens3 kind=pfifo parent=1:200 tc-740 [001] ...1 56.279649: qdisc_create: dev=ens3 kind=fq_codel parent=1:100 tc-741 [001] ...1 56.289996: qdisc_create: dev=ens3 kind=pfifo_fast parent=1:200 tc-745 [000] .N.1 111.687483: qdisc_create: dev=ens3 kind=ingress parent=ffff:fff1 Cc: Jamal Hadi Salim Cc: Jiri Pirko Signed-off-by: Cong Wang Signed-off-by: David S. Miller --- include/trace/events/qdisc.h | 23 +++++++++++++++++++++++ net/sched/sch_api.c | 3 +++ net/sched/sch_generic.c | 4 +++- 3 files changed, 29 insertions(+), 1 deletion(-) diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h index 2b948801afa3..330d32d84485 100644 --- a/include/trace/events/qdisc.h +++ b/include/trace/events/qdisc.h @@ -96,6 +96,29 @@ TRACE_EVENT(qdisc_destroy, TC_H_MAJ(__entry->handle) >> 16, TC_H_MIN(__entry->handle)) ); +TRACE_EVENT(qdisc_create, + + TP_PROTO(const struct Qdisc_ops *ops, struct net_device *dev, u32 parent), + + TP_ARGS(ops, dev, parent), + + TP_STRUCT__entry( + __string( dev, dev->name ) + __string( kind, ops->id ) + __field( u32, parent ) + ), + + TP_fast_assign( + __assign_str(dev, dev->name); + __assign_str(kind, ops->id); + __entry->parent = parent; + ), + + TP_printk("dev=%s kind=%s parent=%x:%x", + __get_str(dev), __get_str(kind), + TC_H_MAJ(__entry->parent) >> 16, TC_H_MIN(__entry->parent)) +); + #endif /* _TRACE_QDISC_H */ /* This part must be outside protection */ diff --git a/net/sched/sch_api.c b/net/sched/sch_api.c index 0d99df1e764d..9a3449b56bd6 100644 --- a/net/sched/sch_api.c +++ b/net/sched/sch_api.c @@ -32,6 +32,8 @@ #include #include +#include + /* Short review. @@ -1283,6 +1285,7 @@ static struct Qdisc *qdisc_create(struct net_device *dev, } qdisc_hash_add(sch, false); + trace_qdisc_create(ops, dev, parent); return sch; diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index abaa446ed01a..a4271e47f220 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -896,8 +896,10 @@ struct Qdisc *qdisc_create_dflt(struct netdev_queue *dev_queue, } sch->parent = parentid; - if (!ops->init || ops->init(sch, NULL, extack) == 0) + if (!ops->init || ops->init(sch, NULL, extack) == 0) { + trace_qdisc_create(ops, dev_queue->dev, parentid); return sch; + } qdisc_put(sch); return NULL; -- cgit From 70f50965338a12e17454c31ad5ece27069719358 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 26 May 2020 21:35:26 -0700 Subject: net_sched: avoid resetting active qdisc for multiple times MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Except for sch_mq and sch_mqprio, each dev queue points to the same root qdisc, so when we reset the dev queues with netdev_for_each_tx_queue() we end up resetting the same instance of the root qdisc for multiple times. Avoid this by checking the __QDISC_STATE_DEACTIVATED bit in each iteration, so for sch_mq/sch_mqprio, we still reset all of them like before, for the rest, we only reset it once. Reported-by: Václav Zindulka Tested-by: Václav Zindulka Cc: Jamal Hadi Salim Cc: Jiri Pirko Signed-off-by: Cong Wang Signed-off-by: David S. Miller --- net/sched/sch_generic.c | 37 +++++++++++++++++++++++-------------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index a4271e47f220..d13e27467470 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -1128,6 +1128,28 @@ void dev_activate(struct net_device *dev) } EXPORT_SYMBOL(dev_activate); +static void qdisc_deactivate(struct Qdisc *qdisc) +{ + bool nolock = qdisc->flags & TCQ_F_NOLOCK; + + if (qdisc->flags & TCQ_F_BUILTIN) + return; + if (test_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state)) + return; + + if (nolock) + spin_lock_bh(&qdisc->seqlock); + spin_lock_bh(qdisc_lock(qdisc)); + + set_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state); + + qdisc_reset(qdisc); + + spin_unlock_bh(qdisc_lock(qdisc)); + if (nolock) + spin_unlock_bh(&qdisc->seqlock); +} + static void dev_deactivate_queue(struct net_device *dev, struct netdev_queue *dev_queue, void *_qdisc_default) @@ -1137,21 +1159,8 @@ static void dev_deactivate_queue(struct net_device *dev, qdisc = rtnl_dereference(dev_queue->qdisc); if (qdisc) { - bool nolock = qdisc->flags & TCQ_F_NOLOCK; - - if (nolock) - spin_lock_bh(&qdisc->seqlock); - spin_lock_bh(qdisc_lock(qdisc)); - - if (!(qdisc->flags & TCQ_F_BUILTIN)) - set_bit(__QDISC_STATE_DEACTIVATED, &qdisc->state); - + qdisc_deactivate(qdisc); rcu_assign_pointer(dev_queue->qdisc, qdisc_default); - qdisc_reset(qdisc); - - spin_unlock_bh(qdisc_lock(qdisc)); - if (nolock) - spin_unlock_bh(&qdisc->seqlock); } } -- cgit From 759ae57f1b7bf5dfea1816f786f568d69efe34e2 Mon Sep 17 00:00:00 2001 From: Cong Wang Date: Tue, 26 May 2020 21:35:27 -0700 Subject: net_sched: get rid of unnecessary dev_qdisc_reset() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Resetting old qdisc on dev_queue->qdisc_sleeping in dev_qdisc_reset() is redundant, because this qdisc, even if not same with dev_queue->qdisc, is reset via qdisc_put() right after calling dev_graft_qdisc() when hitting refcnt 0. This is very easy to observe with qdisc_reset() tracepoint and stack traces. Reported-by: Václav Zindulka Tested-by: Václav Zindulka Cc: Jamal Hadi Salim Cc: Jiri Pirko Signed-off-by: Cong Wang Signed-off-by: David S. Miller --- net/sched/sch_generic.c | 16 ---------------- 1 file changed, 16 deletions(-) diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c index d13e27467470..b19a0021a0bd 100644 --- a/net/sched/sch_generic.c +++ b/net/sched/sch_generic.c @@ -1191,16 +1191,6 @@ static bool some_qdisc_is_busy(struct net_device *dev) return false; } -static void dev_qdisc_reset(struct net_device *dev, - struct netdev_queue *dev_queue, - void *none) -{ - struct Qdisc *qdisc = dev_queue->qdisc_sleeping; - - if (qdisc) - qdisc_reset(qdisc); -} - /** * dev_deactivate_many - deactivate transmissions on several devices * @head: list of devices to deactivate @@ -1237,12 +1227,6 @@ void dev_deactivate_many(struct list_head *head) */ schedule_timeout_uninterruptible(1); } - /* The new qdisc is assigned at this point so we can safely - * unwind stale skb lists and qdisc statistics - */ - netdev_for_each_tx_queue(dev, dev_qdisc_reset, NULL); - if (dev_ingress_queue(dev)) - dev_qdisc_reset(dev, dev_ingress_queue(dev), NULL); } } -- cgit