| Message ID | 20210330113419.4616-3-ciara.loftus@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | AF_XDP Socket Creation Fixes | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for bpf |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | fail | 1 blamed authors not CCed: daniel@iogearbox.net; 9 maintainers not CCed: jonathan.lemon@gmail.com yhs@fb.com kpsingh@kernel.org daniel@iogearbox.net andrii@kernel.org kafai@fb.com ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | fail | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 71 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Tue, Mar 30, 2021 at 5:06 AM Ciara Loftus <ciara.loftus@intel.com> wrote: > > If the call to xsk_socket__create fails, the user may want to retry the > socket creation using the same umem. Ensure that the umem is in the > same state on exit if the call fails by: > 1. ensuring the umem _save pointers are unmodified. > 2. not unmapping the set of umem rings that were set up with the umem > during xsk_umem__create, since those maps existed before the call to > xsk_socket__create and should remain in tact even in the event of > failure. > > Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices") > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/lib/bpf/xsk.c | 29 ++++++++++++++++------------- > 1 file changed, 16 insertions(+), 13 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 443b0cfb45e8..d4991ddff05a 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -743,21 +743,23 @@ static struct xsk_ctx *xsk_get_ctx(struct xsk_umem *umem, int ifindex, > return NULL; > } > > -static void xsk_put_ctx(struct xsk_ctx *ctx) > +static void xsk_put_ctx(struct xsk_ctx *ctx, bool unmap) > { > struct xsk_umem *umem = ctx->umem; > struct xdp_mmap_offsets off; > int err; > > if (--ctx->refcount == 0) { > - err = xsk_get_mmap_offsets(umem->fd, &off); > - if (!err) { > - munmap(ctx->fill->ring - off.fr.desc, > - off.fr.desc + umem->config.fill_size * > - sizeof(__u64)); > - munmap(ctx->comp->ring - off.cr.desc, > - off.cr.desc + umem->config.comp_size * > - sizeof(__u64)); > + if (unmap) { > + err = xsk_get_mmap_offsets(umem->fd, &off); > + if (!err) { > + munmap(ctx->fill->ring - off.fr.desc, > + off.fr.desc + umem->config.fill_size * > + sizeof(__u64)); > + munmap(ctx->comp->ring - off.cr.desc, > + off.cr.desc + umem->config.comp_size * > + sizeof(__u64)); > + } The whole function increases indent, since it changes anyway could you write it as: { if (--ctx->refcount) return; if (!unmap) goto out_free; err = xsk_get if (err) goto out_free; munmap(); out_free: list_del free } other than this it looks fine to me. Bjorn, Magnus, please review. > } > > list_del(&ctx->list); > @@ -797,8 +799,6 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk, > memcpy(ctx->ifname, ifname, IFNAMSIZ - 1); > ctx->ifname[IFNAMSIZ - 1] = '\0'; > > - umem->fill_save = NULL; > - umem->comp_save = NULL; > ctx->fill = fill; > ctx->comp = comp; > list_add(&ctx->list, &umem->ctx_list); > @@ -854,6 +854,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > struct xsk_socket *xsk; > struct xsk_ctx *ctx; > int err, ifindex; > + bool unmap = umem->fill_save != fill; > > if (!umem || !xsk_ptr || !(rx || tx)) > return -EFAULT; > @@ -994,6 +995,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > } > > *xsk_ptr = xsk; > + umem->fill_save = NULL; > + umem->comp_save = NULL; > return 0; > > out_mmap_tx: > @@ -1005,7 +1008,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > munmap(rx_map, off.rx.desc + > xsk->config.rx_size * sizeof(struct xdp_desc)); > out_put_ctx: > - xsk_put_ctx(ctx); > + xsk_put_ctx(ctx, unmap); > out_socket: > if (--umem->refcount) > close(xsk->fd); > @@ -1071,7 +1074,7 @@ void xsk_socket__delete(struct xsk_socket *xsk) > } > } > > - xsk_put_ctx(ctx); > + xsk_put_ctx(ctx, true); > > umem->refcount--; > /* Do not close an fd that also has an associated umem connected > -- > 2.17.1 >
On Tue, 30 Mar 2021 at 17:08, Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Tue, Mar 30, 2021 at 5:06 AM Ciara Loftus <ciara.loftus@intel.com> wrote: > > [...] > > if (--ctx->refcount == 0) { > > - err = xsk_get_mmap_offsets(umem->fd, &off); > > - if (!err) { > > - munmap(ctx->fill->ring - off.fr.desc, > > - off.fr.desc + umem->config.fill_size * > > - sizeof(__u64)); > > - munmap(ctx->comp->ring - off.cr.desc, > > - off.cr.desc + umem->config.comp_size * > > - sizeof(__u64)); > > + if (unmap) { > > + err = xsk_get_mmap_offsets(umem->fd, &off); > > + if (!err) { > > + munmap(ctx->fill->ring - off.fr.desc, > > + off.fr.desc + umem->config.fill_size * > > + sizeof(__u64)); > > + munmap(ctx->comp->ring - off.cr.desc, > > + off.cr.desc + umem->config.comp_size * > > + sizeof(__u64)); > > + } > > The whole function increases indent, since it changes anyway > could you write it as: > { > if (--ctx->refcount) > return; > if (!unmap) > goto out_free; > err = xsk_get > if (err) > goto out_free; > munmap(); > out_free: > list_del > free > } > Yes, please try to reduce the nesting, and while at it try to expand the as much as possible of the munmap arguments to the full 100 chars. Björn
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 443b0cfb45e8..d4991ddff05a 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -743,21 +743,23 @@ static struct xsk_ctx *xsk_get_ctx(struct xsk_umem *umem, int ifindex, return NULL; } -static void xsk_put_ctx(struct xsk_ctx *ctx) +static void xsk_put_ctx(struct xsk_ctx *ctx, bool unmap) { struct xsk_umem *umem = ctx->umem; struct xdp_mmap_offsets off; int err; if (--ctx->refcount == 0) { - err = xsk_get_mmap_offsets(umem->fd, &off); - if (!err) { - munmap(ctx->fill->ring - off.fr.desc, - off.fr.desc + umem->config.fill_size * - sizeof(__u64)); - munmap(ctx->comp->ring - off.cr.desc, - off.cr.desc + umem->config.comp_size * - sizeof(__u64)); + if (unmap) { + err = xsk_get_mmap_offsets(umem->fd, &off); + if (!err) { + munmap(ctx->fill->ring - off.fr.desc, + off.fr.desc + umem->config.fill_size * + sizeof(__u64)); + munmap(ctx->comp->ring - off.cr.desc, + off.cr.desc + umem->config.comp_size * + sizeof(__u64)); + } } list_del(&ctx->list); @@ -797,8 +799,6 @@ static struct xsk_ctx *xsk_create_ctx(struct xsk_socket *xsk, memcpy(ctx->ifname, ifname, IFNAMSIZ - 1); ctx->ifname[IFNAMSIZ - 1] = '\0'; - umem->fill_save = NULL; - umem->comp_save = NULL; ctx->fill = fill; ctx->comp = comp; list_add(&ctx->list, &umem->ctx_list); @@ -854,6 +854,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, struct xsk_socket *xsk; struct xsk_ctx *ctx; int err, ifindex; + bool unmap = umem->fill_save != fill; if (!umem || !xsk_ptr || !(rx || tx)) return -EFAULT; @@ -994,6 +995,8 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, } *xsk_ptr = xsk; + umem->fill_save = NULL; + umem->comp_save = NULL; return 0; out_mmap_tx: @@ -1005,7 +1008,7 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, munmap(rx_map, off.rx.desc + xsk->config.rx_size * sizeof(struct xdp_desc)); out_put_ctx: - xsk_put_ctx(ctx); + xsk_put_ctx(ctx, unmap); out_socket: if (--umem->refcount) close(xsk->fd); @@ -1071,7 +1074,7 @@ void xsk_socket__delete(struct xsk_socket *xsk) } } - xsk_put_ctx(ctx); + xsk_put_ctx(ctx, true); umem->refcount--; /* Do not close an fd that also has an associated umem connected
If the call to xsk_socket__create fails, the user may want to retry the socket creation using the same umem. Ensure that the umem is in the same state on exit if the call fails by: 1. ensuring the umem _save pointers are unmodified. 2. not unmapping the set of umem rings that were set up with the umem during xsk_umem__create, since those maps existed before the call to xsk_socket__create and should remain in tact even in the event of failure. Fixes: 2f6324a3937f ("libbpf: Support shared umems between queues and devices") Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- tools/lib/bpf/xsk.c | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-)