All of lore.kernel.org
 help / color / mirror / Atom feed
* [Patch net-next 0/5] net_sched: reduce the number of qdisc resets
@ 2020-05-27  4:35 Cong Wang
  2020-05-27  4:35 ` [Patch net-next 1/5] net_sched: use qdisc_reset() in qdisc_destroy() Cong Wang
                   ` (5 more replies)
  0 siblings, 6 replies; 8+ messages in thread
From: Cong Wang @ 2020-05-27  4:35 UTC (permalink / raw)
  To: netdev; +Cc: vaclav.zindulka, Cong Wang

This patchset aims to reduce the number of qdisc resets during
qdisc tear down. Patch 1~3 are preparation for their following
patches, especially patch 2 and patch 3 add a few tracepoints
so that we can observe the whole lifetime of qdisc's. Patch 4
and patch 5 are the ones do the actual work. Please find more
details in each patch description.

Vaclav Zindulka tested this patchset and his large ruleset with
over 13k qdiscs defined got from 22s to 520ms.

---

Cong Wang (5):
  net_sched: use qdisc_reset() in qdisc_destroy()
  net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()
  net_sched: add a tracepoint for qdisc creation
  net_sched: avoid resetting active qdisc for multiple times
  net_sched: get rid of unnecessary dev_qdisc_reset()

 include/trace/events/qdisc.h | 75 ++++++++++++++++++++++++++++++++++++
 net/sched/sch_api.c          |  3 ++
 net/sched/sch_generic.c      | 75 +++++++++++++++---------------------
 3 files changed, 110 insertions(+), 43 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [Patch net-next 1/5] net_sched: use qdisc_reset() in qdisc_destroy()
  2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
@ 2020-05-27  4:35 ` Cong Wang
  2020-05-27  4:35 ` [Patch net-next 2/5] net_sched: add tracepoints for qdisc_reset() and qdisc_destroy() Cong Wang
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2020-05-27  4:35 UTC (permalink / raw)
  To: netdev; +Cc: vaclav.zindulka, Cong Wang, Jamal Hadi Salim, Jiri Pirko

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 <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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);
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Patch net-next 2/5] net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()
  2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
  2020-05-27  4:35 ` [Patch net-next 1/5] net_sched: use qdisc_reset() in qdisc_destroy() Cong Wang
@ 2020-05-27  4:35 ` Cong Wang
  2024-02-29 19:05   ` Steven Rostedt
  2020-05-27  4:35 ` [Patch net-next 3/5] net_sched: add a tracepoint for qdisc creation Cong Wang
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 8+ messages in thread
From: Cong Wang @ 2020-05-27  4:35 UTC (permalink / raw)
  To: netdev; +Cc: vaclav.zindulka, Cong Wang, Jamal Hadi Salim, Jiri Pirko

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 <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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 <linux/netdevice.h>
 #include <linux/tracepoint.h>
 #include <linux/ftrace.h>
+#include <linux/pkt_sched.h>
+#include <net/sch_generic.h>
 
 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);
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Patch net-next 3/5] net_sched: add a tracepoint for qdisc creation
  2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
  2020-05-27  4:35 ` [Patch net-next 1/5] net_sched: use qdisc_reset() in qdisc_destroy() Cong Wang
  2020-05-27  4:35 ` [Patch net-next 2/5] net_sched: add tracepoints for qdisc_reset() and qdisc_destroy() Cong Wang
@ 2020-05-27  4:35 ` Cong Wang
  2020-05-27  4:35 ` [Patch net-next 4/5] net_sched: avoid resetting active qdisc for multiple times Cong Wang
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2020-05-27  4:35 UTC (permalink / raw)
  To: netdev; +Cc: vaclav.zindulka, Cong Wang, Jamal Hadi Salim, Jiri Pirko

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 <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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 <net/pkt_sched.h>
 #include <net/pkt_cls.h>
 
