Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()

On Mon, Jul 12, 2021 at 11:01 AM Yunsheng Lin [off-list ref] wrote:

On 2021/7/12 3:03, Cong Wang wrote:

quoted

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.

Yes, I will fix it.

quoted

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

Yes, I agree, my patch uses qdisc_enqueue_skb, because __dev_xmit_skb
invoke the qdisc_xxx api.

https://patchwork.kernel.org/project/netdevbpf/patch/20210711050007.1200-1-xiangxia.m.yue@gmail.com/

So that the below code can reuse that wrapper too.

quoted

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

-- 
Best regards, Tonghao