[PATCH 0/2] xdp: adjust xdp redirect tracepoint

STALE3344d

10 messages, 4 authors, 2017-08-18 · open the first message on its own page

[PATCH 0/2] xdp: adjust xdp redirect tracepoint

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-17 16:22:30

Working on streamlining the tracepoints for XDP.  The eBPF programs
and XDP have no flow-control or queueing.  Investigating using
tracepoint to provide a feedback on XDP_REDIRECT xmit overflow events.

---

Jesper Dangaard Brouer (2):
      ixgbe: change ndo_xdp_xmit return code on xmit errors
      xdp: adjust xdp redirect tracepoint to include return error code


 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
 include/trace/events/xdp.h                    |   11 +++++++----
 net/core/filter.c                             |   19 ++++++++++++-------
 3 files changed, 20 insertions(+), 12 deletions(-)

[PATCH 1/2] ixgbe: change ndo_xdp_xmit return code on xmit errors

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-17 16:22:35

Use errno -ENOSPC ("No space left on device") when the XDP xmit
have no space left on the TX ring buffer, instead of -ENOMEM.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index f9fd8d8f1bef..017cd5e85c97 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c

@@ -9860,7 +9860,7 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)
 
 	err = ixgbe_xmit_xdp_ring(adapter, xdp);
 	if (err != IXGBE_XDP_TX)
-		return -ENOMEM;
+		return -ENOSPC;
 
 	return 0;
 }

[PATCH 2/2] xdp: adjust xdp redirect tracepoint to include return error code

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-17 16:22:41

The return error code need to be included in the tracepoint
xdp:xdp_redirect, else its not possible to distinguish successful or
failed XDP_REDIRECT transmits.

XDP have no queuing mechanism. Thus, it is fairly easily to overrun a
NIC transmit queue.  The eBPF program invoking helpers (bpf_redirect
or bpf_redirect_map) to redirect a packet doesn't get any feedback
whether the packet was actually transmitted.

Info on failed transmits in the tracepoint xdp:xdp_redirect, is
interesting as this opens for providing a feedback-loop to the
receiving XDP program.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---
 include/trace/events/xdp.h |   11 +++++++----
 net/core/filter.c          |   19 ++++++++++++-------
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 7b1eb7b4be41..0e42e69f773b 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h

@@ -53,15 +53,16 @@ TRACE_EVENT(xdp_redirect,
 
 	TP_PROTO(const struct net_device *from,
 		 const struct net_device *to,
-		 const struct bpf_prog *xdp, u32 act),
+		 const struct bpf_prog *xdp, u32 act, int err),
 
-	TP_ARGS(from, to, xdp, act),
+	TP_ARGS(from, to, xdp, act, err),
 
 	TP_STRUCT__entry(
 		__string(name_from, from->name)
 		__string(name_to, to->name)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, err)
 	),
 
 	TP_fast_assign(

@@ -70,12 +71,14 @@ TRACE_EVENT(xdp_redirect,
 		__assign_str(name_from, from->name);
 		__assign_str(name_to, to->name);
 		__entry->act = act;
+		__entry->err = err;
 	),
 
-	TP_printk("prog=%s from=%s to=%s action=%s",
+	TP_printk("prog=%s from=%s to=%s action=%s err=%d",
 		  __print_hex_str(__entry->prog_tag, 8),
 		  __get_str(name_from), __get_str(name_to),
-		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB))
+		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB),
+		  __entry->err)
 );
 #endif /* _TRACE_XDP_H */

diff --git a/net/core/filter.c b/net/core/filter.c
index 5afe3ac191ec..70c9631da7f2 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c

@@ -2496,14 +2496,16 @@ int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
 	struct bpf_map *map = ri->map;
 	u32 index = ri->ifindex;
 	struct net_device *fwd;
-	int err = -EINVAL;
+	int err;
 
 	ri->ifindex = 0;
 	ri->map = NULL;
 
 	fwd = __dev_map_lookup_elem(map, index);
-	if (!fwd)
+	if (!fwd) {
+		err = -EINVAL;
 		goto out;
+	}
 
 	if (ri->map_to_flush && (ri->map_to_flush != map))
 		xdp_do_flush_map();

@@ -2513,7 +2515,7 @@ int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
 		ri->map_to_flush = map;
 
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
 	return err;
 }

