Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
From: Tonghao Zhang <hidden>
Date: 2021-07-12 00:51:25
On Mon, Jul 12, 2021 at 3:03 AM Cong Wang [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Qitao Xu <redacted> Tracepoint trace_qdisc_enqueue() is introduced to trace skb at the entrance of TC layer on TX side. This is kinda symmetric to trace_qdisc_dequeue(), and together they can be used to calculate the packet queueing latency. It is more accurate than trace_net_dev_queue(), because we already successfully enqueue the packet at that point. Note, trace ring buffer is only accessible to privileged users, it is safe to use %px to print a real kernel address here. Reviewed-by: Cong Wang <redacted> Cc: Jamal Hadi Salim <jhs@mojatatu.com> Cc: Jiri Pirko <jiri@resnulli.us> Signed-off-by: Qitao Xu <redacted> --- include/trace/events/qdisc.h | 26 ++++++++++++++++++++++++++ net/core/dev.c | 9 +++++++++ 2 files changed, 35 insertions(+)diff --git a/include/trace/events/qdisc.h b/include/trace/events/qdisc.h index 58209557cb3a..c3006c6b4a87 100644 --- a/include/trace/events/qdisc.h +++ b/include/trace/events/qdisc.h@@ -46,6 +46,32 @@ TRACE_EVENT(qdisc_dequeue, __entry->txq_state, __entry->packets, __entry->skbaddr ) ); +TRACE_EVENT(qdisc_enqueue, + + TP_PROTO(struct Qdisc *qdisc, const struct netdev_queue *txq, struct sk_buff *skb), + + TP_ARGS(qdisc, txq, skb), + + TP_STRUCT__entry( + __field(struct Qdisc *, qdisc) + __field(void *, skbaddr) + __field(int, ifindex) + __field(u32, handle) + __field(u32, parent) + ), + + TP_fast_assign( + __entry->qdisc = qdisc; + __entry->skbaddr = skb; + __entry->ifindex = txq->dev ? txq->dev->ifindex : 0; + __entry->handle = qdisc->handle; + __entry->parent = qdisc->parent; + ),
Hi qitao, cong Why not support the txq, we get more info from txq. and we should take care of the return value of q->enqueue, because we can know what happens in the qdisc queue(not necessary to work with qdisc:dequeue). and we can use a tracepoint filter for the return value too. we should introduce a new function to instead of now codes, that may make the codes clean. Please review my patch for more info. https://patchwork.kernel.org/project/netdevbpf/patch/20210711050007.1200-1-xiangxia.m.yue@gmail.com/
quoted hunk ↗ jump to hunk
+ TP_printk("enqueue ifindex=%d qdisc handle=0x%X parent=0x%X skbaddr=%px", + __entry->ifindex, __entry->handle, __entry->parent, __entry->skbaddr) +); + TRACE_EVENT(qdisc_reset, TP_PROTO(struct Qdisc *q),diff --git a/net/core/dev.c b/net/core/dev.c index c253c2aafe97..20b9376de301 100644 --- a/net/core/dev.c +++ b/net/core/dev.c@@ -131,6 +131,7 @@ #include <trace/events/napi.h> #include <trace/events/net.h> #include <trace/events/skb.h> +#include <trace/events/qdisc.h> #include <linux/inetdevice.h> #include <linux/cpu_rmap.h> #include <linux/static_key.h>@@ -3864,6 +3865,8 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, if (unlikely(!nolock_qdisc_is_empty(q))) { rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK; + if (rc == NET_XMIT_SUCCESS) + trace_qdisc_enqueue(q, txq, skb); __qdisc_run(q); qdisc_run_end(q);@@ -3880,6 +3883,9 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, } rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK; + if (rc == NET_XMIT_SUCCESS) + trace_qdisc_enqueue(q, txq, skb); + qdisc_run(q); no_lock_out:@@ -3924,6 +3930,9 @@ static inline int __dev_xmit_skb(struct sk_buff *skb, struct Qdisc *q, rc = NET_XMIT_SUCCESS; } else { rc = q->enqueue(skb, q, &to_free) & NET_XMIT_MASK; + if (rc == NET_XMIT_SUCCESS) + trace_qdisc_enqueue(q, txq, skb); + if (qdisc_run_begin(q)) { if (unlikely(contended)) { spin_unlock(&q->busylock); --2.27.0
-- Best regards, Tonghao