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.chttps://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(-)
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 <redacted>
---
net/core/filter.c | 8 --------
1 file changed, 8 deletions(-)
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 <redacted>
---
include/linux/filter.h | 3 ++-
net/core/dev.c | 4 ++--
net/core/filter.c | 36 ++++++++++++++++++++++--------------
3 files changed, 26 insertions(+), 17 deletions(-)
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 <redacted>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)interr;if(unlikely(test_bit(__IXGBE_DOWN,&adapter->state)))-return-EINVAL;+return-ENOLINK;/* During program transitions its possible adapter->xdp_prog is assigned*butringhasnotbeenconfiguredyet.Inthiscasesimplyabortxmit.*/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)
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 <redacted>
---
include/trace/events/xdp.h | 17 ++++++++---------
net/core/filter.c | 6 +++---
2 files changed, 11 insertions(+), 12 deletions(-)
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 <redacted>
---
include/trace/events/xdp.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-22 21:19:42
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 <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-22 21:21:40
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 <redacted>
Makes sense to make this consistent.
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-22 21:22:00
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 <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-22 21:23:08
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 <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-22 21:30:23
On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
quoted hunk
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 <redacted>
---
include/trace/events/xdp.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
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.
On Tue, 22 Aug 2017 23:30:21 +0200
Daniel Borkmann [off-list ref] wrote:
On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
quoted
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 <redacted>
---
include/trace/events/xdp.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
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
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-23 08:54:18
On 08/23/2017 09:41 AM, Jesper Dangaard Brouer wrote:
On Tue, 22 Aug 2017 23:30:21 +0200
Daniel Borkmann [off-list ref] wrote:
quoted
On 08/22/2017 10:47 PM, Jesper Dangaard Brouer wrote:
quoted
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 <redacted>
---
include/trace/events/xdp.h | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
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.
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.chttps://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(-)
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 <redacted>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/filter.c | 8 --------
1 file changed, 8 deletions(-)
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 <redacted>
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(-)
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 <redacted>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)interr;if(unlikely(test_bit(__IXGBE_DOWN,&adapter->state)))-return-EINVAL;+return-ENOLINK;/* During program transitions its possible adapter->xdp_prog is assigned*butringhasnotbeenconfiguredyet.Inthiscasesimplyabortxmit.*/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)
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 <redacted>
---
include/trace/events/xdp.h | 24 ++++++++++++------------
net/core/filter.c | 6 +++---
2 files changed, 15 insertions(+), 15 deletions(-)
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 <redacted>
---
include/trace/events/xdp.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-23 10:59:31
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 <redacted>
From: Daniel Borkmann <daniel@iogearbox.net> Date: 2017-08-23 10:59:51
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 <redacted>
On Wed, 23 Aug 2017 12:15:14 +0200
Jesper Dangaard Brouer [off-list ref] 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
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.chttps://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(-)
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 <redacted>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
net/core/filter.c | 8 --------
1 file changed, 8 deletions(-)
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 <redacted>
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(-)
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 <redacted>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
@@ -9849,14 +9849,14 @@ static int ixgbe_xdp_xmit(struct net_device *dev, struct xdp_buff *xdp)interr;if(unlikely(test_bit(__IXGBE_DOWN,&adapter->state)))-return-EINVAL;+return-ENETDOWN;/* During program transitions its possible adapter->xdp_prog is assigned*butringhasnotbeenconfiguredyet.Inthiscasesimplyabortxmit.*/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)
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 <redacted>
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(-)
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 <redacted>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
---
include/trace/events/xdp.h | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
From: John Fastabend <john.fastabend@gmail.com> Date: 2017-08-24 14:46:25
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.chttps://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()
---
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.chttps://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()