[PATCH net-next 0/5] xdp: more work on xdp tracepoints

[PATCH net-next 0/5] xdp: more work on xdp tracepoints

[Jesper Dangaard Brouer]

More work on streamlining the tracepoints for XDP.

I've created a simple xdp_monitor application that uses this
tracepoint, and prints statistics. Available at github:

https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_kern.c
https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_user.c

---

Jesper Dangaard Brouer (5):
      xdp: remove bpf_warn_invalid_xdp_redirect
      xdp: make generic xdp redirect use tracepoint trace_xdp_redirect
      ixgbe: use return codes from ndo_xdp_xmit that are distinguishable
      xdp: remove net_device names from xdp_redirect tracepoint
      xdp: get tracepoints xdp_exception and xdp_redirect in sync


 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 +-
 include/linux/filter.h                        |    3 +-
 include/trace/events/xdp.h                    |   33 ++++++++---------
 net/core/dev.c                                |    4 +-
 net/core/filter.c                             |   48 +++++++++++++------------
 5 files changed, 46 insertions(+), 46 deletions(-)

[PATCH net-next 1/5] xdp: remove bpf_warn_invalid_xdp_redirect

[Jesper Dangaard Brouer]

Given there is a tracepoint that can track the error code
of xdp_do_redirect calls, the WARN_ONCE in bpf_warn_invalid_xdp_redirect
doesn't seem relevant any longer.  Simply remove the function.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 net/core/filter.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index fa2115695037..31c579749679 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2499,7 +2499,6 @@ static int __bpf_tx_xdp(struct net_device *dev,
 	int err;
 
 	if (!dev->netdev_ops->ndo_xdp_xmit) {
-		bpf_warn_invalid_xdp_redirect(dev->ifindex);
 		return -EOPNOTSUPP;
 	}
 
@@ -2572,7 +2571,6 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!fwd)) {
-		bpf_warn_invalid_xdp_redirect(index);
 		err = -EINVAL;
 		goto out;
 	}
@@ -2593,7 +2591,6 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb)
 	dev = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!dev)) {
-		bpf_warn_invalid_xdp_redirect(index);
 		goto err;
 	}
 
@@ -3573,11 +3570,6 @@ void bpf_warn_invalid_xdp_action(u32 act)
 }
 EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
 
-void bpf_warn_invalid_xdp_redirect(u32 ifindex)
-{
-	WARN_ONCE(1, "Illegal XDP redirect to unsupported device ifindex(%i)\n", ifindex);
-}
-
 static bool __is_valid_sock_ops_access(int off, int size)
 {
 	if (off < 0 || off >= sizeof(struct bpf_sock_ops))

Re: [PATCH net-next 1/5] xdp: remove bpf_warn_invalid_xdp_redirect

[Daniel Borkmann]

On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
> Given there is a tracepoint that can track the error code
> of xdp_do_redirect calls, the WARN_ONCE in bpf_warn_invalid_xdp_redirect
> doesn't seem relevant any longer.  Simply remove the function.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH net-next 2/5] xdp: make generic xdp redirect use tracepoint trace_xdp_redirect

[Jesper Dangaard Brouer]

If the xdp_do_generic_redirect() call fails, it trigger the
trace_xdp_exception tracepoint.  It seems better to use the same
tracepoint trace_xdp_redirect, as the native xdp_do_redirect{,_map} does.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/linux/filter.h |    3 ++-
 net/core/dev.c         |    4 ++--
 net/core/filter.c      |   36 ++++++++++++++++++++++--------------
 3 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 7015116331af..d29e58fde364 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -718,7 +718,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
  * because we only track one map and force a flush when the map changes.
  * This does not appear to be a real limitation for existing software.
  */
-int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb);
+int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
+			    struct bpf_prog *prog);
 int xdp_do_redirect(struct net_device *dev,
 		    struct xdp_buff *xdp,
 		    struct bpf_prog *prog);
diff --git a/net/core/dev.c b/net/core/dev.c
index 40b28e417072..270b54754821 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3953,7 +3953,8 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 		if (act != XDP_PASS) {
 			switch (act) {
 			case XDP_REDIRECT:
-				err = xdp_do_generic_redirect(skb->dev, skb);
+				err = xdp_do_generic_redirect(skb->dev, skb,
+							      xdp_prog);
 				if (err)
 					goto out_redir;
 			/* fallthru to submit skb */
@@ -3966,7 +3967,6 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 	}
 	return XDP_PASS;
 out_redir:
-	trace_xdp_exception(skb->dev, xdp_prog, XDP_REDIRECT);
 	kfree_skb(skb);
 	return XDP_DROP;
 }
diff --git a/net/core/filter.c b/net/core/filter.c
index 31c579749679..2d7cdb2c5c66 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2582,29 +2582,37 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
 
-int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb)
+int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
+			    struct bpf_prog *xdp_prog)
 {
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
-	unsigned int len;
 	u32 index = ri->ifindex;
+	struct net_device *fwd;
+	unsigned int len;
+	int err = 0;
 
-	dev = dev_get_by_index_rcu(dev_net(dev), index);
+	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
-	if (unlikely(!dev)) {
-		goto err;
+	if (unlikely(!fwd)) {
+		err = -EINVAL;
+		goto out;
 	}
 
-	if (unlikely(!(dev->flags & IFF_UP)))
-		goto err;
+	if (unlikely(!(fwd->flags & IFF_UP))) {
+		err = -ENOLINK;
+		goto out;
+	}
 
-	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
-	if (skb->len > len)
-		goto err;
+	len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN;
+	if (skb->len > len) {
+		err = -EMSGSIZE;
+		goto out;
+	}
 
-	skb->dev = dev;
-	return 0;
-err:
-	return -EINVAL;
+	skb->dev = fwd;
+out:
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);
 

Re: [PATCH net-next 2/5] xdp: make generic xdp redirect use tracepoint trace_xdp_redirect

[Daniel Borkmann]

On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
> If the xdp_do_generic_redirect() call fails, it trigger the
> trace_xdp_exception tracepoint.  It seems better to use the same
> tracepoint trace_xdp_redirect, as the native xdp_do_redirect{,_map} does.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Makes sense to make this consistent.

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH net-next 3/5] ixgbe: use return codes from ndo_xdp_xmit that are distinguishable

