netdev - Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAM_iQpUmO8S-7cgG=yy2TsNoc9nHpW__-WCfhcfV-_eAf46K7A@mail.gmail.com>
Date:   Sun, 11 Jul 2021 18:47:30 -0700
From:   Cong Wang <xiyou.wangcong@...il.com>
To:     Tonghao Zhang <xiangxia.m.yue@...il.com>
Cc:     Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Qitao Xu <qitao.xu@...edance.com>,
        Cong Wang <cong.wang@...edance.com>,
        Jamal Hadi Salim <jhs@...atatu.com>,
        Jiri Pirko <jiri@...nulli.us>
Subject: Re: [Patch net-next v2] net_sched: introduce tracepoint trace_qdisc_enqueue()

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