Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
@ 2021-07-11 19:03 Cong Wang
  2021-07-12  0:49 ` Tonghao Zhang
  2021-07-12  3:01 ` Yunsheng Lin
  0 siblings, 2 replies; 18+ messages in thread
From: Cong Wang @ 2021-07-11 19:03 UTC (permalink / raw)
  To: netdev; +Cc: xiangxia.m.yue, Qitao Xu, Cong Wang, Jamal Hadi Salim, Jiri Pirko

From: Qitao Xu <qitao.xu@bytedance.com>

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 <cong.wang@bytedance.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
Cc: Jiri Pirko <jiri@resnulli.us>
Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
---
 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)
+					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


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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-11 19:03 [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue() Cong Wang
@ 2021-07-12  0:49 ` Tonghao Zhang
  2021-07-12  1:47   ` Cong Wang
  2021-07-12  3:01 ` Yunsheng Lin
  1 sibling, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  0:49 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 3:03 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> From: Qitao Xu <qitao.xu@bytedance.com>
>
> 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 <cong.wang@bytedance.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> ---
>  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/

> +       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

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  0:49 ` Tonghao Zhang
@ 2021-07-12  1:47   ` Cong Wang
  2021-07-12  3:01     ` Tonghao Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Cong Wang @ 2021-07-12  1:47 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 5:50 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 3:03 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > From: Qitao Xu <qitao.xu@bytedance.com>
> >
> > 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 <cong.wang@bytedance.com>
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>
> > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > ---
> >  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).

> 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.