[Jesper Dangaard Brouer]

For XDP_REDIRECT the use of return code -EINVAL is confusing, as it is
used in three different cases.  (1) When the index or ifindex lookup
fails, and in the ixgbe driver (2) when link is down and (3) when XDP
have not been enabled.

The return code can be picked up by the tracepoint xdp:xdp_redirect
for diagnosing why XDP_REDIRECT isn't working.  Thus, there is a need
different return codes to tell the issues apart.

I'm considering using a specific err-code scheme for XDP_REDIRECT
instead of using these errno codes.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8d3224ad6434..3afb8c4b9d48 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)
 	int err;
 
 	if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
-		return -EINVAL;
+		return -ENOLINK;
 
 	/* During program transitions its possible adapter->xdp_prog is assigned
 	 * but ring has not been configured yet. In this case simply abort xmit.
 	 */
 	ring = adapter->xdp_prog ? adapter->xdp_ring[smp_processor_id()] : NULL;
 	if (unlikely(!ring))
-		return -EINVAL;
+		return -ENXIO;
 
 	err = ixgbe_xmit_xdp_ring(adapter, xdp);
 	if (err != IXGBE_XDP_TX)

Re: [PATCH net-next 3/5] ixgbe: use return codes from ndo_xdp_xmit that are distinguishable

[Daniel Borkmann]

On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
> For XDP_REDIRECT the use of return code -EINVAL is confusing, as it is
> used in three different cases.  (1) When the index or ifindex lookup
> fails, and in the ixgbe driver (2) when link is down and (3) when XDP
> have not been enabled.
>
> The return code can be picked up by the tracepoint xdp:xdp_redirect
> for diagnosing why XDP_REDIRECT isn't working.  Thus, there is a need
> different return codes to tell the issues apart.
>
> I'm considering using a specific err-code scheme for XDP_REDIRECT
> instead of using these errno codes.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH net-next 4/5] xdp: remove net_device names from xdp_redirect tracepoint

[Jesper Dangaard Brouer]

There is too much overhead in the current trace_xdp_redirect
tracepoint as it does strcpy and strlen on the net_device names.

Besides, exposing the ifindex/index is actually the information that
is needed in the tracepoint to diagnose issues.  When a lookup fails
(either ifindex or devmap index) then there is a need for saying which
to_index that have issues.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/trace/events/xdp.h |   17 ++++++++---------
 net/core/filter.c          |    6 +++---
 2 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 0e42e69f773b..7511bed80558 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -51,15 +51,14 @@ TRACE_EVENT(xdp_exception,
 
 TRACE_EVENT(xdp_redirect,
 
-	TP_PROTO(const struct net_device *from,
-		 const struct net_device *to,
+	TP_PROTO(int from_index, int to_index,
 		 const struct bpf_prog *xdp, u32 act, int err),
 
-	TP_ARGS(from, to, xdp, act, err),
+	TP_ARGS(from_index, to_index, xdp, act, err),
 
 	TP_STRUCT__entry(
-		__string(name_from, from->name)
-		__string(name_to, to->name)
+		__field(int, from_index)
+		__field(int, to_index)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
 		__field(int, err)
@@ -68,15 +67,15 @@ TRACE_EVENT(xdp_redirect,
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
-		__assign_str(name_from, from->name);
-		__assign_str(name_to, to->name);
+		__entry->from_index	= from_index;
+		__entry->to_index	= to_index;
 		__entry->act = act;
 		__entry->err = err;
 	),
 
-	TP_printk("prog=%s from=%s to=%s action=%s err=%d",
+	TP_printk("prog=%s from=%d to=%d action=%s err=%d",
 		  __print_hex_str(__entry->prog_tag, 8),
-		  __get_str(name_from), __get_str(name_to),
+		  __entry->from_index, __entry->to_index,
 		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB),
 		  __entry->err)
 );
diff --git a/net/core/filter.c b/net/core/filter.c
index 2d7cdb2c5c66..35f0383fa2b9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2553,7 +2553,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, err);
+	trace_xdp_redirect(dev->ifindex, index, xdp_prog, XDP_REDIRECT, err);
 	return err;
 }
 
@@ -2577,7 +2577,7 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 
 	err = __bpf_tx_xdp(fwd, NULL, xdp, 0);
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev->ifindex, index, xdp_prog, XDP_REDIRECT, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
@@ -2611,7 +2611,7 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 
 	skb->dev = fwd;
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev->ifindex, index, xdp_prog, XDP_REDIRECT, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);

Re: [PATCH net-next 4/5] xdp: remove net_device names from xdp_redirect tracepoint

[Daniel Borkmann]

On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
> There is too much overhead in the current trace_xdp_redirect
> tracepoint as it does strcpy and strlen on the net_device names.
>
> Besides, exposing the ifindex/index is actually the information that
> is needed in the tracepoint to diagnose issues.  When a lookup fails
> (either ifindex or devmap index) then there is a need for saying which
> to_index that have issues.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Jesper Dangaard Brouer]

Remove the net_device string name from the xdp_exception tracepoint,
like the xdp_redirect tracepoint.

Align the TP_STRUCT to have common entries between these two
tracepoint.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/trace/events/xdp.h |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 7511bed80558..6495b0d9d5c7 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception,
 	TP_ARGS(dev, xdp, act),
 
 	TP_STRUCT__entry(
-		__string(name, dev->name)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, ifindex)
 	),
 
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
-		__assign_str(name, dev->name);
-		__entry->act = act;
+		__entry->act		= act;
+		__entry->ifindex	= dev->ifindex;
 	),
 