@@ -2523,6 +2525,7 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
 	struct net_device *fwd;
 	u32 index = ri->ifindex;
+	int err;
 
 	if (ri->map)
 		return xdp_do_redirect_map(dev, xdp, xdp_prog);

@@ -2532,12 +2535,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	ri->map = NULL;
 	if (unlikely(!fwd)) {
 		bpf_warn_invalid_xdp_redirect(index);
-		return -EINVAL;
+		err = -EINVAL;
+		goto out;
 	}
 
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
-
-	return __bpf_tx_xdp(fwd, NULL, xdp, 0);
+	err = __bpf_tx_xdp(fwd, NULL, xdp, 0);
+out:
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);

Re: [PATCH 1/2] ixgbe: change ndo_xdp_xmit return code on xmit errors

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-08-17 18:32:51

On 08/17/2017 09:22 AM, Jesper Dangaard Brouer wrote:

Use errno -ENOSPC ("No space left on device") when the XDP xmit
have no space left on the TX ring buffer, instead of -ENOMEM.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---

Seems like an improvement to me. You could also potentially
distinguish between DMA mapping errors and descriptor overrun
but likely not needed.

Acked-by: John Fastabend <john.fastabend@gmail.com>

Re: [PATCH 2/2] xdp: adjust xdp redirect tracepoint to include return error code

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-08-17 18:46:19

On 08/17/2017 09:22 AM, Jesper Dangaard Brouer wrote:

The return error code need to be included in the tracepoint
xdp:xdp_redirect, else its not possible to distinguish successful or
failed XDP_REDIRECT transmits.

XDP have no queuing mechanism. Thus, it is fairly easily to overrun a
NIC transmit queue.  The eBPF program invoking helpers (bpf_redirect
or bpf_redirect_map) to redirect a packet doesn't get any feedback
whether the packet was actually transmitted.

Info on failed transmits in the tracepoint xdp:xdp_redirect, is
interesting as this opens for providing a feedback-loop to the
receiving XDP program.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---

[...]

quoted hunk

@@ -2532,12 +2535,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	ri->map = NULL;
 	if (unlikely(!fwd)) {
 		bpf_warn_invalid_xdp_redirect(index);
-		return -EINVAL;
+		err = -EINVAL;
+		goto out;

It doesn't look like there is a check in trace_xdp_redirect to
avoid dereferencing a NULL fwd pointer here (*to in trace code
path). Did I miss something?

quoted hunk

 	}
 
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
-
-	return __bpf_tx_xdp(fwd, NULL, xdp, 0);
+	err = __bpf_tx_xdp(fwd, NULL, xdp, 0);
+out:
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
 

Re: [PATCH 2/2] xdp: adjust xdp redirect tracepoint to include return error code

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-17 19:29:03

On Thu, 17 Aug 2017 11:46:10 -0700 John Fastabend [off-list ref] wrote:

On 08/17/2017 09:22 AM, Jesper Dangaard Brouer wrote:

quoted

The return error code need to be included in the tracepoint
xdp:xdp_redirect, else its not possible to distinguish successful or
failed XDP_REDIRECT transmits.

XDP have no queuing mechanism. Thus, it is fairly easily to overrun a
NIC transmit queue.  The eBPF program invoking helpers (bpf_redirect
or bpf_redirect_map) to redirect a packet doesn't get any feedback
whether the packet was actually transmitted.

Info on failed transmits in the tracepoint xdp:xdp_redirect, is
interesting as this opens for providing a feedback-loop to the
receiving XDP program.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---

[...]

quoted

@@ -2532,12 +2535,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	ri->map = NULL;
 	if (unlikely(!fwd)) {
 		bpf_warn_invalid_xdp_redirect(index);
-		return -EINVAL;
+		err = -EINVAL;
+		goto out;  

It doesn't look like there is a check in trace_xdp_redirect to
avoid dereferencing a NULL fwd pointer here (*to in trace code
path). Did I miss something?

Nice that you spotted this in your review, but the __string() macro
used in trace code already takes case of this, see output:

 xdp:xdp_redirect: prog=39cf08f65683838a from=ixgbe2 to=(null) action=REDIRECT err=-22

quoted

 	}
 
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT);
-
-	return __bpf_tx_xdp(fwd, NULL, xdp, 0);
+	err = __bpf_tx_xdp(fwd, NULL, xdp, 0);
+out:
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 2/2] xdp: adjust xdp redirect tracepoint to include return error code