> 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.

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

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-11 19:03 [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue() Cong Wang
  2021-07-12  0:49 ` Tonghao Zhang
@ 2021-07-12  3:01 ` Yunsheng Lin
  2021-07-12  3:06   ` Tonghao Zhang
  2021-07-12  3:34   ` Cong Wang
  1 sibling, 2 replies; 18+ messages in thread
From: Yunsheng Lin @ 2021-07-12  3:01 UTC (permalink / raw)
  To: Cong Wang, netdev
  Cc: xiangxia.m.yue, Qitao Xu, Cong Wang, Jamal Hadi Salim, Jiri Pirko

On 2021/7/12 3:03, Cong Wang wrote:
> From: Qitao Xu <qitao.xu@bytedance.com>
> 
> 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 <cong.wang@bytedance.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> ---
>  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.

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

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  1:47   ` Cong Wang
@ 2021-07-12  3:01     ` Tonghao Zhang
  2021-07-12  3:23       ` Cong Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  3:01 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 9:47 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 5:50 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 3:03 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > From: Qitao Xu <qitao.xu@bytedance.com>
> > >
> > > 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 <cong.wang@bytedance.com>
> > > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > > Cc: Jiri Pirko <jiri@resnulli.us>
> > > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > > ---
> > >  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!
> > 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.


> > 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.
>
> > 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

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:01 ` Yunsheng Lin
@ 2021-07-12  3:06   ` Tonghao Zhang
  2021-07-12  3:34   ` Cong Wang
  1 sibling, 0 replies; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  3:06 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: Cong Wang, Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 11:01 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2021/7/12 3:03, Cong Wang wrote:
> > From: Qitao Xu <qitao.xu@bytedance.com>
> >
> > 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 <cong.wang@bytedance.com>
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>
> > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > ---
> >  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.
> > +                                     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.
>
> >                               __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

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:01     ` Tonghao Zhang
@ 2021-07-12  3:23       ` Cong Wang
  2021-07-12  3:35         ` Tonghao Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Cong Wang @ 2021-07-12  3:23 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 8:02 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 9:47 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Sun, Jul 11, 2021 at 5:50 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 3:03 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > From: Qitao Xu <qitao.xu@bytedance.com>
> > > >
> > > > 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 <cong.wang@bytedance.com>
> > > > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > > > Cc: Jiri Pirko <jiri@resnulli.us>
> > > > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > > > ---
> > > >  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!
> > > 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.
>

Sure, in that case a different packet is dropped, once again you
can trace it with kfree_skb() if you want. What's the problem?

>
> > > 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.
> >
> > > 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

I seriously doubt it, because we actually used %p in the beginning
too and got the same address for two different packets, this is why
we have to move to %px. It is 100% reproducible, so it probably
means you didn't test it at all.

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:01 ` Yunsheng Lin
  2021-07-12  3:06   ` Tonghao Zhang
@ 2021-07-12  3:34   ` Cong Wang
  1 sibling, 0 replies; 18+ messages in thread
From: Cong Wang @ 2021-07-12  3:34 UTC (permalink / raw)
  To: Yunsheng Lin
  Cc: Linux Kernel Network Developers, Tonghao Zhang, Qitao Xu,
	Cong Wang, Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 8:01 PM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2021/7/12 3:03, Cong Wang wrote:
> > From: Qitao Xu <qitao.xu@bytedance.com>
> >
> > 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 <cong.wang@bytedance.com>
> > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > Cc: Jiri Pirko <jiri@resnulli.us>
> > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > ---
> >  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?

Sure. See the other reply from on why dropped packets are not
interesting here.

>
> 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.

Totally disagree, because trace_qdisc_dequeue() is only called for
successful cases too (see dequeue_skb()), it does not make sense
to let them be different.

>
> > +                                     trace_qdisc_enqueue(q, txq, skb);
>
> Does it make sense to wrap the about to something like:

Nope. Because ->enqueue() is called by lower layer qdisc's
too, but here we only want to track root, aka, entrance of TC.
I know this may be confusing, please blame trace_qdisc_dequeue()
which only tracks the exit. ;)

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:23       ` Cong Wang
@ 2021-07-12  3:35         ` Tonghao Zhang
  2021-07-12  3:39           ` Cong Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  3:35 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 8:02 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 9:47 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Sun, Jul 11, 2021 at 5:50 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 12, 2021 at 3:03 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > From: Qitao Xu <qitao.xu@bytedance.com>
> > > > >
> > > > > 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 <cong.wang@bytedance.com>
> > > > > Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> > > > > Cc: Jiri Pirko <jiri@resnulli.us>
> > > > > Signed-off-by: Qitao Xu <qitao.xu@bytedance.com>
> > > > > ---
> > > > >  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!
> > > > 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.
> >
>
> Sure, in that case a different packet is dropped, once again you
> can trace it with kfree_skb() if you want. What's the problem?
It's ok, but we can make it better. Yunsheng Lin may have explained why?
> >
> > > > 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.
> > >
> > > > 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
>
> I seriously doubt it, because we actually used %p in the beginning
> too and got the same address for two different packets, this is why
> we have to move to %px. It is 100% reproducible, so it probably
> means you didn't test it at all.
I use iperf to send the packets, not found the same address really

          <idle>-0       [043] ..s.    62.493634: qdisc_enqueue:
enqueue ifindex=2 qdisc handle=0x0 parent=0x2C
skbaddr=00000000a40f93fb ret=0
          <idle>-0       [043] ..s.    62.494641: qdisc_enqueue:
enqueue ifindex=2 qdisc handle=0x0 parent=0x20
skbaddr=00000000c3c53e95 ret=0
          <idle>-0       [014] ..s.    64.473877: qdisc_enqueue:
enqueue ifindex=2 qdisc handle=0x0 parent=0x20
skbaddr=00000000ad610424 ret=0
          <idle>-0       [014] ..s.    64.473896: qdisc_enqueue:
enqueue ifindex=2 qdisc handle=0x0 parent=0x20
skbaddr=00000000112a562d ret=0

>
> Thanks.



-- 
Best regards, Tonghao

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:35         ` Tonghao Zhang
@ 2021-07-12  3:39           ` Cong Wang
  2021-07-12  3:48             ` Tonghao Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Cong Wang @ 2021-07-12  3:39 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > Sure, in that case a different packet is dropped, once again you