-	TP_printk("prog=%s device=%s action=%s",
+	TP_printk("prog=%s action=%s ifindex=%d",
 		  __print_hex_str(__entry->prog_tag, 8),
-		  __get_str(name),
-		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB))
+		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB),
+		  __entry->ifindex)
 );
 
 TRACE_EVENT(xdp_redirect,
@@ -57,26 +57,26 @@ TRACE_EVENT(xdp_redirect,
 	TP_ARGS(from_index, to_index, xdp, act, err),
 
 	TP_STRUCT__entry(
-		__field(int, from_index)
-		__field(int, to_index)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, from_index)
+		__field(int, to_index)
 		__field(int, err)
 	),
 
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
+		__entry->act		= act;
 		__entry->from_index	= from_index;
 		__entry->to_index	= to_index;
-		__entry->act = act;
-		__entry->err = err;
+		__entry->err		= err;
 	),
 
-	TP_printk("prog=%s from=%d to=%d action=%s err=%d",
+	TP_printk("prog=%s action=%s from=%d to=%d err=%d",
 		  __print_hex_str(__entry->prog_tag, 8),
-		  __entry->from_index, __entry->to_index,
 		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB),
+		  __entry->from_index, __entry->to_index,
 		  __entry->err)
 );
 #endif /* _TRACE_XDP_H */

Re: [PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Daniel Borkmann]

