libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
diff mbox series

Message ID 20190909174619.1735-1-toke@redhat.com
State Changes Requested
Delegated to: BPF Maintainers
Headers show
Series
  • libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
Related show

Commit Message

Toke Høiland-Jørgensen Sept. 9, 2019, 5:46 p.m. UTC
The xsk_socket__create() function fails and returns an error if it cannot
get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
was not added until kernel 5.3, so this means that creating XSK sockets
always fails on older kernels.

Since the option is just used to set the zero-copy flag in the xsk struct,
there really is no need to error out if the getsockopt() call fails.

Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
---
 tools/lib/bpf/xsk.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

Comments

Yonghong Song Sept. 9, 2019, 5:53 p.m. UTC | #1
On 9/9/19 10:46 AM, Toke Høiland-Jørgensen wrote:
> The xsk_socket__create() function fails and returns an error if it cannot
> get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
> was not added until kernel 5.3, so this means that creating XSK sockets
> always fails on older kernels.
> 
> Since the option is just used to set the zero-copy flag in the xsk struct,
> there really is no need to error out if the getsockopt() call fails.
> 
> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> ---
>   tools/lib/bpf/xsk.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index 680e63066cf3..598e487d9ce8 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -603,12 +603,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>   
>   	optlen = sizeof(opts);
>   	err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
> -	if (err) {
> -		err = -errno;
> -		goto out_mmap_tx;
> -	}
> -
> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
> +	if (!err)
> +		xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>   
>   	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>   		err = xsk_setup_xdp_prog(xsk);