> > can trace it with kfree_skb() if you want. What's the problem?
> It's ok, but we can make it better. Yunsheng Lin may have explained why?

Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
I fail to see it. You are just asking for duplications. If you do not see it by
yourself, it means you don't understand or need it at all. ;)

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:39           ` Cong Wang
@ 2021-07-12  3:48             ` Tonghao Zhang
  2021-07-12  4:02               ` Cong Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  3:48 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > Sure, in that case a different packet is dropped, once again you
> > > can trace it with kfree_skb() if you want. What's the problem?
> > It's ok, but we can make it better. Yunsheng Lin may have explained why?
>
> Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
I mean we can use one tracepoint to know what happened in the queue,
not necessary to trace enqueue and  kfree_skb()
If so, we must match when the packet is dropped and what packets. if
we use the return value in trace_qdisc_requeue. It
is easy to know what happened(when, where, what packets were dropped ).

> I fail to see it. You are just asking for duplications. If you do not see it by
> yourself, it means you don't understand or need it at all. ;)
I added the tracepoint in centos 8 4.18 kernel version in our servers
for a long time.
> Thanks.



-- 
Best regards, Tonghao

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  3:48             ` Tonghao Zhang
@ 2021-07-12  4:02               ` Cong Wang
  2021-07-12  4:12                 ` Tonghao Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Cong Wang @ 2021-07-12  4:02 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > Sure, in that case a different packet is dropped, once again you
> > > > can trace it with kfree_skb() if you want. What's the problem?
> > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> >
> > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> I mean we can use one tracepoint to know what happened in the queue,
> not necessary to trace enqueue and  kfree_skb()

This is wrong, packets can be dropped for other reasons too, tracing
enqueue is clearly not sufficient even if you trace it unconditionally.
For a quick example, codel drops packets at dequeue rather than
enqueue. ;)

> If so, we must match when the packet is dropped and what packets. if
> we use the return value in trace_qdisc_requeue. It
> is easy to know what happened(when, where, what packets were dropped ).

I am afraid you have to watch the dropped packets.

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  4:02               ` Cong Wang
@ 2021-07-12  4:12                 ` Tonghao Zhang
  2021-07-12  4:19                   ` Cong Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  4:12 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > Sure, in that case a different packet is dropped, once again you
> > > > > can trace it with kfree_skb() if you want. What's the problem?
> > > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> > >
> > > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> > I mean we can use one tracepoint to know what happened in the queue,
> > not necessary to trace enqueue and  kfree_skb()
>
> This is wrong, packets can be dropped for other reasons too, tracing
no matter where the packet is dropped, we should allow user to know
whether dropped in the enqueue.  and the
what the return value.
> enqueue is clearly not sufficient even if you trace it unconditionally.
> For a quick example, codel drops packets at dequeue rather than
> enqueue. ;)
>
> > If so, we must match when the packet is dropped and what packets. if
> > we use the return value in trace_qdisc_requeue. It
> > is easy to know what happened(when, where, what packets were dropped ).
>
> I am afraid you have to watch the dropped packets.
>
> Thanks.



-- 
Best regards, Tonghao

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  4:12                 ` Tonghao Zhang
@ 2021-07-12  4:19                   ` Cong Wang
  2021-07-12  4:40                     ` Tonghao Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Cong Wang @ 2021-07-12  4:19 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 9:12 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > Sure, in that case a different packet is dropped, once again you
> > > > > > can trace it with kfree_skb() if you want. What's the problem?
> > > > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> > > >
> > > > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> > > I mean we can use one tracepoint to know what happened in the queue,
> > > not necessary to trace enqueue and  kfree_skb()
> >
> > This is wrong, packets can be dropped for other reasons too, tracing
> no matter where the packet is dropped, we should allow user to know
> whether dropped in the enqueue.  and the
> what the return value.

