Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
From: Yunsheng Lin <hidden>
Date: 2021-07-12 03:01:09
On 2021/7/12 3:03, Cong Wang 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; + ), + + 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)
If NET_XMIT_CN is returned, the skb seems to be enqueued too? Also instead of checking the rc before calling the trace_*, maybe it make more sense to add the rc to the tracepoint, so that the checking is avoided, and we are able to tell the enqueuing result of a specific skb from that tracepoint too.
+ trace_qdisc_enqueue(q, txq, skb);
Does it make sense to wrap the about to something like:
int sch_enqueue(....)
{
rc = q->enqueue(skb, q, &to_free)..
....
trace_qdisc_enqueue(q, txq, skb);
}
So that the below code can reuse that wrapper too.
quoted hunk ↗ jump to hunk
__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);