[PATCH bpf] uapi/bpf: Remove text about bpf_redirect_map() giving higher performance

[PATCH bpf] uapi/bpf: Remove text about bpf_redirect_map() giving higher performance

[Toke Høiland-Jørgensen]

The performance of bpf_redirect() is now roughly the same as that of
bpf_redirect_map(). However, David Ahern pointed out that the header file
has not been updated to reflect this, and still says that a significant
performance increase is possible when using bpf_redirect_map(). Remove this
text from the bpf_redirect_map() description, and reword the description in
bpf_redirect() slightly.

Fixes: 1d233886dd90 ("xdp: Use bulking for non-map XDP_REDIRECT and consolidate code paths")
Suggested-by: David Ahern <dsahern@gmail.com>
Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 include/uapi/linux/bpf.h | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index f1d74a2bd234..7a526d917fb3 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1045,9 +1045,9 @@ union bpf_attr {
  * 		supports redirection to the egress interface, and accepts no
  * 		flag at all.
  *
- * 		The same effect can be attained with the more generic
- * 		**bpf_redirect_map**\ (), which requires specific maps to be
- * 		used but offers better performance.
+ * 		The same effect can also be attained with the more generic
+ * 		**bpf_redirect_map**\ (), which uses a BPF map to store the
+ * 		redirect target instead of providing it directly to the helper.
  * 	Return
  * 		For XDP, the helper returns **XDP_REDIRECT** on success or
  * 		**XDP_ABORTED** on error. For other program types, the values
@@ -1610,12 +1610,6 @@ union bpf_attr {
  * 		one of the XDP program return codes up to XDP_TX, as chosen by
  * 		the caller. Any higher bits in the *flags* argument must be
  * 		unset.
- *
- * 		When used to redirect packets to net devices, this helper
- * 		provides a high performance increase over **bpf_redirect**\ ().
- * 		This is due to various implementation details of the underlying
- * 		mechanisms, one of which is the fact that **bpf_redirect_map**\
- * 		() tries to send packet as a "bulk" to the device.
  * 	Return
  * 		**XDP_REDIRECT** on success, or **XDP_ABORTED** on error.
  *
-- 
2.25.0

Re: [PATCH bpf] uapi/bpf: Remove text about bpf_redirect_map() giving higher performance

[Quentin Monnet]

2020-02-18 08:46 UTC+0100 ~ Toke Høiland-Jørgensen <toke@redhat.com>
> The performance of bpf_redirect() is now roughly the same as that of
> bpf_redirect_map(). However, David Ahern pointed out that the header file
> has not been updated to reflect this, and still says that a significant
> performance increase is possible when using bpf_redirect_map(). Remove this
> text from the bpf_redirect_map() description, and reword the description in
> bpf_redirect() slightly.
> 
> Fixes: 1d233886dd90 ("xdp: Use bulking for non-map XDP_REDIRECT and consolidate code paths")
> Suggested-by: David Ahern <dsahern@gmail.com>
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>   include/uapi/linux/bpf.h | 12 +++---------
>   1 file changed, 3 insertions(+), 9 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index f1d74a2bd234..7a526d917fb3 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1045,9 +1045,9 @@ union bpf_attr {
>    * 		supports redirection to the egress interface, and accepts no
>    * 		flag at all.
>    *
> - * 		The same effect can be attained with the more generic
> - * 		**bpf_redirect_map**\ (), which requires specific maps to be
> - * 		used but offers better performance.
> + * 		The same effect can also be attained with the more generic
> + * 		**bpf_redirect_map**\ (), which uses a BPF map to store the
> + * 		redirect target instead of providing it directly to the helper.
>    * 	Return
>    * 		For XDP, the helper returns **XDP_REDIRECT** on success or
>    * 		**XDP_ABORTED** on error. For other program types, the values
> @@ -1610,12 +1610,6 @@ union bpf_attr {
>    * 		one of the XDP program return codes up to XDP_TX, as chosen by
>    * 		the caller. Any higher bits in the *flags* argument must be
>    * 		unset.
> - *
> - * 		When used to redirect packets to net devices, this helper
> - * 		provides a high performance increase over **bpf_redirect**\ ().
> - * 		This is due to various implementation details of the underlying
> - * 		mechanisms, one of which is the fact that **bpf_redirect_map**\
> - * 		() tries to send packet as a "bulk" to the device.
>    * 	Return
>    * 		**XDP_REDIRECT** on success, or **XDP_ABORTED** on error.
>    *
> 

We could maybe leave something like “See also bpf_redirect()" in the 
description of "bpf_redirect_map()"?

Either way,
Reviewed-by: Quentin Monnet <quentin@isovalent.com>

Thanks,
Quentin

Re: [PATCH bpf] uapi/bpf: Remove text about bpf_redirect_map() giving higher performance

[Toke Høiland-Jørgensen]

Quentin Monnet <quentin@isovalent.com> writes:

> 2020-02-18 08:46 UTC+0100 ~ Toke Høiland-Jørgensen <toke@redhat.com>
>> The performance of bpf_redirect() is now roughly the same as that of
>> bpf_redirect_map(). However, David Ahern pointed out that the header file
>> has not been updated to reflect this, and still says that a significant
>> performance increase is possible when using bpf_redirect_map(). Remove this
>> text from the bpf_redirect_map() description, and reword the description in
>> bpf_redirect() slightly.
>> 
>> Fixes: 1d233886dd90 ("xdp: Use bulking for non-map XDP_REDIRECT and consolidate code paths")
>> Suggested-by: David Ahern <dsahern@gmail.com>
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>   include/uapi/linux/bpf.h | 12 +++---------
>>   1 file changed, 3 insertions(+), 9 deletions(-)
>> 
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index f1d74a2bd234..7a526d917fb3 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -1045,9 +1045,9 @@ union bpf_attr {
>>    * 		supports redirection to the egress interface, and accepts no
>>    * 		flag at all.
>>    *
>> - * 		The same effect can be attained with the more generic
>> - * 		**bpf_redirect_map**\ (), which requires specific maps to be
>> - * 		used but offers better performance.
>> + * 		The same effect can also be attained with the more generic
>> + * 		**bpf_redirect_map**\ (), which uses a BPF map to store the
>> + * 		redirect target instead of providing it directly to the helper.
>>    * 	Return
>>    * 		For XDP, the helper returns **XDP_REDIRECT** on success or
>>    * 		**XDP_ABORTED** on error. For other program types, the values
>> @@ -1610,12 +1610,6 @@ union bpf_attr {
>>    * 		one of the XDP program return codes up to XDP_TX, as chosen by
>>    * 		the caller. Any higher bits in the *flags* argument must be
>>    * 		unset.
>> - *
>> - * 		When used to redirect packets to net devices, this helper
>> - * 		provides a high performance increase over **bpf_redirect**\ ().
>> - * 		This is due to various implementation details of the underlying
>> - * 		mechanisms, one of which is the fact that **bpf_redirect_map**\
>> - * 		() tries to send packet as a "bulk" to the device.
>>    * 	Return
>>    * 		**XDP_REDIRECT** on success, or **XDP_ABORTED** on error.
>>    *
>> 
>
> We could maybe leave something like “See also bpf_redirect()" in the 
> description of "bpf_redirect_map()"?

Sure, why not? I see that I also forgot to fix up the 'Return' part of
bpf_redirect_map(), so I'll send a v2.

> Either way,
> Reviewed-by: Quentin Monnet <quentin@isovalent.com>

Thanks!

-Toke