diff mbox

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

Message ID 150343485998.31091.4036615069956176401.stgit@firesoul
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jesper Dangaard Brouer Aug. 22, 2017, 8:47 p.m. UTC 
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(-)

Comments

Daniel Borkmann Aug. 22, 2017, 9:30 p.m. UTC | #1 
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.
Jesper Dangaard Brouer Aug. 23, 2017, 7:41 a.m. UTC | #2 
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.
Daniel Borkmann Aug. 23, 2017, 8:54 a.m. UTC | #3 
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!
Jesper Dangaard Brouer Aug. 23, 2017, 10:15 a.m. UTC | #4 
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(-)

--
Jesper Dangaard Brouer Aug. 23, 2017, 11:16 a.m. UTC | #5 
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
David Miller Aug. 24, 2017, 12:07 a.m. UTC | #6 
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 :)
Jesper Dangaard Brouer Aug. 24, 2017, 10:32 a.m. UTC | #7 
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(-)

--
John Fastabend Aug. 24, 2017, 2:46 p.m. UTC | #8 
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.
David Miller Aug. 24, 2017, 7 p.m. UTC | #9 
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.
diff mbox

Patch

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