* [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 related [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 related [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, other threads:[~2019-09-16 13:11 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).