Subject: RE: [PATCH bpf-next v4 1/6] xsk: add tracepoints for packet drops

From: "Loftus, Ciara" <[email-masked]>
Date: Mon, 8 Feb 2021 09:11:45 +0000

> >
> > This commit introduces tracing infrastructure for AF_XDP sockets
> > (xsks) and a new trace event called 'xsk_packet_drop'. This trace
> > event is triggered when a packet cannot be processed by the socket
> > due to one of the following issues:
> > (1) packet exceeds the maximum permitted size.
> > (2) invalid fill descriptor address.
> > (3) invalid tx descriptor field.
> >
> > The trace provides information about the error to the user. For
> > example the size vs permitted size is provided for (1). For (2)
> > and (3) the relevant descriptor fields are printed. This information
> > should help a user troubleshoot packet drops by providing this extra
> > level of detail which is not available through use of simple counters.
> >
> > The tracepoint can be enabled/disabled by toggling
> > /sys/kernel/debug/tracing/events/xsk/xsk_packet_drop/enable
> >
> > Signed-off-by: Ciara Loftus <[email-masked]>
> > ---
> >  MAINTAINERS                       |  1 +
> >  include/linux/bpf_trace.h         |  1 +
> >  include/trace/events/xsk.h        | 73
> +++++++++++++++++++++++++++++++
> >  include/uapi/linux/if_xdp.h       |  6 +++
> >  kernel/bpf/core.c                 |  1 +
> >  net/xdp/xsk.c                     |  7 ++-
> >  net/xdp/xsk_buff_pool.c           |  3 ++
> >  net/xdp/xsk_queue.h               |  4 ++
> >  tools/include/uapi/linux/if_xdp.h |  6 +++
> >  9 files changed, 101 insertions(+), 1 deletion(-)
> >  create mode 100644 include/trace/events/xsk.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 1df56a32d2df..efe6662d4198 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -19440,6 +19440,7 @@ S:      Maintained
> >  F:     Documentation/networking/af_xdp.rst
> >  F:     include/net/xdp_sock*
> >  F:     include/net/xsk_buff_pool.h
> > +F:     include/trace/events/xsk.h
> >  F:     include/uapi/linux/if_xdp.h
> >  F:     include/uapi/linux/xdp_diag.h
> >  F:     include/net/netns/xdp.h
> > diff --git a/include/linux/bpf_trace.h b/include/linux/bpf_trace.h
> > index ddf896abcfb6..477d29b6c2c1 100644
> > --- a/include/linux/bpf_trace.h
> > +++ b/include/linux/bpf_trace.h
> > @@ -3,5 +3,6 @@
> >  #define __LINUX_BPF_TRACE_H__
> >
> >  #include <trace/events/xdp.h>
> > +#include <trace/events/xsk.h>
> >
> >  #endif /* __LINUX_BPF_TRACE_H__ */
> > diff --git a/include/trace/events/xsk.h b/include/trace/events/xsk.h
> > new file mode 100644
> > index 000000000000..e2984fad372c
> > --- /dev/null
> > +++ b/include/trace/events/xsk.h
> > @@ -0,0 +1,73 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/* Copyright(c) 2021 Intel Corporation. */
> > +
> > +#undef TRACE_SYSTEM
> > +#define TRACE_SYSTEM xsk
> > +
> > +#if !defined(_TRACE_XSK_H) || defined(TRACE_HEADER_MULTI_READ)
> > +#define _TRACE_XSK_H
> > +
> > +#include <linux/if_xdp.h>
> > +#include <linux/tracepoint.h>
> > +
> > +#define print_reason(reason) \
> > +       __print_symbolic(reason, \
> > +                       { XSK_TRACE_DROP_PKT_TOO_BIG, "packet too big" }, \
> > +                       { XSK_TRACE_DROP_INVALID_FILLADDR, "invalid fill addr" }, \
> > +                       { XSK_TRACE_DROP_INVALID_TXD, "invalid tx desc" })
> > +
> > +#define print_val1(reason) \
> > +       __print_symbolic(reason, \
> > +                       { XSK_TRACE_DROP_PKT_TOO_BIG, "len" }, \
> > +                       { XSK_TRACE_DROP_INVALID_FILLADDR, "addr" }, \
> > +                       { XSK_TRACE_DROP_INVALID_TXD, "addr" })
> > +
> > +#define print_val2(reason) \
> > +       __print_symbolic(reason, \
> > +                       { XSK_TRACE_DROP_PKT_TOO_BIG, "max" }, \
> > +                       { XSK_TRACE_DROP_INVALID_FILLADDR, "not_used" }, \
> > +                       { XSK_TRACE_DROP_INVALID_TXD, "len" })
> > +
> > +#define print_val3(reason) \
> > +       __print_symbolic(reason, \
> > +                       { XSK_TRACE_DROP_PKT_TOO_BIG, "not_used" }, \
> > +                       { XSK_TRACE_DROP_INVALID_FILLADDR, "not_used" }, \
> > +                       { XSK_TRACE_DROP_INVALID_TXD, "options" })
> > +
> > +
> > +
> 
> nit: 3 empty lines.
> 
> > +TRACE_EVENT(xsk_packet_drop,
> > +
> > +       TP_PROTO(char *name, u16 queue_id, u32 reason, u64 val1, u64 val2,
> u64 val3),
> > +
> > +       TP_ARGS(name, queue_id, reason, val1, val2, val3),
> > +
> > +       TP_STRUCT__entry(
> > +               __field(char *, name)
> > +               __field(u16, queue_id)
> > +               __field(u32, reason)
> > +               __field(u64, val1)
> > +               __field(u32, val2)
> > +               __field(u32, val3)
> > +       ),
> > +
> > +       TP_fast_assign(
> > +               __entry->name = name;
> > +               __entry->queue_id = queue_id;
> > +               __entry->reason = reason;
> > +               __entry->val1 = val1;
> > +               __entry->val2 = val2;
> > +               __entry->val3 = val3;
> > +       ),
> > +
> > +       TP_printk("netdev: %s qid %u reason: %s: %s %llu %s %u %s %u",
> > +                 __entry->name, __entry->queue_id, print_reason(__entry-
> >reason),
> > +                 print_val1(__entry->reason), __entry->val1,
> > +                 print_val2(__entry->reason), __entry->val2,
> > +                 print_val3(__entry->reason), __entry->val3
> > +       )
> > +);
> > +
> > +#endif /* _TRACE_XSK_H */
> > +
> > +#include <trace/define_trace.h>
> > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> > index a78a8096f4ce..d7eb031d2465 100644
> > --- a/include/uapi/linux/if_xdp.h
> > +++ b/include/uapi/linux/if_xdp.h
> > @@ -108,4 +108,10 @@ struct xdp_desc {
> >
> >  /* UMEM descriptor is __u64 */
> >
> > +enum xdp_trace_reasons {
> 
> xdp_trace_reasons above, vs. XSK_TRACE_ below. Is this intentional?

It makes sense to change it to xsk_trace_reasons. I'll respin with this change, and also remove the empty lines you found.

Thanks,
Ciara

> 
> > +       XSK_TRACE_DROP_PKT_TOO_BIG,
> > +       XSK_TRACE_DROP_INVALID_FILLADDR,
> > +       XSK_TRACE_DROP_INVALID_TXD,
> > +};
> > +
> 
> [...]