Since 'zc' is not used by anybody, maybe all codes 'zc' related can be 
removed? It can be added back back once there is an interface to use 'zc'?

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 842c4fd55859..24fa313524fb 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -65,7 +65,6 @@ struct xsk_socket {
         int xsks_map_fd;
         __u32 queue_id;
         char ifname[IFNAMSIZ];
-       bool zc;
  };

  struct xsk_nl_info {
@@ -491,7 +490,6 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, 
const char *ifname,
         void *rx_map = NULL, *tx_map = NULL;
         struct sockaddr_xdp sxdp = {};
         struct xdp_mmap_offsets off;
-       struct xdp_options opts;
         struct xsk_socket *xsk;
         socklen_t optlen;
         int err;
@@ -611,15 +609,6 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, 
const char *ifname,

         xsk->prog_fd = -1;

-       optlen = sizeof(opts);
-       err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
-       if (err) {
-               err = -errno;
-               goto out_mmap_tx;
-       }
-
-       xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
-
         if (!(xsk->config.libbpf_flags & 
XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
                 err = xsk_setup_xdp_prog(xsk);
                 if (err)
Toke Høiland-Jørgensen Sept. 9, 2019, 11:06 p.m. UTC | #2
Yonghong Song <yhs@fb.com> writes:

> On 9/9/19 10:46 AM, Toke Høiland-Jørgensen wrote:
>> The xsk_socket__create() function fails and returns an error if it cannot
>> get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
>> was not added until kernel 5.3, so this means that creating XSK sockets
>> always fails on older kernels.
>> 
>> Since the option is just used to set the zero-copy flag in the xsk struct,
>> there really is no need to error out if the getsockopt() call fails.
>> 
>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> ---
>>   tools/lib/bpf/xsk.c | 8 ++------
>>   1 file changed, 2 insertions(+), 6 deletions(-)
>> 
>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>> index 680e63066cf3..598e487d9ce8 100644
>> --- a/tools/lib/bpf/xsk.c
>> +++ b/tools/lib/bpf/xsk.c
>> @@ -603,12 +603,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>>   
>>   	optlen = sizeof(opts);
>>   	err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
>> -	if (err) {
>> -		err = -errno;
>> -		goto out_mmap_tx;
>> -	}
>> -
>> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>> +	if (!err)
>> +		xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>   
>>   	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>>   		err = xsk_setup_xdp_prog(xsk);
>
> Since 'zc' is not used by anybody, maybe all codes 'zc' related can be 
> removed? It can be added back back once there is an interface to use
> 'zc'?

Fine with me; up to the maintainers what they prefer, I guess? :)

-Toke
Yonghong Song Sept. 13, 2019, 6:53 p.m. UTC | #3
On 9/10/19 12:06 AM, Toke Høiland-Jørgensen wrote:
> Yonghong Song <yhs@fb.com> writes:
> 
>> On 9/9/19 10:46 AM, Toke Høiland-Jørgensen wrote:
>>> The xsk_socket__create() function fails and returns an error if it cannot
>>> get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
>>> was not added until kernel 5.3, so this means that creating XSK sockets
>>> always fails on older kernels.
>>>
>>> Since the option is just used to set the zero-copy flag in the xsk struct,
>>> there really is no need to error out if the getsockopt() call fails.
>>>
>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>> ---
>>>    tools/lib/bpf/xsk.c | 8 ++------
>>>    1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>>> index 680e63066cf3..598e487d9ce8 100644
>>> --- a/tools/lib/bpf/xsk.c
>>> +++ b/tools/lib/bpf/xsk.c
>>> @@ -603,12 +603,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>>>    
>>>    	optlen = sizeof(opts);
>>>    	err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
>>> -	if (err) {
>>> -		err = -errno;
>>> -		goto out_mmap_tx;
>>> -	}
>>> -
>>> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>> +	if (!err)
>>> +		xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>>    
>>>    	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>>>    		err = xsk_setup_xdp_prog(xsk);
>>
>> Since 'zc' is not used by anybody, maybe all codes 'zc' related can be
>> removed? It can be added back back once there is an interface to use
>> 'zc'?
> 
> Fine with me; up to the maintainers what they prefer, I guess? :)

Maxim,

Your originally introduced `'zc' and getting XDP_OPTIONS.
What is your opinion of how to deal with the unused xsk->zc?

> 
> -Toke
>
Björn Töpel Sept. 16, 2019, 5:09 a.m. UTC | #4
On Fri, 13 Sep 2019 at 21:46, Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 9/10/19 12:06 AM, Toke Høiland-Jørgensen wrote:
> > Yonghong Song <yhs@fb.com> writes:
> >
> >> On 9/9/19 10:46 AM, Toke Høiland-Jørgensen wrote:
> >>> The xsk_socket__create() function fails and returns an error if it cannot
> >>> get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
> >>> was not added until kernel 5.3, so this means that creating XSK sockets
> >>> always fails on older kernels.
> >>>
> >>> Since the option is just used to set the zero-copy flag in the xsk struct,
> >>> there really is no need to error out if the getsockopt() call fails.
> >>>
> >>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >>> ---
> >>>    tools/lib/bpf/xsk.c | 8 ++------
> >>>    1 file changed, 2 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> >>> index 680e63066cf3..598e487d9ce8 100644
> >>> --- a/tools/lib/bpf/xsk.c
> >>> +++ b/tools/lib/bpf/xsk.c
> >>> @@ -603,12 +603,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
> >>>
> >>>     optlen = sizeof(opts);
> >>>     err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
> >>> -   if (err) {
> >>> -           err = -errno;
> >>> -           goto out_mmap_tx;
> >>> -   }
> >>> -
> >>> -   xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
> >>> +   if (!err)
> >>> +           xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
> >>>
> >>>     if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
> >>>             err = xsk_setup_xdp_prog(xsk);
> >>
> >> Since 'zc' is not used by anybody, maybe all codes 'zc' related can be
> >> removed? It can be added back back once there is an interface to use
> >> 'zc'?
> >
> > Fine with me; up to the maintainers what they prefer, I guess? :)
>
> Maxim,
>
> Your originally introduced `'zc' and getting XDP_OPTIONS.
> What is your opinion of how to deal with the unused xsk->zc?
>

This was previously discussed here [1]. The TL;DR version is "stay
tuned for a proper interface". :-)


Björn

[1] https://lore.kernel.org/bpf/CAJ8uoz1qhaHwebmjOOS9xfJe93Eq0v=SXhQUnjHv7imVL3ONsQ@mail.gmail.com/#t



> >
> > -Toke
> >
Daniel Borkmann Sept. 16, 2019, 8:08 a.m. UTC | #5
On 9/13/19 8:53 PM, Yonghong Song wrote:
> On 9/10/19 12:06 AM, Toke Høiland-Jørgensen wrote:
>> Yonghong Song <yhs@fb.com> writes:
>>> On 9/9/19 10:46 AM, Toke Høiland-Jørgensen wrote:
>>>> The xsk_socket__create() function fails and returns an error if it cannot
>>>> get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
>>>> was not added until kernel 5.3, so this means that creating XSK sockets
>>>> always fails on older kernels.
>>>>
>>>> Since the option is just used to set the zero-copy flag in the xsk struct,
>>>> there really is no need to error out if the getsockopt() call fails.
>>>>
>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>> ---
>>>>     tools/lib/bpf/xsk.c | 8 ++------
>>>>     1 file changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>>>> index 680e63066cf3..598e487d9ce8 100644
>>>> --- a/tools/lib/bpf/xsk.c
>>>> +++ b/tools/lib/bpf/xsk.c
>>>> @@ -603,12 +603,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>>>>     
>>>>     	optlen = sizeof(opts);
>>>>     	err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
>>>> -	if (err) {
>>>> -		err = -errno;
>>>> -		goto out_mmap_tx;
>>>> -	}
>>>> -
>>>> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>>> +	if (!err)
>>>> +		xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>>>     
>>>>     	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>>>>     		err = xsk_setup_xdp_prog(xsk);
>>>
>>> Since 'zc' is not used by anybody, maybe all codes 'zc' related can be
>>> removed? It can be added back back once there is an interface to use
>>> 'zc'?
>>
>> Fine with me; up to the maintainers what they prefer, I guess? :)

