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

On Mon, Jul 12, 2021 at 9:47 AM Cong Wang [off-list ref] wrote:

On Sun, Jul 11, 2021 at 5:50 PM Tonghao Zhang [off-list ref] wrote:

quoted

On Mon, Jul 12, 2021 at 3:03 AM Cong Wang [off-list ref] 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;
+       ),

Hi qitao, cong
Why not support the txq, we get more info from txq.

Because we only want to calculate queueing latency, not anything
else. If you need it, you are welcome to add anything reasonable
in the future, it won't break ABI (see 3dd344ea84e122f791ab).

Thanks!

quoted

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.

Disagree. Because we really have no interest in dropped packets.
Even if we really do, we could trace kfree_skb(), not really here.

The qdisc returns not only the NET_XMIT_DROP, right ?
skbprio_enqueue, sfq_enqueue and red_enqueue may return the NET_XMIT_CN.

quoted

we should introduce a new function to instead of now codes, that may
make the codes clean.  Please review my patch for more info.

Just 3 lines of code, it is totally personal taste.

quoted

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

I did review it. Like I said, %p does not work. Have you tested your
patches? ;)

Yes, I tested them, I didn't find the error.  my patch is based on
commit id 89212e160b81e778f829b89743570665810e3b13

Thanks.

-- 
Best regards, Tonghao