BPF Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
@ 2019-09-09 17:46 Toke Høiland-Jørgensen
  2019-09-09 17:53 ` Yonghong Song
  0 siblings, 1 reply; 7+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-09-09 17:46 UTC (permalink / raw)
  To: Alexei Starovoitov, Daniel Borkmann, netdev, bpf
  Cc: Toke Høiland-Jørgensen

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);
-- 
2.23.0


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
  2019-09-09 17:46 [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS Toke Høiland-Jørgensen
@ 2019-09-09 17:53 ` Yonghong Song
  2019-09-09 23:06   ` Toke Høiland-Jørgensen
  0 siblings, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2019-09-09 17:53 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, Alexei Starovoitov,
	Daniel Borkmann, netdev, bpf



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)

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
  2019-09-09 17:53 ` Yonghong Song
@ 2019-09-09 23:06   ` Toke Høiland-Jørgensen
  2019-09-13 18:53     ` Yonghong Song
  0 siblings, 1 reply; 7+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-09-09 23:06 UTC (permalink / raw)
  To: Yonghong Song, Alexei Starovoitov, Daniel Borkmann, netdev\, bpf\

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
  2019-09-09 23:06   ` Toke Høiland-Jørgensen
@ 2019-09-13 18:53     ` Yonghong Song
  2019-09-16  5:09       ` Björn Töpel
  2019-09-16  8:08       ` Daniel Borkmann
  0 siblings, 2 replies; 7+ messages in thread
From: Yonghong Song @ 2019-09-13 18:53 UTC (permalink / raw)
  To: Toke Høiland-Jørgensen, Alexei Starovoitov,
	Daniel Borkmann, netdev, bpf, maximmi



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
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
  2019-09-13 18:53     ` Yonghong Song
@ 2019-09-16  5:09       ` Björn Töpel
  2019-09-16  8:08       ` Daniel Borkmann
  1 sibling, 0 replies; 7+ messages in thread
From: Björn Töpel @ 2019-09-16  5:09 UTC (permalink / raw)
  To: Yonghong Song
  Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
	Daniel Borkmann, netdev, bpf, maximmi, Karlsson, Magnus

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
  2019-09-13 18:53     ` Yonghong Song
  2019-09-16  5:09       ` Björn Töpel
@ 2019-09-16  8:08       ` Daniel Borkmann
  2019-09-16 12:31         ` Toke Høiland-Jørgensen
  1 sibling, 1 reply; 7+ messages in thread
From: Daniel Borkmann @ 2019-09-16  8:08 UTC (permalink / raw)
  To: Yonghong Song, Toke Høiland-Jørgensen,
	Alexei Starovoitov, netdev, bpf, maximmi

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!

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS
  2019-09-16  8:08       ` Daniel Borkmann
@ 2019-09-16 12:31         ` Toke Høiland-Jørgensen
  0 siblings, 0 replies; 7+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-09-16 12:31 UTC (permalink / raw)
  To: Daniel Borkmann, Yonghong Song, Alexei Starovoitov, netdev\, bpf\,
	maximmi\

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, back to index

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-09 17:46 [PATCH] libbpf: Don't error out if getsockopt() fails for XDP_OPTIONS Toke Høiland-Jørgensen
2019-09-09 17:53 ` Yonghong Song
2019-09-09 23:06   ` Toke Høiland-Jørgensen
2019-09-13 18:53     ` Yonghong Song
2019-09-16  5:09       ` Björn Töpel
2019-09-16  8:08       ` Daniel Borkmann
2019-09-16 12:31         ` Toke Høiland-Jørgensen

BPF Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/bpf/0 bpf/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 bpf bpf/ https://lore.kernel.org/bpf \
		bpf@vger.kernel.org bpf@archiver.kernel.org
	public-inbox-index bpf

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.bpf


AGPL code for this site: git clone https://public-inbox.org/ public-inbox