| 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 | expand |
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)
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
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 >
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 > >
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!
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
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);
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(-)