On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
> Remove the net_device string name from the xdp_exception tracepoint,
> like the xdp_redirect tracepoint.
>
> Align the TP_STRUCT to have common entries between these two
> tracepoint.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>   include/trace/events/xdp.h |   24 ++++++++++++------------
>   1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> index 7511bed80558..6495b0d9d5c7 100644
> --- a/include/trace/events/xdp.h
> +++ b/include/trace/events/xdp.h
> @@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception,
>   	TP_ARGS(dev, xdp, act),
>
>   	TP_STRUCT__entry(
> -		__string(name, dev->name)
>   		__array(u8, prog_tag, 8)
>   		__field(u32, act)
> +		__field(int, ifindex)
>   	),
>
>   	TP_fast_assign(
>   		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
>   		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
> -		__assign_str(name, dev->name);
> -		__entry->act = act;
> +		__entry->act		= act;
> +		__entry->ifindex	= dev->ifindex;
>   	),
>
[...]
>   TRACE_EVENT(xdp_redirect,
> @@ -57,26 +57,26 @@ TRACE_EVENT(xdp_redirect,
>   	TP_ARGS(from_index, to_index, xdp, act, err),
>
>   	TP_STRUCT__entry(
> -		__field(int, from_index)
> -		__field(int, to_index)
>   		__array(u8, prog_tag, 8)
>   		__field(u32, act)
> +		__field(int, from_index)
> +		__field(int, to_index)
>   		__field(int, err)
>   	),
>
>   	TP_fast_assign(
>   		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
>   		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
> +		__entry->act		= act;
>   		__entry->from_index	= from_index;
>   		__entry->to_index	= to_index;
> -		__entry->act = act;

If you already get them in sync, could you also make it consistent
that for both tracepoints in TP_ARGS() we use either ifindex
directly or device pointer and extract it from TP_fast_assign().
Right now, it's mixed.

Re: [PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Jesper Dangaard Brouer]

On Tue, 22 Aug 2017 23:30:21 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
> > Remove the net_device string name from the xdp_exception tracepoint,
> > like the xdp_redirect tracepoint.
> >
> > Align the TP_STRUCT to have common entries between these two
> > tracepoint.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> >   include/trace/events/xdp.h |   24 ++++++++++++------------
> >   1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> > index 7511bed80558..6495b0d9d5c7 100644
> > --- a/include/trace/events/xdp.h
> > +++ b/include/trace/events/xdp.h
> > @@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception,
> >   	TP_ARGS(dev, xdp, act),
> >
> >   	TP_STRUCT__entry(
> > -		__string(name, dev->name)
> >   		__array(u8, prog_tag, 8)
> >   		__field(u32, act)
> > +		__field(int, ifindex)
> >   	),
> >
> >   	TP_fast_assign(
> >   		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
> >   		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
> > -		__assign_str(name, dev->name);
> > -		__entry->act = act;
> > +		__entry->act		= act;
> > +		__entry->ifindex	= dev->ifindex;
> >   	),
> >  
> [...]
> >   TRACE_EVENT(xdp_redirect,
> > @@ -57,26 +57,26 @@ TRACE_EVENT(xdp_redirect,
> >   	TP_ARGS(from_index, to_index, xdp, act, err),
> >
> >   	TP_STRUCT__entry(
> > -		__field(int, from_index)
> > -		__field(int, to_index)
> >   		__array(u8, prog_tag, 8)
> >   		__field(u32, act)
> > +		__field(int, from_index)
> > +		__field(int, to_index)
> >   		__field(int, err)
> >   	),
> >
> >   	TP_fast_assign(
> >   		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
> >   		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
> > +		__entry->act		= act;
> >   		__entry->from_index	= from_index;
> >   		__entry->to_index	= to_index;
> > -		__entry->act = act;  
> 
> If you already get them in sync, could you also make it consistent
> that for both tracepoints in TP_ARGS() we use either ifindex
> directly or device pointer and extract it from TP_fast_assign().
> Right now, it's mixed.

I did this because, in the (bpf_redirect)_map case, I was hoping that
"from_index" could be come the index from the map.  Looking closer at
the devmap design, I cannot see how we can ever know what (the bpf_prog
thinks) is the corresponding map-ingress/from_index.

Based on that, I think we can/should use the device pointer as arg (as
you suggested). And then rename "from_index" to "ifindex", where
ifindex is the XDP ifindex the xdp_buff arrived on.

I guess we can keep the "to_index".  We also need to extend the
tracepoint args with a "map", to let the trace user tell whether the
"to_index" is an ifindex or a map-index.

I'll code this up and submit at V2.
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Daniel Borkmann]

On 08/23/2017 09:41 AM, Jesper Dangaard Brouer wrote:
> On Tue, 22 Aug 2017 23:30:21 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
>>> Remove the net_device string name from the xdp_exception tracepoint,
>>> like the xdp_redirect tracepoint.
>>>
>>> Align the TP_STRUCT to have common entries between these two
>>> tracepoint.
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>>    include/trace/events/xdp.h |   24 ++++++++++++------------
>>>    1 file changed, 12 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
>>> index 7511bed80558..6495b0d9d5c7 100644
>>> --- a/include/trace/events/xdp.h
>>> +++ b/include/trace/events/xdp.h
>>> @@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception,
>>>    	TP_ARGS(dev, xdp, act),
>>>
>>>    	TP_STRUCT__entry(
>>> -		__string(name, dev->name)
>>>    		__array(u8, prog_tag, 8)
>>>    		__field(u32, act)
>>> +		__field(int, ifindex)
>>>    	),
>>>
>>>    	TP_fast_assign(
>>>    		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
>>>    		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
>>> -		__assign_str(name, dev->name);
>>> -		__entry->act = act;
>>> +		__entry->act		= act;
>>> +		__entry->ifindex	= dev->ifindex;
>>>    	),
>>>
>> [...]
>>>    TRACE_EVENT(xdp_redirect,
>>> @@ -57,26 +57,26 @@ TRACE_EVENT(xdp_redirect,
>>>    	TP_ARGS(from_index, to_index, xdp, act, err),
>>>
>>>    	TP_STRUCT__entry(
>>> -		__field(int, from_index)
>>> -		__field(int, to_index)
>>>    		__array(u8, prog_tag, 8)
>>>    		__field(u32, act)
>>> +		__field(int, from_index)
>>> +		__field(int, to_index)
>>>    		__field(int, err)
>>>    	),
>>>
>>>    	TP_fast_assign(
>>>    		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
>>>    		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
>>> +		__entry->act		= act;
>>>    		__entry->from_index	= from_index;
>>>    		__entry->to_index	= to_index;
>>> -		__entry->act = act;
>>
>> If you already get them in sync, could you also make it consistent
>> that for both tracepoints in TP_ARGS() we use either ifindex
>> directly or device pointer and extract it from TP_fast_assign().
>> Right now, it's mixed.
>
> I did this because, in the (bpf_redirect)_map case, I was hoping that
> "from_index" could be come the index from the map.  Looking closer at
> the devmap design, I cannot see how we can ever know what (the bpf_prog
> thinks) is the corresponding map-ingress/from_index.
>
> Based on that, I think we can/should use the device pointer as arg (as
> you suggested). And then rename "from_index" to "ifindex", where
> ifindex is the XDP ifindex the xdp_buff arrived on.
>
> I guess we can keep the "to_index".  We also need to extend the
> tracepoint args with a "map", to let the trace user tell whether the
> "to_index" is an ifindex or a map-index.

Yeah, makes sense, right now we cannot differentiate between that.

> I'll code this up and submit at V2.

Thanks!

[V2 PATCH net-next 0/5] xdp: more work on xdp tracepoints

[Jesper Dangaard Brouer]

More work on streamlining the tracepoints for XDP.

I've created a simple xdp_monitor application that uses this
tracepoint, and prints statistics. Available at github:

https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_kern.c
https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_user.c

V2: Change trace_xdp_redirect() to align with args of trace_xdp_exception()

---

Jesper Dangaard Brouer (5):
      xdp: remove bpf_warn_invalid_xdp_redirect
      xdp: make generic xdp redirect use tracepoint trace_xdp_redirect
      ixgbe: use return codes from ndo_xdp_xmit that are distinguishable
      xdp: remove net_device names from xdp_redirect tracepoint
      xdp: get tracepoints xdp_exception and xdp_redirect in sync


 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 +-
 include/linux/filter.h                        |    3 +-
 include/trace/events/xdp.h                    |   36 +++++++++----------
 net/core/dev.c                                |    4 +-
 net/core/filter.c                             |   48 +++++++++++++------------
 5 files changed, 48 insertions(+), 47 deletions(-)

[V2 PATCH net-next 1/5] xdp: remove bpf_warn_invalid_xdp_redirect

[Jesper Dangaard Brouer]

Given there is a tracepoint that can track the error code
of xdp_do_redirect calls, the WARN_ONCE in bpf_warn_invalid_xdp_redirect
doesn't seem relevant any longer.  Simply remove the function.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/core/filter.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index fa2115695037..31c579749679 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2499,7 +2499,6 @@ static int __bpf_tx_xdp(struct net_device *dev,
 	int err;
 
 	if (!dev->netdev_ops->ndo_xdp_xmit) {
-		bpf_warn_invalid_xdp_redirect(dev->ifindex);
 		return -EOPNOTSUPP;
 	}
 
@@ -2572,7 +2571,6 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!fwd)) {
-		bpf_warn_invalid_xdp_redirect(index);
 		err = -EINVAL;
 		goto out;
 	}
@@ -2593,7 +2591,6 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb)
 	dev = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!dev)) {
-		bpf_warn_invalid_xdp_redirect(index);
 		goto err;
 	}
 
@@ -3573,11 +3570,6 @@ void bpf_warn_invalid_xdp_action(u32 act)
 }
 EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
 
-void bpf_warn_invalid_xdp_redirect(u32 ifindex)
-{
-	WARN_ONCE(1, "Illegal XDP redirect to unsupported device ifindex(%i)\n", ifindex);
-}
-
 static bool __is_valid_sock_ops_access(int off, int size)
 {
 	if (off < 0 || off >= sizeof(struct bpf_sock_ops))

[V2 PATCH net-next 2/5] xdp: make generic xdp redirect use tracepoint trace_xdp_redirect

[Jesper Dangaard Brouer]

If the xdp_do_generic_redirect() call fails, it trigger the
trace_xdp_exception tracepoint.  It seems better to use the same
tracepoint trace_xdp_redirect, as the native xdp_do_redirect{,_map} does.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/linux/filter.h |    3 ++-
 net/core/dev.c         |    4 ++--
 net/core/filter.c      |   36 ++++++++++++++++++++++--------------
 3 files changed, 26 insertions(+), 17 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 7015116331af..d29e58fde364 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -718,7 +718,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
  * because we only track one map and force a flush when the map changes.
  * This does not appear to be a real limitation for existing software.
  */
-int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb);
+int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
+			    struct bpf_prog *prog);
 int xdp_do_redirect(struct net_device *dev,
 		    struct xdp_buff *xdp,
 		    struct bpf_prog *prog);
diff --git a/net/core/dev.c b/net/core/dev.c
index 40b28e417072..270b54754821 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3953,7 +3953,8 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 		if (act != XDP_PASS) {
 			switch (act) {
 			case XDP_REDIRECT:
-				err = xdp_do_generic_redirect(skb->dev, skb);
+				err = xdp_do_generic_redirect(skb->dev, skb,
+							      xdp_prog);
 				if (err)
 					goto out_redir;
 			/* fallthru to submit skb */
@@ -3966,7 +3967,6 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 	}
 	return XDP_PASS;
 out_redir:
-	trace_xdp_exception(skb->dev, xdp_prog, XDP_REDIRECT);
 	kfree_skb(skb);
 	return XDP_DROP;
 }
diff --git a/net/core/filter.c b/net/core/filter.c
index 31c579749679..2d7cdb2c5c66 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2582,29 +2582,37 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
 
-int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb)
+int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
+			    struct bpf_prog *xdp_prog)
 {
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
-	unsigned int len;
 	u32 index = ri->ifindex;
+	struct net_device *fwd;
+	unsigned int len;
+	int err = 0;
 
-	dev = dev_get_by_index_rcu(dev_net(dev), index);
+	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
-	if (unlikely(!dev)) {
-		goto err;
+	if (unlikely(!fwd)) {
+		err = -EINVAL;
+		goto out;
 	}
 
-	if (unlikely(!(dev->flags & IFF_UP)))
-		goto err;
+	if (unlikely(!(fwd->flags & IFF_UP))) {
+		err = -ENOLINK;
+		goto out;
+	}
 
-	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
-	if (skb->len > len)
-		goto err;
+	len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN;
+	if (skb->len > len) {
+		err = -EMSGSIZE;
+		goto out;
+	}
 
-	skb->dev = dev;
-	return 0;
-err:
-	return -EINVAL;
+	skb->dev = fwd;
+out:
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);
 

[V2 PATCH net-next 3/5] ixgbe: use return codes from ndo_xdp_xmit that are distinguishable

[Jesper Dangaard Brouer]

For XDP_REDIRECT the use of return code -EINVAL is confusing, as it is
used in three different cases.  (1) When the index or ifindex lookup
fails, and in the ixgbe driver (2) when link is down and (3) when XDP
have not been enabled.

The return code can be picked up by the tracepoint xdp:xdp_redirect
for diagnosing why XDP_REDIRECT isn't working.  Thus, there is a need
different return codes to tell the issues apart.

I'm considering using a specific err-code scheme for XDP_REDIRECT
instead of using these errno codes.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8d3224ad6434..3afb8c4b9d48 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)
 	int err;
 
 	if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