+#include <trace/events/qdisc.h>
+
 /*
 
    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;
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Patch net-next 4/5] net_sched: avoid resetting active qdisc for multiple times
  2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
                   ` (2 preceding siblings ...)
  2020-05-27  4:35 ` [Patch net-next 3/5] net_sched: add a tracepoint for qdisc creation Cong Wang
@ 2020-05-27  4:35 ` Cong Wang
  2020-05-27  4:35 ` [Patch net-next 5/5] net_sched: get rid of unnecessary dev_qdisc_reset() Cong Wang
  2020-05-27 22:06 ` [Patch net-next 0/5] net_sched: reduce the number of qdisc resets David Miller
  5 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2020-05-27  4:35 UTC (permalink / raw)
  To: netdev; +Cc: vaclav.zindulka, Cong Wang, Jamal Hadi Salim, Jiri Pirko

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 <vaclav.zindulka@tlapnet.cz>
Tested-by: Václav Zindulka <vaclav.zindulka@tlapnet.cz>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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);
 	}
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [Patch net-next 5/5] net_sched: get rid of unnecessary dev_qdisc_reset()
  2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
                   ` (3 preceding siblings ...)
  2020-05-27  4:35 ` [Patch net-next 4/5] net_sched: avoid resetting active qdisc for multiple times Cong Wang
@ 2020-05-27  4:35 ` Cong Wang
  2020-05-27 22:06 ` [Patch net-next 0/5] net_sched: reduce the number of qdisc resets David Miller
  5 siblings, 0 replies; 8+ messages in thread
From: Cong Wang @ 2020-05-27  4:35 UTC (permalink / raw)
  To: netdev; +Cc: vaclav.zindulka, Cong Wang, Jamal Hadi Salim, Jiri Pirko

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 <vaclav.zindulka@tlapnet.cz>
Tested-by: Václav Zindulka <vaclav.zindulka@tlapnet.cz>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 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);
 	}
 }
 
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [Patch net-next 0/5] net_sched: reduce the number of qdisc resets
  2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
                   ` (4 preceding siblings ...)
  2020-05-27  4:35 ` [Patch net-next 5/5] net_sched: get rid of unnecessary dev_qdisc_reset() Cong Wang
@ 2020-05-27 22:06 ` David Miller
  5 siblings, 0 replies; 8+ messages in thread
From: David Miller @ 2020-05-27 22:06 UTC (permalink / raw)
  To: xiyou.wangcong; +Cc: netdev, vaclav.zindulka

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 26 May 2020 21:35:22 -0700

> This patchset aims to reduce the number of qdisc resets during
> qdisc tear down. Patch 1~3 are preparation for their following
> patches, especially patch 2 and patch 3 add a few tracepoints
> so that we can observe the whole lifetime of qdisc's. Patch 4
> and patch 5 are the ones do the actual work. Please find more
> details in each patch description.
> 
> Vaclav Zindulka tested this patchset and his large ruleset with
> over 13k qdiscs defined got from 22s to 520ms.

Series applied, thanks Cong.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Patch net-next 2/5] net_sched: add tracepoints for qdisc_reset() and qdisc_destroy()
  2020-05-27  4:35 ` [Patch net-next 2/5] net_sched: add tracepoints for qdisc_reset() and qdisc_destroy() Cong Wang
@ 2024-02-29 19:05   ` Steven Rostedt
  0 siblings, 0 replies; 8+ messages in thread
From: Steven Rostedt @ 2024-02-29 19:05 UTC (permalink / raw)
  To: Cong Wang; +Cc: netdev, vaclav.zindulka, Jamal Hadi Salim, Jiri Pirko

On Tue, 26 May 2020 21:35:24 -0700
Cong Wang <xiyou.wangcong@gmail.com> wrote:

> 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 <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  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 <linux/netdevice.h>
>  #include <linux/tracepoint.h>
>  #include <linux/ftrace.h>
> +#include <linux/pkt_sched.h>
> +#include <net/sch_generic.h>
>  
>  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));

I'm doing updates to __assign_str() and __string() and the below errored
out because "qdisc_dev(q)" is not a string.

How does the above work?

static inline struct net_device *qdisc_dev(const struct Qdisc *qdisc)
{
	return qdisc->dev_queue->dev;
}

Where:


struct net_device {
	/* Cacheline organization can be found documented in
	 * Documentation/networking/net_cachelines/net_device.rst.
	 * Please update the document when adding new fields.
	 */

	/* TX read-mostly hotpath */
	__cacheline_group_begin(net_device_read_tx);
	unsigned long long	priv_flags;
	const struct net_device_ops *netdev_ops;

What looks to be returned from qdisc_dev() is not a string??

Is this a bug? You don't really expect this to work do you?

-- Steve



> +		__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);
>  }
>  


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2024-02-29 19:03 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27  4:35 [Patch net-next 0/5] net_sched: reduce the number of qdisc resets Cong Wang
2020-05-27  4:35 ` [Patch net-next 1/5] net_sched: use qdisc_reset() in qdisc_destroy() Cong Wang
2020-05-27  4:35 ` [Patch net-next 2/5] net_sched: add tracepoints for qdisc_reset() and qdisc_destroy() Cong Wang
2024-02-29 19:05   ` Steven Rostedt
2020-05-27  4:35 ` [Patch net-next 3/5] net_sched: add a tracepoint for qdisc creation Cong Wang
2020-05-27  4:35 ` [Patch net-next 4/5] net_sched: avoid resetting active qdisc for multiple times Cong Wang
2020-05-27  4:35 ` [Patch net-next 5/5] net_sched: get rid of unnecessary dev_qdisc_reset() Cong Wang
2020-05-27 22:06 ` [Patch net-next 0/5] net_sched: reduce the number of qdisc resets David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.