Again you can know it by kfree_skb(). And you can not avoid
kfree_skb() no matter how you change enqueue. So, I don't see your
argument of saving kfree_skb() makes any sense here.

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  4:19                   ` Cong Wang
@ 2021-07-12  4:40                     ` Tonghao Zhang
  2021-07-12  4:45                       ` Cong Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-12  4:40 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 12:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 9:12 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > > Sure, in that case a different packet is dropped, once again you
> > > > > > > can trace it with kfree_skb() if you want. What's the problem?
> > > > > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> > > > >
> > > > > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> > > > I mean we can use one tracepoint to know what happened in the queue,
> > > > not necessary to trace enqueue and  kfree_skb()
> > >
> > > This is wrong, packets can be dropped for other reasons too, tracing
> > no matter where the packet is dropped, we should allow user to know
> > whether dropped in the enqueue.  and the
> > what the return value.
>
> Again you can know it by kfree_skb(). And you can not avoid
> kfree_skb() no matter how you change enqueue. So, I don't see your
No, If I know what value returned for specified qdisc , I can know
what happened, not necessarily kfree_skb()
> argument of saving kfree_skb() makes any sense here.
>
> Thanks.



-- 
Best regards, Tonghao

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  4:40                     ` Tonghao Zhang
@ 2021-07-12  4:45                       ` Cong Wang
  2021-07-13  1:56                         ` Tonghao Zhang
  0 siblings, 1 reply; 18+ messages in thread
From: Cong Wang @ 2021-07-12  4:45 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Sun, Jul 11, 2021 at 9:41 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 12:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Sun, Jul 11, 2021 at 9:12 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > > > Sure, in that case a different packet is dropped, once again you
> > > > > > > > can trace it with kfree_skb() if you want. What's the problem?
> > > > > > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> > > > > >
> > > > > > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> > > > > I mean we can use one tracepoint to know what happened in the queue,
> > > > > not necessary to trace enqueue and  kfree_skb()
> > > >
> > > > This is wrong, packets can be dropped for other reasons too, tracing
> > > no matter where the packet is dropped, we should allow user to know
> > > whether dropped in the enqueue.  and the
> > > what the return value.
> >
> > Again you can know it by kfree_skb(). And you can not avoid
> > kfree_skb() no matter how you change enqueue. So, I don't see your
> No, If I know what value returned for specified qdisc , I can know
> what happened, not necessarily kfree_skb()

This is wrong. You have to trace dropped packets because you
need to know when to delete the key (skb address) from the hashtable
you use to calculate the latency. You save the key on enqueue and
remove it on both dequeue and kfree_skb, the only difference is you
only need to calculate the timestamp difference for the former.

Thanks.

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-12  4:45                       ` Cong Wang
@ 2021-07-13  1:56                         ` Tonghao Zhang
  2021-07-15  4:28                           ` Cong Wang
  0 siblings, 1 reply; 18+ messages in thread
From: Tonghao Zhang @ 2021-07-13  1:56 UTC (permalink / raw)
  To: Cong Wang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 12:45 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> On Sun, Jul 11, 2021 at 9:41 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> >
> > On Mon, Jul 12, 2021 at 12:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >
> > > On Sun, Jul 11, 2021 at 9:12 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > >
> > > > On Mon, Jul 12, 2021 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > >
> > > > > On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > > >
> > > > > > On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > > > > Sure, in that case a different packet is dropped, once again you
> > > > > > > > > can trace it with kfree_skb() if you want. What's the problem?
> > > > > > > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> > > > > > >
> > > > > > > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> > > > > > I mean we can use one tracepoint to know what happened in the queue,
> > > > > > not necessary to trace enqueue and  kfree_skb()
> > > > >
> > > > > This is wrong, packets can be dropped for other reasons too, tracing
> > > > no matter where the packet is dropped, we should allow user to know
> > > > whether dropped in the enqueue.  and the
> > > > what the return value.
> > >
> > > Again you can know it by kfree_skb(). And you can not avoid
> > > kfree_skb() no matter how you change enqueue. So, I don't see your
> > No, If I know what value returned for specified qdisc , I can know
> > what happened, not necessarily kfree_skb()
>
> This is wrong. You have to trace dropped packets because you
> need to know when to delete the key (skb address) from the hashtable
> you use to calculate the latency. You save the key on enqueue and
> remove it on both dequeue and kfree_skb, the only difference is you
No, we can set the timestamp or cb in  skb in enqueue and check them in dequeue.
we may not use the hashtable.
If we use hashtable, we still can check the return value, save them to
hashtable or not.
> only need to calculate the timestamp difference for the former.
>
> Thanks.



-- 
Best regards, Tonghao

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

* Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()
  2021-07-13  1:56                         ` Tonghao Zhang