-		return -EINVAL;
+		return -ENOLINK;
 
 	/* During program transitions its possible adapter->xdp_prog is assigned
 	 * but ring has not been configured yet. In this case simply abort xmit.
 	 */
 	ring = adapter->xdp_prog ? adapter->xdp_ring[smp_processor_id()] : NULL;
 	if (unlikely(!ring))
-		return -EINVAL;
+		return -ENXIO;
 
 	err = ixgbe_xmit_xdp_ring(adapter, xdp);
 	if (err != IXGBE_XDP_TX)

[V2 PATCH net-next 4/5] xdp: remove net_device names from xdp_redirect tracepoint

[Jesper Dangaard Brouer]

There is too much overhead in the current trace_xdp_redirect
tracepoint as it does strcpy and strlen on the net_device names.

Besides, exposing the ifindex/index is actually the information that
is needed in the tracepoint to diagnose issues.  When a lookup fails
(either ifindex or devmap index) then there is a need for saying which
to_index that have issues.

V2: Adjust args to be aligned with trace_xdp_exception.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/trace/events/xdp.h |   24 ++++++++++++------------
 net/core/filter.c          |    6 +++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 0e42e69f773b..cd37706c6f55 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -51,33 +51,33 @@ TRACE_EVENT(xdp_exception,
 
 TRACE_EVENT(xdp_redirect,
 
-	TP_PROTO(const struct net_device *from,
-		 const struct net_device *to,
-		 const struct bpf_prog *xdp, u32 act, int err),
+	TP_PROTO(const struct net_device *dev,
+		 const struct bpf_prog *xdp, u32 act,
+		 int to_index, int err),
 
-	TP_ARGS(from, to, xdp, act, err),
+	TP_ARGS(dev, xdp, act, to_index, err),
 
 	TP_STRUCT__entry(
-		__string(name_from, from->name)
-		__string(name_to, to->name)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, ifindex)
+		__field(int, to_index)
 		__field(int, err)
 	),
 
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
-		__assign_str(name_from, from->name);
-		__assign_str(name_to, to->name);
-		__entry->act = act;
-		__entry->err = err;
+		__entry->act		= act;
+		__entry->ifindex	= dev->ifindex;
+		__entry->to_index	= to_index;
+		__entry->err		= err;
 	),
 
