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

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

[Jesper Dangaard Brouer]

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

[Jesper Dangaard Brouer]

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 <brouer@redhat.com>
---
 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;
 }

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

[John Fastabend]

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 <brouer@redhat.com>
> ---
>

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>

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

[Jesper Dangaard Brouer]

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 <brouer@redhat.com>
---
 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 2/2] xdp: adjust xdp redirect tracepoint to include return error code

[John Fastabend]

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 <brouer@redhat.com>
> ---

[...]

> @@ -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?

>  	}
>  
> -	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

[Jesper Dangaard Brouer]

On Thu, 17 Aug 2017 11:46:10 -0700 John Fastabend <john.fastabend@gmail.com> wrote:

> 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 <brouer@redhat.com>
> > ---  
> 
> [...]
> 
> > @@ -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
 
> >  	}
> >  
> > -	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

[John Fastabend]

On 08/17/2017 12:28 PM, Jesper Dangaard Brouer wrote:
> 
> On Thu, 17 Aug 2017 11:46:10 -0700 John Fastabend <john.fastabend@gmail.com> wrote:
> 
>> 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 <brouer@redhat.com>
>>> ---  
>>
>> [...]
>>
>>> @@ -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.

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

[Jesper Dangaard Brouer]

On Thu, 17 Aug 2017 12:43:18 -0700
John Fastabend <john.fastabend@gmail.com> wrote:

> >>> @@ -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

[Daniel Borkmann]

On 08/18/2017 02:29 PM, Jesper Dangaard Brouer wrote:
> On Thu, 17 Aug 2017 12:43:18 -0700
> John Fastabend <john.fastabend@gmail.com> wrote:
>
>>>>> @@ -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

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
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.