On 1/26/21 8:52 AM, Ciara Loftus wrote:

quoted

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?

Thanks for your feedback Daniel.

The stats tell us that there is *a* problem whereas the traces will shed
light on what that problem is. eg. The XSK_TRACE_DROP_PKT_TOO_BIG
trace tells us we dropped a packet on RX due to it being too big vs. ss
would just tell us the packet was dropped.
I will add this rationale in the v3.

[...]

quoted

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)

quoted

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

quoted

  		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?

You are correct. After some consideration I'm going to drop this
trace and some others in the next rev which are not technically
guaranteed drops and could end up spamming the log under
stress as you mentioned.

quoted

  	}

@@ -192,6 +194,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct

xdp_buff *xdp)

quoted

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

quoted

  		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?

This too falls into the not-technically-a-drop category and will be
removed in the next rev. I think a stat would be useful though.
I'll draw up a separate patch. Thanks for the suggestion.

quoted

  			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