-	TP_printk("prog=%s from=%s to=%s action=%s err=%d",
+	TP_printk("prog=%s action=%s ifindex=%d to_index=%d 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),
+		  __entry->ifindex, __entry->to_index,
 		  __entry->err)
 );
 #endif /* _TRACE_XDP_H */
diff --git a/net/core/filter.c b/net/core/filter.c
index 2d7cdb2c5c66..2dbe985aecbb 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2553,7 +2553,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, err);
+	trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err);
 	return err;
 }
 
@@ -2577,7 +2577,7 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 
 	err = __bpf_tx_xdp(fwd, NULL, xdp, 0);
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
@@ -2611,7 +2611,7 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 
 	skb->dev = fwd;
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);

Re: [V2 PATCH net-next 4/5] xdp: remove net_device names from xdp_redirect tracepoint

[Daniel Borkmann]

On 08/23/2017 12:15 PM, Jesper Dangaard Brouer wrote:
> There is too much overhead in the current trace_xdp_redirect
> tracepoint as it does strcpy and strlen on the net_device names.
>
> Besides, exposing the ifindex/index is actually the information that
> is needed in the tracepoint to diagnose issues.  When a lookup fails
> (either ifindex or devmap index) then there is a need for saying which
> to_index that have issues.
>
> V2: Adjust args to be aligned with trace_xdp_exception.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

[V2 PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Jesper Dangaard Brouer]

Remove the net_device string name from the xdp_exception tracepoint,
like the xdp_redirect tracepoint.

Align the TP_STRUCT to have common entries between these two
tracepoint.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/trace/events/xdp.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index cd37706c6f55..27cf2ef35f5f 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception,
 	TP_ARGS(dev, xdp, act),
 
 	TP_STRUCT__entry(
-		__string(name, dev->name)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, ifindex)
 	),
 
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
-		__assign_str(name, dev->name);
-		__entry->act = act;
+		__entry->act		= act;
+		__entry->ifindex	= dev->ifindex;
 	),
 
-	TP_printk("prog=%s device=%s action=%s",
+	TP_printk("prog=%s action=%s ifindex=%d",
 		  __print_hex_str(__entry->prog_tag, 8),
-		  __get_str(name),
-		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB))
+		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB),
+		  __entry->ifindex)
 );
 
 TRACE_EVENT(xdp_redirect,

Re: [V2 PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Daniel Borkmann]

On 08/23/2017 12:15 PM, Jesper Dangaard Brouer wrote:
> Remove the net_device string name from the xdp_exception tracepoint,
> like the xdp_redirect tracepoint.
>
> Align the TP_STRUCT to have common entries between these two
> tracepoint.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [V2 PATCH net-next 0/5] xdp: more work on xdp tracepoints

[Jesper Dangaard Brouer]

On Wed, 23 Aug 2017 12:15:14 +0200
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> More work on streamlining the tracepoints for XDP.
> 
> V2: Change trace_xdp_redirect() to align with args of trace_xdp_exception()

Heads-up DaveM, just rebased my tree and noticed a merge conflict on
net-next... I'll send a V3

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

Re: [V2 PATCH net-next 0/5] xdp: more work on xdp tracepoints

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Wed, 23 Aug 2017 13:16:42 +0200

> On Wed, 23 Aug 2017 12:15:14 +0200
> Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> 
>> More work on streamlining the tracepoints for XDP.
>> 
>> V2: Change trace_xdp_redirect() to align with args of trace_xdp_exception()
> 
> Heads-up DaveM, just rebased my tree and noticed a merge conflict on
> net-next... I'll send a V3

Ok :)

[V3 PATCH net-next 0/5] xdp: more work on xdp tracepoints

[Jesper Dangaard Brouer]

More work on streamlining and performance optimizing the tracepoints
for XDP.

I've created a simple xdp_monitor application that uses this
tracepoint, and prints statistics. Available at github:

https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_kern.c
https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_user.c

The improvement over tracepoint with strcpy: 9810372 - 8428762 = +1381610 pps faster
 - (1/9810372 - 1/8428762)*10^9 = -16.7 nanosec
 - 100-(8428762/9810372*100) = strcpy-trace is 14.08% slower
 - 981037/8428762*100 = removing strcpy made it 11.64% faster

V3: Fix merge conflict with commit e4a8e817d3cb ("bpf: misc xdp redirect cleanups")
V2: Change trace_xdp_redirect() to align with args of trace_xdp_exception()

---

Jesper Dangaard Brouer (5):
      xdp: remove bpf_warn_invalid_xdp_redirect
      xdp: make generic xdp redirect use tracepoint trace_xdp_redirect
      ixgbe: use return codes from ndo_xdp_xmit that are distinguishable
      xdp: remove net_device names from xdp_redirect tracepoint
      xdp: get tracepoints xdp_exception and xdp_redirect in sync


 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 +-
 include/linux/filter.h                        |    3 +-
 include/trace/events/xdp.h                    |   36 ++++++++++---------
 net/core/dev.c                                |    4 +-
 net/core/filter.c                             |   47 +++++++++++++------------
 5 files changed, 49 insertions(+), 45 deletions(-)

[V3 PATCH net-next 1/5] xdp: remove bpf_warn_invalid_xdp_redirect

[Jesper Dangaard Brouer]

Given there is a tracepoint that can track the error code
of xdp_do_redirect calls, the WARN_ONCE in bpf_warn_invalid_xdp_redirect
doesn't seem relevant any longer.  Simply remove the function.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 net/core/filter.c |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 2a0d762a20d8..a5e5f31b41b9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2476,7 +2476,6 @@ static int __bpf_tx_xdp(struct net_device *dev,
 	int err;
 
 	if (!dev->netdev_ops->ndo_xdp_xmit) {
-		bpf_warn_invalid_xdp_redirect(dev->ifindex);
 		return -EOPNOTSUPP;
 	}
 
