Re: [PATCH bpf-next v2 1/6] xsk: add tracepoints for packet drops
From: Daniel Borkmann <daniel@iogearbox.net>
Date: 2021-01-27 12:47:40
Also in:
bpf
On 1/26/21 8:52 AM, Ciara Loftus wrote:
This commit introduces static perf tracepoints for AF_XDP which report information about packet drops, such as the netdev, qid and high level reason for the drop. The tracepoint can be enabled/disabled by toggling /sys/kernel/debug/tracing/events/xsk/xsk_packet_drop/enable
Could you add a rationale to the commit log on why xsk diag stats dump is insufficient here given you add tracepoints to most locations where we also bump the counters already? And given diag infra also exposes the ifindex, queue id, etc, why troubleshooting the xsk socket via ss tool is not sufficient? [...]
quoted hunk ↗ jump to hunk
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c index 4faabd1ecfd1..9b850716630b 100644 --- a/net/xdp/xsk.c +++ b/net/xdp/xsk.c@@ -11,6 +11,7 @@ #define pr_fmt(fmt) "AF_XDP: %s: " fmt, __func__ +#include <linux/bpf_trace.h> #include <linux/if_xdp.h> #include <linux/init.h> #include <linux/sched/mm.h>@@ -158,6 +159,7 @@ static int __xsk_rcv_zc(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len) addr = xp_get_handle(xskb); err = xskq_prod_reserve_desc(xs->rx, addr, len); if (err) { + trace_xsk_packet_drop(xs->dev->name, xs->queue_id, XSK_TRACE_DROP_RXQ_FULL); xs->rx_queue_full++; return err;
I presume if this will be triggered under stress you'll likely also spam your trace event log w/ potentially mio of msgs per sec?
quoted hunk ↗ jump to hunk
}@@ -192,6 +194,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp) len = xdp->data_end - xdp->data; if (len > xsk_pool_get_rx_frame_size(xs->pool)) { + trace_xsk_packet_drop(xs->dev->name, xs->queue_id, XSK_TRACE_DROP_PKT_TOO_BIG); xs->rx_dropped++; return -ENOSPC; }@@ -516,6 +519,8 @@ static int xsk_generic_xmit(struct sock *sk) if (err == NET_XMIT_DROP) { /* SKB completed but not sent */ err = -EBUSY; + trace_xsk_packet_drop(xs->dev->name, xs->queue_id, + XSK_TRACE_DROP_DRV_ERR_TX);
Is there a reason to not bump error counter here?
quoted hunk ↗ jump to hunk
goto out; }diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c index 8de01aaac4a0..d3c1ca83c75d 100644 --- a/net/xdp/xsk_buff_pool.c +++ b/net/xdp/xsk_buff_pool.c@@ -1,5 +1,6 @@ // SPDX-License-Identifier: GPL-2.0