From: John Fastabend <john.fastabend@gmail.com>
Date: 2017-08-17 19:43:28

On 08/17/2017 12:28 PM, Jesper Dangaard Brouer wrote:

On Thu, 17 Aug 2017 11:46:10 -0700 John Fastabend [off-list ref] wrote:

quoted

On 08/17/2017 09:22 AM, Jesper Dangaard Brouer wrote:

quoted

The return error code need to be included in the tracepoint
xdp:xdp_redirect, else its not possible to distinguish successful or
failed XDP_REDIRECT transmits.

XDP have no queuing mechanism. Thus, it is fairly easily to overrun a
NIC transmit queue.  The eBPF program invoking helpers (bpf_redirect
or bpf_redirect_map) to redirect a packet doesn't get any feedback
whether the packet was actually transmitted.

Info on failed transmits in the tracepoint xdp:xdp_redirect, is
interesting as this opens for providing a feedback-loop to the
receiving XDP program.

Signed-off-by: Jesper Dangaard Brouer <redacted>
---

[...]

quoted

@@ -2532,12 +2535,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	ri->map = NULL;
 	if (unlikely(!fwd)) {
 		bpf_warn_invalid_xdp_redirect(index);

I think we should drop the warn_invalid now that we have a tracepoint.
The tracepoint is much nicer for debugging vs a warning for what might
be a valid case depending on xdp program.

quoted

-		return -EINVAL;
+		err = -EINVAL;
+		goto out;  

It doesn't look like there is a check in trace_xdp_redirect to
avoid dereferencing a NULL fwd pointer here (*to in trace code
path). Did I miss something?

Nice that you spotted this in your review, but the __string() macro
used in trace code already takes case of this, see output:

 xdp:xdp_redirect: prog=39cf08f65683838a from=ixgbe2 to=(null) action=REDIRECT err=-22

Great thanks.

Re: [PATCH 2/2] xdp: adjust xdp redirect tracepoint to include return error code

From: Jesper Dangaard Brouer <hidden>
Date: 2017-08-18 12:29:51

On Thu, 17 Aug 2017 12:43:18 -0700
John Fastabend [off-list ref] wrote:

quoted

@@ -2532,12 +2535,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	ri->map = NULL;
 	if (unlikely(!fwd)) {
 		bpf_warn_invalid_xdp_redirect(index);  

I think we should drop the warn_invalid now that we have a tracepoint.
The tracepoint is much nicer for debugging vs a warning for what might
be a valid case depending on xdp program.

I agree.  I'll do that in a follow up patch. I'll likely remove the
bpf_warn_invalid_xdp_redirect() function completely.

We also have bpf_warn_invalid_xdp_action() but that might be relevant
to keep around(?).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH 2/2] xdp: adjust xdp redirect tracepoint to include return error code

From: Daniel Borkmann <daniel@iogearbox.net>
Date: 2017-08-18 12:38:32

On 08/18/2017 02:29 PM, Jesper Dangaard Brouer wrote:

On Thu, 17 Aug 2017 12:43:18 -0700
John Fastabend [off-list ref] wrote:

quoted

@@ -2532,12 +2535,14 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
  	ri->map = NULL;
  	if (unlikely(!fwd)) {
  		bpf_warn_invalid_xdp_redirect(index);

I think we should drop the warn_invalid now that we have a tracepoint.
The tracepoint is much nicer for debugging vs a warning for what might
be a valid case depending on xdp program.

I agree.  I'll do that in a follow up patch. I'll likely remove the
bpf_warn_invalid_xdp_redirect() function completely.

+1

We also have bpf_warn_invalid_xdp_action() but that might be relevant
to keep around(?).

Keeping this is fine, imo.

Re: [PATCH 0/2] xdp: adjust xdp redirect tracepoint

From: David Miller <davem@davemloft.net>
Date: 2017-08-18 23:19:10

From: Jesper Dangaard Brouer <redacted>
Date: Thu, 17 Aug 2017 18:22:27 +0200

Working on streamlining the tracepoints for XDP.  The eBPF programs
and XDP have no flow-control or queueing.  Investigating using
tracepoint to provide a feedback on XDP_REDIRECT xmit overflow events.

Series applied, thanks.

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help