@@ -2543,7 +2542,6 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!fwd)) {
-		bpf_warn_invalid_xdp_redirect(index);
 		err = -EINVAL;
 		goto out;
 	}
@@ -2564,7 +2562,6 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb)
 	dev = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
 	if (unlikely(!dev)) {
-		bpf_warn_invalid_xdp_redirect(index);
 		return -EINVAL;
 	}
 
@@ -3565,11 +3562,6 @@ void bpf_warn_invalid_xdp_action(u32 act)
 }
 EXPORT_SYMBOL_GPL(bpf_warn_invalid_xdp_action);
 
-void bpf_warn_invalid_xdp_redirect(u32 ifindex)
-{
-	WARN_ONCE(1, "Illegal XDP redirect to unsupported device ifindex(%i)\n", ifindex);
-}
-
 static bool __is_valid_sock_ops_access(int off, int size)
 {
 	if (off < 0 || off >= sizeof(struct bpf_sock_ops))

[V3 PATCH net-next 2/5] xdp: make generic xdp redirect use tracepoint trace_xdp_redirect

[Jesper Dangaard Brouer]

If the xdp_do_generic_redirect() call fails, it trigger the
trace_xdp_exception tracepoint.  It seems better to use the same
tracepoint trace_xdp_redirect, as the native xdp_do_redirect{,_map} does.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/linux/filter.h |    3 ++-
 net/core/dev.c         |    4 ++--
 net/core/filter.c      |   35 +++++++++++++++++++++++------------
 3 files changed, 27 insertions(+), 15 deletions(-)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index 7015116331af..d29e58fde364 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -718,7 +718,8 @@ struct bpf_prog *bpf_patch_insn_single(struct bpf_prog *prog, u32 off,
  * because we only track one map and force a flush when the map changes.
  * This does not appear to be a real limitation for existing software.
  */
-int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb);
+int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
+			    struct bpf_prog *prog);
 int xdp_do_redirect(struct net_device *dev,
 		    struct xdp_buff *xdp,
 		    struct bpf_prog *prog);
diff --git a/net/core/dev.c b/net/core/dev.c
index 40b28e417072..270b54754821 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -3953,7 +3953,8 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 		if (act != XDP_PASS) {
 			switch (act) {
 			case XDP_REDIRECT:
-				err = xdp_do_generic_redirect(skb->dev, skb);
+				err = xdp_do_generic_redirect(skb->dev, skb,
+							      xdp_prog);
 				if (err)
 					goto out_redir;
 			/* fallthru to submit skb */
@@ -3966,7 +3967,6 @@ int do_xdp_generic(struct bpf_prog *xdp_prog, struct sk_buff *skb)
 	}
 	return XDP_PASS;
 out_redir:
-	trace_xdp_exception(skb->dev, xdp_prog, XDP_REDIRECT);
 	kfree_skb(skb);
 	return XDP_DROP;
 }
diff --git a/net/core/filter.c b/net/core/filter.c
index a5e5f31b41b9..a04680331033 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2553,26 +2553,37 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
 
-int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb)
+int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
+			    struct bpf_prog *xdp_prog)
 {
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
-	unsigned int len;
 	u32 index = ri->ifindex;
+	struct net_device *fwd;
+	unsigned int len;
+	int err = 0;
 
-	dev = dev_get_by_index_rcu(dev_net(dev), index);
+	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
-	if (unlikely(!dev)) {
-		return -EINVAL;
+	if (unlikely(!fwd)) {
+		err = -EINVAL;
+		goto out;
 	}
 
-	if (unlikely(!(dev->flags & IFF_UP)))
-		return -ENETDOWN;
-	len = dev->mtu + dev->hard_header_len + VLAN_HLEN;
-	if (skb->len > len)
-		return -E2BIG;
+	if (unlikely(!(fwd->flags & IFF_UP))) {
+		err = -ENETDOWN;
+		goto out;
+	}
 
-	skb->dev = dev;
-	return 0;
+	len = fwd->mtu + fwd->hard_header_len + VLAN_HLEN;
+	if (skb->len > len) {
+		err = -EMSGSIZE;
+		goto out;
+	}
+
+	skb->dev = fwd;
+out:
+	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);
 

[V3 PATCH net-next 3/5] ixgbe: use return codes from ndo_xdp_xmit that are distinguishable

[Jesper Dangaard Brouer]

For XDP_REDIRECT the use of return code -EINVAL is confusing, as it is
used in three different cases.  (1) When the index or ifindex lookup
fails, and in the ixgbe driver (2) when link is down and (3) when XDP
have not been enabled.

The return code can be picked up by the tracepoint xdp:xdp_redirect
for diagnosing why XDP_REDIRECT isn't working.  Thus, there is a need
different return codes to tell the issues apart.

I'm considering using a specific err-code scheme for XDP_REDIRECT
instead of using these errno codes.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 8d3224ad6434..d962368d08d0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)
 	int err;
 
 	if (unlikely(test_bit(__IXGBE_DOWN, &adapter->state)))
-		return -EINVAL;
+		return -ENETDOWN;
 
 	/* During program transitions its possible adapter->xdp_prog is assigned
 	 * but ring has not been configured yet. In this case simply abort xmit.
 	 */
 	ring = adapter->xdp_prog ? adapter->xdp_ring[smp_processor_id()] : NULL;
 	if (unlikely(!ring))
-		return -EINVAL;
+		return -ENXIO;
 
 	err = ixgbe_xmit_xdp_ring(adapter, xdp);
 	if (err != IXGBE_XDP_TX)

[V3 PATCH net-next 4/5] xdp: remove net_device names from xdp_redirect tracepoint

[Jesper Dangaard Brouer]

There is too much overhead in the current trace_xdp_redirect
tracepoint as it does strcpy and strlen on the net_device names.

Besides, exposing the ifindex/index is actually the information that
is needed in the tracepoint to diagnose issues.  When a lookup fails
(either ifindex or devmap index) then there is a need for saying which
to_index that have issues.