@ 2021-07-15  4:28                           ` Cong Wang
  0 siblings, 0 replies; 18+ messages in thread
From: Cong Wang @ 2021-07-15  4:28 UTC (permalink / raw)
  To: Tonghao Zhang
  Cc: Linux Kernel Network Developers, Qitao Xu, Cong Wang,
	Jamal Hadi Salim, Jiri Pirko

On Mon, Jul 12, 2021 at 6:57 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
>
> On Mon, Jul 12, 2021 at 12:45 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >
> > On Sun, Jul 11, 2021 at 9:41 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > >
> > > On Mon, Jul 12, 2021 at 12:20 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > >
> > > > On Sun, Jul 11, 2021 at 9:12 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > >
> > > > > On Mon, Jul 12, 2021 at 12:02 PM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Jul 11, 2021 at 8:49 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > > > >
> > > > > > > On Mon, Jul 12, 2021 at 11:39 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Jul 11, 2021 at 8:36 PM Tonghao Zhang <xiangxia.m.yue@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Jul 12, 2021 at 11:23 AM Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > > > > > > > > Sure, in that case a different packet is dropped, once again you
> > > > > > > > > > can trace it with kfree_skb() if you want. What's the problem?
> > > > > > > > > It's ok, but we can make it better. Yunsheng Lin may have explained why?
> > > > > > > >
> > > > > > > > Why it is better to trace dropped packets both in enqueue and in kfree_skb()?
> > > > > > > I mean we can use one tracepoint to know what happened in the queue,
> > > > > > > not necessary to trace enqueue and  kfree_skb()
> > > > > >
> > > > > > This is wrong, packets can be dropped for other reasons too, tracing
> > > > > no matter where the packet is dropped, we should allow user to know
> > > > > whether dropped in the enqueue.  and the
> > > > > what the return value.
> > > >
> > > > Again you can know it by kfree_skb(). And you can not avoid
> > > > kfree_skb() no matter how you change enqueue. So, I don't see your
> > > No, If I know what value returned for specified qdisc , I can know
> > > what happened, not necessarily kfree_skb()
> >
> > This is wrong. You have to trace dropped packets because you
> > need to know when to delete the key (skb address) from the hashtable
> > you use to calculate the latency. You save the key on enqueue and
> > remove it on both dequeue and kfree_skb, the only difference is you
> No, we can set the timestamp or cb in  skb in enqueue and check them in dequeue.
> we may not use the hashtable.

Are you sure this is safe?? How do you ensure we have enough space
in skb->cb[]? More importantly, why do we even modify skb in tracepoint?

> If we use hashtable, we still can check the return value, save them to
> hashtable or not.

How many times do I have to repeat the return value of enqueue is not
sufficient even if you trace it? Can't you just look at codel_dequeue()
and tell me how a drop at dequeue can be reflected to enqueue?

Thanks.

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

end of thread, other threads:[~2021-07-15  4:28 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-11 19:03 [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue() Cong Wang
2021-07-12  0:49 ` Tonghao Zhang
2021-07-12  1:47   ` Cong Wang
2021-07-12  3:01     ` Tonghao Zhang
2021-07-12  3:23       ` Cong Wang
2021-07-12  3:35         ` Tonghao Zhang
2021-07-12  3:39           ` Cong Wang
2021-07-12  3:48             ` Tonghao Zhang
2021-07-12  4:02               ` Cong Wang
2021-07-12  4:12                 ` Tonghao Zhang
2021-07-12  4:19                   ` Cong Wang
2021-07-12  4:40                     ` Tonghao Zhang
2021-07-12  4:45                       ` Cong Wang
2021-07-13  1:56                         ` Tonghao Zhang
2021-07-15  4:28                           ` Cong Wang
2021-07-12  3:01 ` Yunsheng Lin
2021-07-12  3:06   ` Tonghao Zhang
2021-07-12  3:34   ` Cong Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).