Given this is not exposed to applications at this point and we don't do anything
useful with it, lets just remove the zc cruft until there is a proper interface
added to libbpf. Toke, please respin with the suggested removal, thanks!
Toke Høiland-Jørgensen Sept. 16, 2019, 12:31 p.m. UTC | #6
Daniel Borkmann <daniel@iogearbox.net> writes:

> On 9/13/19 8:53 PM, Yonghong Song wrote:
>> On 9/10/19 12:06 AM, Toke Høiland-Jørgensen wrote:
>>> Yonghong Song <yhs@fb.com> writes:
>>>> On 9/9/19 10:46 AM, Toke Høiland-Jørgensen wrote:
>>>>> The xsk_socket__create() function fails and returns an error if it cannot
>>>>> get the XDP_OPTIONS through getsockopt(). However, support for XDP_OPTIONS
>>>>> was not added until kernel 5.3, so this means that creating XSK sockets
>>>>> always fails on older kernels.
>>>>>
>>>>> Since the option is just used to set the zero-copy flag in the xsk struct,
>>>>> there really is no need to error out if the getsockopt() call fails.
>>>>>
>>>>> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com>
>>>>> ---
>>>>>     tools/lib/bpf/xsk.c | 8 ++------
>>>>>     1 file changed, 2 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
>>>>> index 680e63066cf3..598e487d9ce8 100644
>>>>> --- a/tools/lib/bpf/xsk.c
>>>>> +++ b/tools/lib/bpf/xsk.c
>>>>> @@ -603,12 +603,8 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
>>>>>     
>>>>>     	optlen = sizeof(opts);
>>>>>     	err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
>>>>> -	if (err) {
>>>>> -		err = -errno;
>>>>> -		goto out_mmap_tx;
>>>>> -	}
>>>>> -
>>>>> -	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>>>> +	if (!err)
>>>>> +		xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
>>>>>     
>>>>>     	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
>>>>>     		err = xsk_setup_xdp_prog(xsk);
>>>>
>>>> Since 'zc' is not used by anybody, maybe all codes 'zc' related can be
>>>> removed? It can be added back back once there is an interface to use
>>>> 'zc'?
>>>
>>> Fine with me; up to the maintainers what they prefer, I guess? :)
>
> Given this is not exposed to applications at this point and we don't do anything
> useful with it, lets just remove the zc cruft until there is a proper interface
> added to libbpf. Toke, please respin with the suggested removal, thanks!

Sure, can do :)

-Toke

Patch
diff mbox series

diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
index 680e63066cf3..598e487d9ce8 100644
--- a/tools/lib/bpf/xsk.c
+++ b/tools/lib/bpf/xsk.c
@@ -603,12 +603,8 @@  int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname,
 
 	optlen = sizeof(opts);
 	err = getsockopt(xsk->fd, SOL_XDP, XDP_OPTIONS, &opts, &optlen);
-	if (err) {
-		err = -errno;
-		goto out_mmap_tx;
-	}
-
-	xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
+	if (!err)
+		xsk->zc = opts.flags & XDP_OPTIONS_ZEROCOPY;
 
 	if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) {
 		err = xsk_setup_xdp_prog(xsk);