V2: Adjust args to be aligned with trace_xdp_exception.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/trace/events/xdp.h |   24 ++++++++++++------------
 net/core/filter.c          |    6 +++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 0e42e69f773b..cd37706c6f55 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -51,33 +51,33 @@ TRACE_EVENT(xdp_exception,
 
 TRACE_EVENT(xdp_redirect,
 
-	TP_PROTO(const struct net_device *from,
-		 const struct net_device *to,
-		 const struct bpf_prog *xdp, u32 act, int err),
+	TP_PROTO(const struct net_device *dev,
+		 const struct bpf_prog *xdp, u32 act,
+		 int to_index, int err),
 
-	TP_ARGS(from, to, xdp, act, err),
+	TP_ARGS(dev, xdp, act, to_index, err),
 
 	TP_STRUCT__entry(
-		__string(name_from, from->name)
-		__string(name_to, to->name)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, ifindex)
+		__field(int, to_index)
 		__field(int, err)
 	),
 
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
-		__assign_str(name_from, from->name);
-		__assign_str(name_to, to->name);
-		__entry->act = act;
-		__entry->err = err;
+		__entry->act		= act;
+		__entry->ifindex	= dev->ifindex;
+		__entry->to_index	= to_index;
+		__entry->err		= err;
 	),
 
-	TP_printk("prog=%s from=%s to=%s action=%s err=%d",
+	TP_printk("prog=%s action=%s ifindex=%d to_index=%d 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),
+		  __entry->ifindex, __entry->to_index,
 		  __entry->err)
 );
 #endif /* _TRACE_XDP_H */
diff --git a/net/core/filter.c b/net/core/filter.c
index a04680331033..4bcd6baa80c9 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2524,7 +2524,7 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
 	if (likely(!err))
 		ri->map_to_flush = map;
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err);
 	return err;
 }
 
@@ -2548,7 +2548,7 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
 
 	err = __bpf_tx_xdp(fwd, NULL, xdp, 0);
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_redirect);
@@ -2582,7 +2582,7 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 
 	skb->dev = fwd;
 out:
-	trace_xdp_redirect(dev, fwd, xdp_prog, XDP_REDIRECT, err);
+	trace_xdp_redirect(dev, xdp_prog, XDP_REDIRECT, index, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);

[V3 PATCH net-next 5/5] xdp: get tracepoints xdp_exception and xdp_redirect in sync

[Jesper Dangaard Brouer]

Remove the net_device string name from the xdp_exception tracepoint,
like the xdp_redirect tracepoint.

Align the TP_STRUCT to have common entries between these two
tracepoint.

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
 include/trace/events/xdp.h |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index cd37706c6f55..27cf2ef35f5f 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -31,22 +31,22 @@ TRACE_EVENT(xdp_exception,
 	TP_ARGS(dev, xdp, act),
 
 	TP_STRUCT__entry(
-		__string(name, dev->name)
 		__array(u8, prog_tag, 8)
 		__field(u32, act)
+		__field(int, ifindex)
 	),
 
 	TP_fast_assign(
 		BUILD_BUG_ON(sizeof(__entry->prog_tag) != sizeof(xdp->tag));
 		memcpy(__entry->prog_tag, xdp->tag, sizeof(xdp->tag));
-		__assign_str(name, dev->name);
-		__entry->act = act;
+		__entry->act		= act;
+		__entry->ifindex	= dev->ifindex;
 	),
 
-	TP_printk("prog=%s device=%s action=%s",
+	TP_printk("prog=%s action=%s ifindex=%d",
 		  __print_hex_str(__entry->prog_tag, 8),
-		  __get_str(name),
-		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB))
+		  __print_symbolic(__entry->act, __XDP_ACT_SYM_TAB),
+		  __entry->ifindex)
 );
 
 TRACE_EVENT(xdp_redirect,

Re: [V3 PATCH net-next 0/5] xdp: more work on xdp tracepoints

[John Fastabend]

On 08/24/2017 03:32 AM, Jesper Dangaard Brouer wrote:
> More work on streamlining and performance optimizing the tracepoints
> for XDP.
> 
> I've created a simple xdp_monitor application that uses this
> tracepoint, and prints statistics. Available at github:
> 
> https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_kern.c
> https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_user.c
> 
> The improvement over tracepoint with strcpy: 9810372 - 8428762 = +1381610 pps faster
>  - (1/9810372 - 1/8428762)*10^9 = -16.7 nanosec
>  - 100-(8428762/9810372*100) = strcpy-trace is 14.08% slower
>  - 981037/8428762*100 = removing strcpy made it 11.64% faster
> 
> V3: Fix merge conflict with commit e4a8e817d3cb ("bpf: misc xdp redirect cleanups")
> V2: Change trace_xdp_redirect() to align with args of trace_xdp_exception()
> 
> ---

Series looks good to me. Thanks a lot.

Re: [V3 PATCH net-next 0/5] xdp: more work on xdp tracepoints

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Thu, 24 Aug 2017 12:32:58 +0200

> More work on streamlining and performance optimizing the tracepoints
> for XDP.
> 
> I've created a simple xdp_monitor application that uses this
> tracepoint, and prints statistics. Available at github:
> 
> https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_kern.c
> https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/samples/bpf/xdp_monitor_user.c
> 
> The improvement over tracepoint with strcpy: 9810372 - 8428762 = +1381610 pps faster
>  - (1/9810372 - 1/8428762)*10^9 = -16.7 nanosec
>  - 100-(8428762/9810372*100) = strcpy-trace is 14.08% slower
>  - 981037/8428762*100 = removing strcpy made it 11.64% faster
> 
> V3: Fix merge conflict with commit e4a8e817d3cb ("bpf: misc xdp redirect cleanups")
> V2: Change trace_xdp_redirect() to align with args of trace_xdp_exception()

Series applied, thanks Jesper.