* [PATCH v3 bpf-next 0/2] Better handling of xskmap entries
@ 2019-05-31 18:57 Jonathan Lemon
2019-05-31 18:57 ` [PATCH v3 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon
2019-05-31 18:57 ` [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon
0 siblings, 2 replies; 8+ messages in thread
From: Jonathan Lemon @ 2019-05-31 18:57 UTC (permalink / raw)
To: netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson
Respin of v2 to fix stupid error.
Testing with samples/bpf/xdpsock:
[root@kerneltest004.06.atn2 ~]# ./xdpsock -i eth0 -q 0
sock0@eth0:0 rxdrop
pps pkts 1.00
rx 17 17
tx 0 0
Jonathan Lemon (2):
bpf: Allow bpf_map_lookup_elem() on an xskmap
libbpf: remove qidconf and better support external bpf programs.
include/net/xdp_sock.h | 6 +-
kernel/bpf/verifier.c | 6 +-
kernel/bpf/xskmap.c | 4 +-
tools/lib/bpf/xsk.c | 79 ++++---------------
.../bpf/verifier/prevent_map_lookup.c | 15 ----
5 files changed, 27 insertions(+), 83 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH v3 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-05-31 18:57 [PATCH v3 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon @ 2019-05-31 18:57 ` Jonathan Lemon 2019-06-01 23:06 ` Song Liu 2019-05-31 18:57 ` [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Lemon @ 2019-05-31 18:57 UTC (permalink / raw) To: netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson Currently, the AF_XDP code uses a separate map in order to determine if an xsk is bound to a queue. Instead of doing this, have bpf_map_lookup_elem() return the queue_id, as a way of indicating that there is a valid entry at the map index. Rearrange some xdp_sock members to eliminate structure holes. Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> Acked-by: Björn Töpel <bjorn.topel@intel.com> --- include/net/xdp_sock.h | 6 +++--- kernel/bpf/verifier.c | 6 +++++- kernel/bpf/xskmap.c | 4 +++- .../selftests/bpf/verifier/prevent_map_lookup.c | 15 --------------- 4 files changed, 11 insertions(+), 20 deletions(-) diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h index d074b6d60f8a..7d84b1da43d2 100644 --- a/include/net/xdp_sock.h +++ b/include/net/xdp_sock.h @@ -57,12 +57,12 @@ struct xdp_sock { struct net_device *dev; struct xdp_umem *umem; struct list_head flush_node; - u16 queue_id; - struct xsk_queue *tx ____cacheline_aligned_in_smp; - struct list_head list; + u32 queue_id; bool zc; /* Protects multiple processes in the control path */ struct mutex mutex; + struct xsk_queue *tx ____cacheline_aligned_in_smp; + struct list_head list; /* Mutual exclusion of NAPI TX thread and sendmsg error paths * in the SKB destructor callback. */ diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 2778417e6e0c..91c730f85e92 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -2905,10 +2905,14 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env, * appear. */ case BPF_MAP_TYPE_CPUMAP: - case BPF_MAP_TYPE_XSKMAP: if (func_id != BPF_FUNC_redirect_map) goto error; break; + case BPF_MAP_TYPE_XSKMAP: + if (func_id != BPF_FUNC_redirect_map && + func_id != BPF_FUNC_map_lookup_elem) + goto error; + break; case BPF_MAP_TYPE_ARRAY_OF_MAPS: case BPF_MAP_TYPE_HASH_OF_MAPS: if (func_id != BPF_FUNC_map_lookup_elem) diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c index 686d244e798d..249b22089014 100644 --- a/kernel/bpf/xskmap.c +++ b/kernel/bpf/xskmap.c @@ -154,7 +154,9 @@ void __xsk_map_flush(struct bpf_map *map) static void *xsk_map_lookup_elem(struct bpf_map *map, void *key) { - return ERR_PTR(-EOPNOTSUPP); + struct xdp_sock *xs = __xsk_map_lookup_elem(map, *(u32 *)key); + + return xs ? &xs->queue_id : NULL; } static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value, diff --git a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c index bbdba990fefb..da7a4b37cb98 100644 --- a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c +++ b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c @@ -28,21 +28,6 @@ .errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem", .prog_type = BPF_PROG_TYPE_SOCK_OPS, }, -{ - "prevent map lookup in xskmap", - .insns = { - BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), - BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), - BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), - BPF_LD_MAP_FD(BPF_REG_1, 0), - BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), - BPF_EXIT_INSN(), - }, - .fixup_map_xskmap = { 3 }, - .result = REJECT, - .errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem", - .prog_type = BPF_PROG_TYPE_XDP, -}, { "prevent map lookup in stack trace", .insns = { -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap 2019-05-31 18:57 ` [PATCH v3 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon @ 2019-06-01 23:06 ` Song Liu 0 siblings, 0 replies; 8+ messages in thread From: Song Liu @ 2019-06-01 23:06 UTC (permalink / raw) To: Jonathan Lemon; +Cc: Networking, Kernel Team, bjorn.topel, magnus.karlsson > On May 31, 2019, at 11:57 AM, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > Currently, the AF_XDP code uses a separate map in order to > determine if an xsk is bound to a queue. Instead of doing this, > have bpf_map_lookup_elem() return the queue_id, as a way of > indicating that there is a valid entry at the map index. > > Rearrange some xdp_sock members to eliminate structure holes. > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > Acked-by: Björn Töpel <bjorn.topel@intel.com> Acked-by: Song Liu <songliubraving@fb.com> > --- > include/net/xdp_sock.h | 6 +++--- > kernel/bpf/verifier.c | 6 +++++- > kernel/bpf/xskmap.c | 4 +++- > .../selftests/bpf/verifier/prevent_map_lookup.c | 15 --------------- > 4 files changed, 11 insertions(+), 20 deletions(-) > > diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h > index d074b6d60f8a..7d84b1da43d2 100644 > --- a/include/net/xdp_sock.h > +++ b/include/net/xdp_sock.h > @@ -57,12 +57,12 @@ struct xdp_sock { > struct net_device *dev; > struct xdp_umem *umem; > struct list_head flush_node; > - u16 queue_id; > - struct xsk_queue *tx ____cacheline_aligned_in_smp; > - struct list_head list; > + u32 queue_id; > bool zc; > /* Protects multiple processes in the control path */ > struct mutex mutex; > + struct xsk_queue *tx ____cacheline_aligned_in_smp; > + struct list_head list; > /* Mutual exclusion of NAPI TX thread and sendmsg error paths > * in the SKB destructor callback. > */ > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 2778417e6e0c..91c730f85e92 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -2905,10 +2905,14 @@ static int check_map_func_compatibility(struct bpf_verifier_env *env, > * appear. > */ > case BPF_MAP_TYPE_CPUMAP: > - case BPF_MAP_TYPE_XSKMAP: > if (func_id != BPF_FUNC_redirect_map) > goto error; > break; > + case BPF_MAP_TYPE_XSKMAP: > + if (func_id != BPF_FUNC_redirect_map && > + func_id != BPF_FUNC_map_lookup_elem) > + goto error; > + break; > case BPF_MAP_TYPE_ARRAY_OF_MAPS: > case BPF_MAP_TYPE_HASH_OF_MAPS: > if (func_id != BPF_FUNC_map_lookup_elem) > diff --git a/kernel/bpf/xskmap.c b/kernel/bpf/xskmap.c > index 686d244e798d..249b22089014 100644 > --- a/kernel/bpf/xskmap.c > +++ b/kernel/bpf/xskmap.c > @@ -154,7 +154,9 @@ void __xsk_map_flush(struct bpf_map *map) > > static void *xsk_map_lookup_elem(struct bpf_map *map, void *key) > { > - return ERR_PTR(-EOPNOTSUPP); > + struct xdp_sock *xs = __xsk_map_lookup_elem(map, *(u32 *)key); > + > + return xs ? &xs->queue_id : NULL; > } > > static int xsk_map_update_elem(struct bpf_map *map, void *key, void *value, > diff --git a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c > index bbdba990fefb..da7a4b37cb98 100644 > --- a/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c > +++ b/tools/testing/selftests/bpf/verifier/prevent_map_lookup.c > @@ -28,21 +28,6 @@ > .errstr = "cannot pass map_type 18 into func bpf_map_lookup_elem", > .prog_type = BPF_PROG_TYPE_SOCK_OPS, > }, > -{ > - "prevent map lookup in xskmap", > - .insns = { > - BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0), > - BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > - BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8), > - BPF_LD_MAP_FD(BPF_REG_1, 0), > - BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_map_lookup_elem), > - BPF_EXIT_INSN(), > - }, > - .fixup_map_xskmap = { 3 }, > - .result = REJECT, > - .errstr = "cannot pass map_type 17 into func bpf_map_lookup_elem", > - .prog_type = BPF_PROG_TYPE_XDP, > -}, > { > "prevent map lookup in stack trace", > .insns = { > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs. 2019-05-31 18:57 [PATCH v3 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon 2019-05-31 18:57 ` [PATCH v3 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon @ 2019-05-31 18:57 ` Jonathan Lemon 2019-06-01 23:05 ` Song Liu 1 sibling, 1 reply; 8+ messages in thread From: Jonathan Lemon @ 2019-05-31 18:57 UTC (permalink / raw) To: netdev; +Cc: kernel-team, bjorn.topel, magnus.karlsson Use the recent change to XSKMAP bpf_map_lookup_elem() to test if there is a xsk present in the map instead of duplicating the work with qidconf. Fix things so callers using XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD bypass any internal bpf maps, so xsk_socket__{create|delete} works properly. Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> --- tools/lib/bpf/xsk.c | 79 +++++++++------------------------------------ 1 file changed, 16 insertions(+), 63 deletions(-) diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 38667b62f1fe..7ce7494b5b50 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -60,10 +60,8 @@ struct xsk_socket { struct xsk_umem *umem; struct xsk_socket_config config; int fd; - int xsks_map; int ifindex; int prog_fd; - int qidconf_map_fd; int xsks_map_fd; __u32 queue_id; char ifname[IFNAMSIZ]; @@ -265,15 +263,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) /* This is the C-program: * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx) * { - * int *qidconf, index = ctx->rx_queue_index; + * int index = ctx->rx_queue_index; * * // A set entry here means that the correspnding queue_id * // has an active AF_XDP socket bound to it. - * qidconf = bpf_map_lookup_elem(&qidconf_map, &index); - * if (!qidconf) - * return XDP_ABORTED; - * - * if (*qidconf) + * if (bpf_map_lookup_elem(&xsks_map, &index)) * return bpf_redirect_map(&xsks_map, index, 0); * * return XDP_PASS; @@ -286,15 +280,10 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_1, -4), BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), - BPF_LD_MAP_FD(BPF_REG_1, xsk->qidconf_map_fd), + BPF_LD_MAP_FD(BPF_REG_1, xsk->xsks_map_fd), BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), - BPF_MOV32_IMM(BPF_REG_0, 0), - /* if r1 == 0 goto +8 */ - BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 8), BPF_MOV32_IMM(BPF_REG_0, 2), - /* r1 = *(u32 *)(r1 + 0) */ - BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0), /* if r1 == 0 goto +5 */ BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 5), /* r2 = *(u32 *)(r10 - 4) */ @@ -366,18 +355,11 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk) if (max_queues < 0) return max_queues; - fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "qidconf_map", + fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", sizeof(int), sizeof(int), max_queues, 0); if (fd < 0) return fd; - xsk->qidconf_map_fd = fd; - fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", - sizeof(int), sizeof(int), max_queues, 0); - if (fd < 0) { - close(xsk->qidconf_map_fd); - return fd; - } xsk->xsks_map_fd = fd; return 0; @@ -385,10 +367,8 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk) static void xsk_delete_bpf_maps(struct xsk_socket *xsk) { - close(xsk->qidconf_map_fd); + bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); close(xsk->xsks_map_fd); - xsk->qidconf_map_fd = -1; - xsk->xsks_map_fd = -1; } static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) @@ -417,10 +397,9 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) if (err) goto out_map_ids; - for (i = 0; i < prog_info.nr_map_ids; i++) { - if (xsk->qidconf_map_fd != -1 && xsk->xsks_map_fd != -1) - break; + xsk->xsks_map_fd = -1; + for (i = 0; i < prog_info.nr_map_ids; i++) { fd = bpf_map_get_fd_by_id(map_ids[i]); if (fd < 0) continue; @@ -431,11 +410,6 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) continue; } - if (!strcmp(map_info.name, "qidconf_map")) { - xsk->qidconf_map_fd = fd; - continue; - } - if (!strcmp(map_info.name, "xsks_map")) { xsk->xsks_map_fd = fd; continue; @@ -445,40 +419,18 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) } err = 0; - if (xsk->qidconf_map_fd < 0 || xsk->xsks_map_fd < 0) { + if (xsk->xsks_map_fd == -1) err = -ENOENT; - xsk_delete_bpf_maps(xsk); - } out_map_ids: free(map_ids); return err; } -static void xsk_clear_bpf_maps(struct xsk_socket *xsk) -{ - int qid = false; - - bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); - bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); -} - static int xsk_set_bpf_maps(struct xsk_socket *xsk) { - int qid = true, fd = xsk->fd, err; - - err = bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); - if (err) - goto out; - - err = bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, &fd, 0); - if (err) - goto out; - - return 0; -out: - xsk_clear_bpf_maps(xsk); - return err; + return bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, + &xsk->fd, 0); } static int xsk_setup_xdp_prog(struct xsk_socket *xsk) @@ -514,6 +466,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket *xsk) out_load: close(xsk->prog_fd); + xsk->prog_fd = -1; out_maps: xsk_delete_bpf_maps(xsk); return err; @@ -643,9 +596,7 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname, goto out_mmap_tx; } - xsk->qidconf_map_fd = -1; - xsk->xsks_map_fd = -1; - + xsk->prog_fd = -1; if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { err = xsk_setup_xdp_prog(xsk); if (err) @@ -708,8 +659,10 @@ void xsk_socket__delete(struct xsk_socket *xsk) if (!xsk) return; - xsk_clear_bpf_maps(xsk); - xsk_delete_bpf_maps(xsk); + if (xsk->prog_fd != -1) { + xsk_delete_bpf_maps(xsk); + close(xsk->prog_fd); + } optlen = sizeof(off); err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen); -- 2.17.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs. 2019-05-31 18:57 ` [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon @ 2019-06-01 23:05 ` Song Liu 2019-06-02 4:18 ` Jonathan Lemon 0 siblings, 1 reply; 8+ messages in thread From: Song Liu @ 2019-06-01 23:05 UTC (permalink / raw) To: Jonathan Lemon; +Cc: Networking, Kernel Team, bjorn.topel, magnus.karlsson > On May 31, 2019, at 11:57 AM, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > Use the recent change to XSKMAP bpf_map_lookup_elem() to test if > there is a xsk present in the map instead of duplicating the work > with qidconf. > > Fix things so callers using XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD > bypass any internal bpf maps, so xsk_socket__{create|delete} works > properly. > > Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > --- > tools/lib/bpf/xsk.c | 79 +++++++++------------------------------------ > 1 file changed, 16 insertions(+), 63 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 38667b62f1fe..7ce7494b5b50 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -60,10 +60,8 @@ struct xsk_socket { > struct xsk_umem *umem; > struct xsk_socket_config config; > int fd; > - int xsks_map; > int ifindex; > int prog_fd; > - int qidconf_map_fd; > int xsks_map_fd; > __u32 queue_id; > char ifname[IFNAMSIZ]; > @@ -265,15 +263,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) > /* This is the C-program: > * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx) > * { > - * int *qidconf, index = ctx->rx_queue_index; > + * int index = ctx->rx_queue_index; > * > * // A set entry here means that the correspnding queue_id > * // has an active AF_XDP socket bound to it. > - * qidconf = bpf_map_lookup_elem(&qidconf_map, &index); > - * if (!qidconf) > - * return XDP_ABORTED; > - * > - * if (*qidconf) > + * if (bpf_map_lookup_elem(&xsks_map, &index)) > * return bpf_redirect_map(&xsks_map, index, 0); > * > * return XDP_PASS; > @@ -286,15 +280,10 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) > BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_1, -4), > BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), > - BPF_LD_MAP_FD(BPF_REG_1, xsk->qidconf_map_fd), > + BPF_LD_MAP_FD(BPF_REG_1, xsk->xsks_map_fd), > BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), > BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), > - BPF_MOV32_IMM(BPF_REG_0, 0), > - /* if r1 == 0 goto +8 */ > - BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 8), > BPF_MOV32_IMM(BPF_REG_0, 2), > - /* r1 = *(u32 *)(r1 + 0) */ > - BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0), > /* if r1 == 0 goto +5 */ > BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 5), > /* r2 = *(u32 *)(r10 - 4) */ > @@ -366,18 +355,11 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk) > if (max_queues < 0) > return max_queues; > > - fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "qidconf_map", > + fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", > sizeof(int), sizeof(int), max_queues, 0); > if (fd < 0) > return fd; > - xsk->qidconf_map_fd = fd; > > - fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", > - sizeof(int), sizeof(int), max_queues, 0); > - if (fd < 0) { > - close(xsk->qidconf_map_fd); > - return fd; > - } > xsk->xsks_map_fd = fd; > > return 0; > @@ -385,10 +367,8 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk) > > static void xsk_delete_bpf_maps(struct xsk_socket *xsk) > { > - close(xsk->qidconf_map_fd); > + bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); > close(xsk->xsks_map_fd); > - xsk->qidconf_map_fd = -1; > - xsk->xsks_map_fd = -1; > } > > static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > @@ -417,10 +397,9 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > if (err) > goto out_map_ids; > > - for (i = 0; i < prog_info.nr_map_ids; i++) { > - if (xsk->qidconf_map_fd != -1 && xsk->xsks_map_fd != -1) > - break; > + xsk->xsks_map_fd = -1; > > + for (i = 0; i < prog_info.nr_map_ids; i++) { > fd = bpf_map_get_fd_by_id(map_ids[i]); > if (fd < 0) > continue; > @@ -431,11 +410,6 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > continue; > } > > - if (!strcmp(map_info.name, "qidconf_map")) { > - xsk->qidconf_map_fd = fd; > - continue; > - } > - > if (!strcmp(map_info.name, "xsks_map")) { > xsk->xsks_map_fd = fd; > continue; > @@ -445,40 +419,18 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > } > > err = 0; > - if (xsk->qidconf_map_fd < 0 || xsk->xsks_map_fd < 0) { > + if (xsk->xsks_map_fd == -1) > err = -ENOENT; > - xsk_delete_bpf_maps(xsk); > - } > > out_map_ids: > free(map_ids); > return err; > } > > -static void xsk_clear_bpf_maps(struct xsk_socket *xsk) > -{ > - int qid = false; > - > - bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); > - bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); > -} > - > static int xsk_set_bpf_maps(struct xsk_socket *xsk) > { > - int qid = true, fd = xsk->fd, err; > - > - err = bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); > - if (err) > - goto out; > - > - err = bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, &fd, 0); > - if (err) > - goto out; > - > - return 0; > -out: > - xsk_clear_bpf_maps(xsk); > - return err; > + return bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, > + &xsk->fd, 0); > } > > static int xsk_setup_xdp_prog(struct xsk_socket *xsk) > @@ -514,6 +466,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket *xsk) > > out_load: > close(xsk->prog_fd); > + xsk->prog_fd = -1; I found xsk->prog_fd confusing. Why do we need to set it here? I think we don't need to call xsk_delete_bpf_maps() in out_load path? > out_maps: > xsk_delete_bpf_maps(xsk); > return err; > @@ -643,9 +596,7 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname, > goto out_mmap_tx; > } > > - xsk->qidconf_map_fd = -1; > - xsk->xsks_map_fd = -1; > - > + xsk->prog_fd = -1; > if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { > err = xsk_setup_xdp_prog(xsk); > if (err) > @@ -708,8 +659,10 @@ void xsk_socket__delete(struct xsk_socket *xsk) > if (!xsk) > return; > > - xsk_clear_bpf_maps(xsk); > - xsk_delete_bpf_maps(xsk); > + if (xsk->prog_fd != -1) { > + xsk_delete_bpf_maps(xsk); > + close(xsk->prog_fd); Here, we use prog_fd != -1 to gate xsk_delete_bpf_maps(), which is confusing. I looked at the code for quite sometime, but still cannot confirm it is correct. Thanks, Song > + } > > optlen = sizeof(off); > err = getsockopt(xsk->fd, SOL_XDP, XDP_MMAP_OFFSETS, &off, &optlen); > -- > 2.17.1 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs. 2019-06-01 23:05 ` Song Liu @ 2019-06-02 4:18 ` Jonathan Lemon 2019-06-02 6:55 ` Song Liu 2019-06-03 11:11 ` Björn Töpel 0 siblings, 2 replies; 8+ messages in thread From: Jonathan Lemon @ 2019-06-02 4:18 UTC (permalink / raw) To: Song Liu; +Cc: Networking, Kernel Team, bjorn.topel, magnus.karlsson On 1 Jun 2019, at 16:05, Song Liu wrote: >> On May 31, 2019, at 11:57 AM, Jonathan Lemon >> <jonathan.lemon@gmail.com> wrote: >> >> Use the recent change to XSKMAP bpf_map_lookup_elem() to test if >> there is a xsk present in the map instead of duplicating the work >> with qidconf. >> >> Fix things so callers using XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD >> bypass any internal bpf maps, so xsk_socket__{create|delete} works >> properly. >> >> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> >> --- >> tools/lib/bpf/xsk.c | 79 >> +++++++++------------------------------------ >> 1 file changed, 16 insertions(+), 63 deletions(-) >> >> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c >> index 38667b62f1fe..7ce7494b5b50 100644 >> --- a/tools/lib/bpf/xsk.c >> +++ b/tools/lib/bpf/xsk.c >> @@ -60,10 +60,8 @@ struct xsk_socket { >> struct xsk_umem *umem; >> struct xsk_socket_config config; >> int fd; >> - int xsks_map; >> int ifindex; >> int prog_fd; >> - int qidconf_map_fd; >> int xsks_map_fd; >> __u32 queue_id; >> char ifname[IFNAMSIZ]; >> @@ -265,15 +263,11 @@ static int xsk_load_xdp_prog(struct xsk_socket >> *xsk) >> /* This is the C-program: >> * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx) >> * { >> - * int *qidconf, index = ctx->rx_queue_index; >> + * int index = ctx->rx_queue_index; >> * >> * // A set entry here means that the correspnding queue_id >> * // has an active AF_XDP socket bound to it. >> - * qidconf = bpf_map_lookup_elem(&qidconf_map, &index); >> - * if (!qidconf) >> - * return XDP_ABORTED; >> - * >> - * if (*qidconf) >> + * if (bpf_map_lookup_elem(&xsks_map, &index)) >> * return bpf_redirect_map(&xsks_map, index, 0); >> * >> * return XDP_PASS; >> @@ -286,15 +280,10 @@ static int xsk_load_xdp_prog(struct xsk_socket >> *xsk) >> BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_1, -4), >> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), >> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), >> - BPF_LD_MAP_FD(BPF_REG_1, xsk->qidconf_map_fd), >> + BPF_LD_MAP_FD(BPF_REG_1, xsk->xsks_map_fd), >> BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), >> BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), >> - BPF_MOV32_IMM(BPF_REG_0, 0), >> - /* if r1 == 0 goto +8 */ >> - BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 8), >> BPF_MOV32_IMM(BPF_REG_0, 2), >> - /* r1 = *(u32 *)(r1 + 0) */ >> - BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0), >> /* if r1 == 0 goto +5 */ >> BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 5), >> /* r2 = *(u32 *)(r10 - 4) */ >> @@ -366,18 +355,11 @@ static int xsk_create_bpf_maps(struct >> xsk_socket *xsk) >> if (max_queues < 0) >> return max_queues; >> >> - fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "qidconf_map", >> + fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", >> sizeof(int), sizeof(int), max_queues, 0); >> if (fd < 0) >> return fd; >> - xsk->qidconf_map_fd = fd; >> >> - fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", >> - sizeof(int), sizeof(int), max_queues, 0); >> - if (fd < 0) { >> - close(xsk->qidconf_map_fd); >> - return fd; >> - } >> xsk->xsks_map_fd = fd; >> >> return 0; >> @@ -385,10 +367,8 @@ static int xsk_create_bpf_maps(struct xsk_socket >> *xsk) >> >> static void xsk_delete_bpf_maps(struct xsk_socket *xsk) >> { >> - close(xsk->qidconf_map_fd); >> + bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); >> close(xsk->xsks_map_fd); >> - xsk->qidconf_map_fd = -1; >> - xsk->xsks_map_fd = -1; >> } >> >> static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) >> @@ -417,10 +397,9 @@ static int xsk_lookup_bpf_maps(struct xsk_socket >> *xsk) >> if (err) >> goto out_map_ids; >> >> - for (i = 0; i < prog_info.nr_map_ids; i++) { >> - if (xsk->qidconf_map_fd != -1 && xsk->xsks_map_fd != -1) >> - break; >> + xsk->xsks_map_fd = -1; >> >> + for (i = 0; i < prog_info.nr_map_ids; i++) { >> fd = bpf_map_get_fd_by_id(map_ids[i]); >> if (fd < 0) >> continue; >> @@ -431,11 +410,6 @@ static int xsk_lookup_bpf_maps(struct xsk_socket >> *xsk) >> continue; >> } >> >> - if (!strcmp(map_info.name, "qidconf_map")) { >> - xsk->qidconf_map_fd = fd; >> - continue; >> - } >> - >> if (!strcmp(map_info.name, "xsks_map")) { >> xsk->xsks_map_fd = fd; >> continue; >> @@ -445,40 +419,18 @@ static int xsk_lookup_bpf_maps(struct >> xsk_socket *xsk) >> } >> >> err = 0; >> - if (xsk->qidconf_map_fd < 0 || xsk->xsks_map_fd < 0) { >> + if (xsk->xsks_map_fd == -1) >> err = -ENOENT; >> - xsk_delete_bpf_maps(xsk); >> - } >> >> out_map_ids: >> free(map_ids); >> return err; >> } >> >> -static void xsk_clear_bpf_maps(struct xsk_socket *xsk) >> -{ >> - int qid = false; >> - >> - bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); >> - bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); >> -} >> - >> static int xsk_set_bpf_maps(struct xsk_socket *xsk) >> { >> - int qid = true, fd = xsk->fd, err; >> - >> - err = bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, >> &qid, 0); >> - if (err) >> - goto out; >> - >> - err = bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, &fd, >> 0); >> - if (err) >> - goto out; >> - >> - return 0; >> -out: >> - xsk_clear_bpf_maps(xsk); >> - return err; >> + return bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, >> + &xsk->fd, 0); >> } >> >> static int xsk_setup_xdp_prog(struct xsk_socket *xsk) >> @@ -514,6 +466,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket >> *xsk) >> >> out_load: >> close(xsk->prog_fd); >> + xsk->prog_fd = -1; > > I found xsk->prog_fd confusing. Why do we need to set it here? I suppose this one isn't strictly required - I set it as a guard out of habit. xsk is (currently) immediately freed by the caller, so it can be removed. The main logic is: xsk->prog_fd = -1; if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { err = xsk_setup_xdp_prog(xsk); The user may pass INHIBIT_PROG_LOAD, which bypasses setting up the xdp program (and any maps associated with the program), allowing installation of a custom program. The cleanup behavior is then gated on prog_fd being -1, > > I think we don't need to call xsk_delete_bpf_maps() in out_load path? Hmm, there's two out_load paths, but only one needs the delete maps call. Let me redo the error handling so it's a bit more explicit. > >> out_maps: >> xsk_delete_bpf_maps(xsk); >> return err; >> @@ -643,9 +596,7 @@ int xsk_socket__create(struct xsk_socket >> **xsk_ptr, const char *ifname, >> goto out_mmap_tx; >> } >> >> - xsk->qidconf_map_fd = -1; >> - xsk->xsks_map_fd = -1; >> - >> + xsk->prog_fd = -1; >> if (!(xsk->config.libbpf_flags & >> XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { >> err = xsk_setup_xdp_prog(xsk); >> if (err) >> @@ -708,8 +659,10 @@ void xsk_socket__delete(struct xsk_socket *xsk) >> if (!xsk) >> return; >> >> - xsk_clear_bpf_maps(xsk); >> - xsk_delete_bpf_maps(xsk); >> + if (xsk->prog_fd != -1) { >> + xsk_delete_bpf_maps(xsk); >> + close(xsk->prog_fd); > > Here, we use prog_fd != -1 to gate xsk_delete_bpf_maps(), which is > confusing. I looked at the code for quite sometime, but still cannot > confirm it is correct. See above reasoning - with INHIBIT_PROG_LOAD, there is no library-provided program or maps, so cleanup actions are skipped. -- Jonathan ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs. 2019-06-02 4:18 ` Jonathan Lemon @ 2019-06-02 6:55 ` Song Liu 2019-06-03 11:11 ` Björn Töpel 1 sibling, 0 replies; 8+ messages in thread From: Song Liu @ 2019-06-02 6:55 UTC (permalink / raw) To: Jonathan Lemon; +Cc: Networking, Kernel Team, bjorn.topel, magnus.karlsson > On Jun 1, 2019, at 9:18 PM, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > On 1 Jun 2019, at 16:05, Song Liu wrote: > >>> On May 31, 2019, at 11:57 AM, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: >>> >>> Use the recent change to XSKMAP bpf_map_lookup_elem() to test if >>> there is a xsk present in the map instead of duplicating the work >>> with qidconf. >>> >>> Fix things so callers using XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD >>> bypass any internal bpf maps, so xsk_socket__{create|delete} works >>> properly. >>> >>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> >>> --- >>> tools/lib/bpf/xsk.c | 79 +++++++++------------------------------------ >>> 1 file changed, 16 insertions(+), 63 deletions(-) >>> >>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c >>> index 38667b62f1fe..7ce7494b5b50 100644 >>> --- a/tools/lib/bpf/xsk.c >>> +++ b/tools/lib/bpf/xsk.c >>> @@ -60,10 +60,8 @@ struct xsk_socket { >>> struct xsk_umem *umem; >>> struct xsk_socket_config config; >>> int fd; >>> - int xsks_map; >>> int ifindex; >>> int prog_fd; >>> - int qidconf_map_fd; >>> int xsks_map_fd; >>> __u32 queue_id; >>> char ifname[IFNAMSIZ]; >>> @@ -265,15 +263,11 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) >>> /* This is the C-program: >>> * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx) >>> * { >>> - * int *qidconf, index = ctx->rx_queue_index; >>> + * int index = ctx->rx_queue_index; >>> * >>> * // A set entry here means that the correspnding queue_id >>> * // has an active AF_XDP socket bound to it. >>> - * qidconf = bpf_map_lookup_elem(&qidconf_map, &index); >>> - * if (!qidconf) >>> - * return XDP_ABORTED; >>> - * >>> - * if (*qidconf) >>> + * if (bpf_map_lookup_elem(&xsks_map, &index)) >>> * return bpf_redirect_map(&xsks_map, index, 0); >>> * >>> * return XDP_PASS; >>> @@ -286,15 +280,10 @@ static int xsk_load_xdp_prog(struct xsk_socket *xsk) >>> BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_1, -4), >>> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), >>> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), >>> - BPF_LD_MAP_FD(BPF_REG_1, xsk->qidconf_map_fd), >>> + BPF_LD_MAP_FD(BPF_REG_1, xsk->xsks_map_fd), >>> BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), >>> BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), >>> - BPF_MOV32_IMM(BPF_REG_0, 0), >>> - /* if r1 == 0 goto +8 */ >>> - BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 8), >>> BPF_MOV32_IMM(BPF_REG_0, 2), >>> - /* r1 = *(u32 *)(r1 + 0) */ >>> - BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0), >>> /* if r1 == 0 goto +5 */ >>> BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 5), >>> /* r2 = *(u32 *)(r10 - 4) */ >>> @@ -366,18 +355,11 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk) >>> if (max_queues < 0) >>> return max_queues; >>> >>> - fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "qidconf_map", >>> + fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", >>> sizeof(int), sizeof(int), max_queues, 0); >>> if (fd < 0) >>> return fd; >>> - xsk->qidconf_map_fd = fd; >>> >>> - fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", >>> - sizeof(int), sizeof(int), max_queues, 0); >>> - if (fd < 0) { >>> - close(xsk->qidconf_map_fd); >>> - return fd; >>> - } >>> xsk->xsks_map_fd = fd; >>> >>> return 0; >>> @@ -385,10 +367,8 @@ static int xsk_create_bpf_maps(struct xsk_socket *xsk) >>> >>> static void xsk_delete_bpf_maps(struct xsk_socket *xsk) >>> { >>> - close(xsk->qidconf_map_fd); >>> + bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); >>> close(xsk->xsks_map_fd); >>> - xsk->qidconf_map_fd = -1; >>> - xsk->xsks_map_fd = -1; >>> } >>> >>> static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) >>> @@ -417,10 +397,9 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) >>> if (err) >>> goto out_map_ids; >>> >>> - for (i = 0; i < prog_info.nr_map_ids; i++) { >>> - if (xsk->qidconf_map_fd != -1 && xsk->xsks_map_fd != -1) >>> - break; >>> + xsk->xsks_map_fd = -1; >>> >>> + for (i = 0; i < prog_info.nr_map_ids; i++) { >>> fd = bpf_map_get_fd_by_id(map_ids[i]); >>> if (fd < 0) >>> continue; >>> @@ -431,11 +410,6 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) >>> continue; >>> } >>> >>> - if (!strcmp(map_info.name, "qidconf_map")) { >>> - xsk->qidconf_map_fd = fd; >>> - continue; >>> - } >>> - >>> if (!strcmp(map_info.name, "xsks_map")) { >>> xsk->xsks_map_fd = fd; >>> continue; >>> @@ -445,40 +419,18 @@ static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) >>> } >>> >>> err = 0; >>> - if (xsk->qidconf_map_fd < 0 || xsk->xsks_map_fd < 0) { >>> + if (xsk->xsks_map_fd == -1) >>> err = -ENOENT; >>> - xsk_delete_bpf_maps(xsk); >>> - } >>> >>> out_map_ids: >>> free(map_ids); >>> return err; >>> } >>> >>> -static void xsk_clear_bpf_maps(struct xsk_socket *xsk) >>> -{ >>> - int qid = false; >>> - >>> - bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); >>> - bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); >>> -} >>> - >>> static int xsk_set_bpf_maps(struct xsk_socket *xsk) >>> { >>> - int qid = true, fd = xsk->fd, err; >>> - >>> - err = bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); >>> - if (err) >>> - goto out; >>> - >>> - err = bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, &fd, 0); >>> - if (err) >>> - goto out; >>> - >>> - return 0; >>> -out: >>> - xsk_clear_bpf_maps(xsk); >>> - return err; >>> + return bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, >>> + &xsk->fd, 0); >>> } >>> >>> static int xsk_setup_xdp_prog(struct xsk_socket *xsk) >>> @@ -514,6 +466,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket *xsk) >>> >>> out_load: >>> close(xsk->prog_fd); >>> + xsk->prog_fd = -1; >> >> I found xsk->prog_fd confusing. Why do we need to set it here? > > I suppose this one isn't strictly required - I set it as a guard out of habit. > xsk is (currently) immediately freed by the caller, so it can be removed. > > > The main logic is: > > xsk->prog_fd = -1; > if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { > err = xsk_setup_xdp_prog(xsk); > > The user may pass INHIBIT_PROG_LOAD, which bypasses setting up the xdp program > (and any maps associated with the program), allowing installation of a custom > program. The cleanup behavior is then gated on prog_fd being -1, > >> >> I think we don't need to call xsk_delete_bpf_maps() in out_load path? > > Hmm, there's two out_load paths, but only one needs the delete maps call. Let > me redo the error handling so it's a bit more explicit. > > >> >>> out_maps: >>> xsk_delete_bpf_maps(xsk); >>> return err; >>> @@ -643,9 +596,7 @@ int xsk_socket__create(struct xsk_socket **xsk_ptr, const char *ifname, >>> goto out_mmap_tx; >>> } >>> >>> - xsk->qidconf_map_fd = -1; >>> - xsk->xsks_map_fd = -1; >>> - >>> + xsk->prog_fd = -1; >>> if (!(xsk->config.libbpf_flags & XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { >>> err = xsk_setup_xdp_prog(xsk); >>> if (err) >>> @@ -708,8 +659,10 @@ void xsk_socket__delete(struct xsk_socket *xsk) >>> if (!xsk) >>> return; >>> >>> - xsk_clear_bpf_maps(xsk); >>> - xsk_delete_bpf_maps(xsk); >>> + if (xsk->prog_fd != -1) { >>> + xsk_delete_bpf_maps(xsk); >>> + close(xsk->prog_fd); >> >> Here, we use prog_fd != -1 to gate xsk_delete_bpf_maps(), which is >> confusing. I looked at the code for quite sometime, but still cannot >> confirm it is correct. > > See above reasoning - with INHIBIT_PROG_LOAD, there is no library-provided > program or maps, so cleanup actions are skipped. > -- > Jonathan Thanks for the explanation. Acked-by: Song Liu <songliubraving@fb.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs. 2019-06-02 4:18 ` Jonathan Lemon 2019-06-02 6:55 ` Song Liu @ 2019-06-03 11:11 ` Björn Töpel 1 sibling, 0 replies; 8+ messages in thread From: Björn Töpel @ 2019-06-03 11:11 UTC (permalink / raw) To: Jonathan Lemon Cc: Song Liu, Networking, Kernel Team, Björn Töpel, Karlsson, Magnus On Sun, 2 Jun 2019 at 06:19, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > On 1 Jun 2019, at 16:05, Song Liu wrote: > > >> On May 31, 2019, at 11:57 AM, Jonathan Lemon > >> <jonathan.lemon@gmail.com> wrote: > >> > >> Use the recent change to XSKMAP bpf_map_lookup_elem() to test if > >> there is a xsk present in the map instead of duplicating the work > >> with qidconf. > >> > >> Fix things so callers using XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD > >> bypass any internal bpf maps, so xsk_socket__{create|delete} works > >> properly. > >> > >> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com> > >> --- > >> tools/lib/bpf/xsk.c | 79 > >> +++++++++------------------------------------ > >> 1 file changed, 16 insertions(+), 63 deletions(-) > >> > >> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > >> index 38667b62f1fe..7ce7494b5b50 100644 > >> --- a/tools/lib/bpf/xsk.c > >> +++ b/tools/lib/bpf/xsk.c > >> @@ -60,10 +60,8 @@ struct xsk_socket { > >> struct xsk_umem *umem; > >> struct xsk_socket_config config; > >> int fd; > >> - int xsks_map; > >> int ifindex; > >> int prog_fd; > >> - int qidconf_map_fd; > >> int xsks_map_fd; > >> __u32 queue_id; > >> char ifname[IFNAMSIZ]; > >> @@ -265,15 +263,11 @@ static int xsk_load_xdp_prog(struct xsk_socket > >> *xsk) > >> /* This is the C-program: > >> * SEC("xdp_sock") int xdp_sock_prog(struct xdp_md *ctx) > >> * { > >> - * int *qidconf, index = ctx->rx_queue_index; > >> + * int index = ctx->rx_queue_index; > >> * > >> * // A set entry here means that the correspnding queue_id > >> * // has an active AF_XDP socket bound to it. > >> - * qidconf = bpf_map_lookup_elem(&qidconf_map, &index); > >> - * if (!qidconf) > >> - * return XDP_ABORTED; > >> - * > >> - * if (*qidconf) > >> + * if (bpf_map_lookup_elem(&xsks_map, &index)) > >> * return bpf_redirect_map(&xsks_map, index, 0); > >> * > >> * return XDP_PASS; > >> @@ -286,15 +280,10 @@ static int xsk_load_xdp_prog(struct xsk_socket > >> *xsk) > >> BPF_STX_MEM(BPF_W, BPF_REG_10, BPF_REG_1, -4), > >> BPF_MOV64_REG(BPF_REG_2, BPF_REG_10), > >> BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -4), > >> - BPF_LD_MAP_FD(BPF_REG_1, xsk->qidconf_map_fd), > >> + BPF_LD_MAP_FD(BPF_REG_1, xsk->xsks_map_fd), > >> BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem), > >> BPF_MOV64_REG(BPF_REG_1, BPF_REG_0), > >> - BPF_MOV32_IMM(BPF_REG_0, 0), > >> - /* if r1 == 0 goto +8 */ > >> - BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 8), > >> BPF_MOV32_IMM(BPF_REG_0, 2), > >> - /* r1 = *(u32 *)(r1 + 0) */ > >> - BPF_LDX_MEM(BPF_W, BPF_REG_1, BPF_REG_1, 0), > >> /* if r1 == 0 goto +5 */ > >> BPF_JMP_IMM(BPF_JEQ, BPF_REG_1, 0, 5), > >> /* r2 = *(u32 *)(r10 - 4) */ > >> @@ -366,18 +355,11 @@ static int xsk_create_bpf_maps(struct > >> xsk_socket *xsk) > >> if (max_queues < 0) > >> return max_queues; > >> > >> - fd = bpf_create_map_name(BPF_MAP_TYPE_ARRAY, "qidconf_map", > >> + fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", > >> sizeof(int), sizeof(int), max_queues, 0); > >> if (fd < 0) > >> return fd; > >> - xsk->qidconf_map_fd = fd; > >> > >> - fd = bpf_create_map_name(BPF_MAP_TYPE_XSKMAP, "xsks_map", > >> - sizeof(int), sizeof(int), max_queues, 0); > >> - if (fd < 0) { > >> - close(xsk->qidconf_map_fd); > >> - return fd; > >> - } > >> xsk->xsks_map_fd = fd; > >> > >> return 0; > >> @@ -385,10 +367,8 @@ static int xsk_create_bpf_maps(struct xsk_socket > >> *xsk) > >> > >> static void xsk_delete_bpf_maps(struct xsk_socket *xsk) > >> { > >> - close(xsk->qidconf_map_fd); > >> + bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); > >> close(xsk->xsks_map_fd); > >> - xsk->qidconf_map_fd = -1; > >> - xsk->xsks_map_fd = -1; > >> } > >> > >> static int xsk_lookup_bpf_maps(struct xsk_socket *xsk) > >> @@ -417,10 +397,9 @@ static int xsk_lookup_bpf_maps(struct xsk_socket > >> *xsk) > >> if (err) > >> goto out_map_ids; > >> > >> - for (i = 0; i < prog_info.nr_map_ids; i++) { > >> - if (xsk->qidconf_map_fd != -1 && xsk->xsks_map_fd != -1) > >> - break; > >> + xsk->xsks_map_fd = -1; > >> > >> + for (i = 0; i < prog_info.nr_map_ids; i++) { > >> fd = bpf_map_get_fd_by_id(map_ids[i]); > >> if (fd < 0) > >> continue; > >> @@ -431,11 +410,6 @@ static int xsk_lookup_bpf_maps(struct xsk_socket > >> *xsk) > >> continue; > >> } > >> > >> - if (!strcmp(map_info.name, "qidconf_map")) { > >> - xsk->qidconf_map_fd = fd; > >> - continue; > >> - } > >> - > >> if (!strcmp(map_info.name, "xsks_map")) { > >> xsk->xsks_map_fd = fd; > >> continue; > >> @@ -445,40 +419,18 @@ static int xsk_lookup_bpf_maps(struct > >> xsk_socket *xsk) > >> } > >> > >> err = 0; > >> - if (xsk->qidconf_map_fd < 0 || xsk->xsks_map_fd < 0) { > >> + if (xsk->xsks_map_fd == -1) > >> err = -ENOENT; > >> - xsk_delete_bpf_maps(xsk); > >> - } > >> > >> out_map_ids: > >> free(map_ids); > >> return err; > >> } > >> > >> -static void xsk_clear_bpf_maps(struct xsk_socket *xsk) > >> -{ > >> - int qid = false; > >> - > >> - bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, &qid, 0); > >> - bpf_map_delete_elem(xsk->xsks_map_fd, &xsk->queue_id); > >> -} > >> - > >> static int xsk_set_bpf_maps(struct xsk_socket *xsk) > >> { > >> - int qid = true, fd = xsk->fd, err; > >> - > >> - err = bpf_map_update_elem(xsk->qidconf_map_fd, &xsk->queue_id, > >> &qid, 0); > >> - if (err) > >> - goto out; > >> - > >> - err = bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, &fd, > >> 0); > >> - if (err) > >> - goto out; > >> - > >> - return 0; > >> -out: > >> - xsk_clear_bpf_maps(xsk); > >> - return err; > >> + return bpf_map_update_elem(xsk->xsks_map_fd, &xsk->queue_id, > >> + &xsk->fd, 0); > >> } > >> > >> static int xsk_setup_xdp_prog(struct xsk_socket *xsk) > >> @@ -514,6 +466,7 @@ static int xsk_setup_xdp_prog(struct xsk_socket > >> *xsk) > >> > >> out_load: > >> close(xsk->prog_fd); > >> + xsk->prog_fd = -1; > > > > I found xsk->prog_fd confusing. Why do we need to set it here? > > I suppose this one isn't strictly required - I set it as a guard out of > habit. > xsk is (currently) immediately freed by the caller, so it can be > removed. > > > The main logic is: > > xsk->prog_fd = -1; > if (!(xsk->config.libbpf_flags & > XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { > err = xsk_setup_xdp_prog(xsk); > > The user may pass INHIBIT_PROG_LOAD, which bypasses setting up the xdp > program > (and any maps associated with the program), allowing installation of a > custom > program. The cleanup behavior is then gated on prog_fd being -1, > > > > > I think we don't need to call xsk_delete_bpf_maps() in out_load path? > > Hmm, there's two out_load paths, but only one needs the delete maps > call. Let > me redo the error handling so it's a bit more explicit. > You're going for a V4? I took this series for a spin: Tested-by: Björn Töpel <bjorn.topel@intel.com> Very nice cleanup, Jonathan! Björn > > > > >> out_maps: > >> xsk_delete_bpf_maps(xsk); > >> return err; > >> @@ -643,9 +596,7 @@ int xsk_socket__create(struct xsk_socket > >> **xsk_ptr, const char *ifname, > >> goto out_mmap_tx; > >> } > >> > >> - xsk->qidconf_map_fd = -1; > >> - xsk->xsks_map_fd = -1; > >> - > >> + xsk->prog_fd = -1; > >> if (!(xsk->config.libbpf_flags & > >> XSK_LIBBPF_FLAGS__INHIBIT_PROG_LOAD)) { > >> err = xsk_setup_xdp_prog(xsk); > >> if (err) > >> @@ -708,8 +659,10 @@ void xsk_socket__delete(struct xsk_socket *xsk) > >> if (!xsk) > >> return; > >> > >> - xsk_clear_bpf_maps(xsk); > >> - xsk_delete_bpf_maps(xsk); > >> + if (xsk->prog_fd != -1) { > >> + xsk_delete_bpf_maps(xsk); > >> + close(xsk->prog_fd); > > > > Here, we use prog_fd != -1 to gate xsk_delete_bpf_maps(), which is > > confusing. I looked at the code for quite sometime, but still cannot > > confirm it is correct. > > See above reasoning - with INHIBIT_PROG_LOAD, there is no > library-provided > program or maps, so cleanup actions are skipped. > -- > Jonathan ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-06-03 11:11 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-05-31 18:57 [PATCH v3 bpf-next 0/2] Better handling of xskmap entries Jonathan Lemon 2019-05-31 18:57 ` [PATCH v3 bpf-next 1/2] bpf: Allow bpf_map_lookup_elem() on an xskmap Jonathan Lemon 2019-06-01 23:06 ` Song Liu 2019-05-31 18:57 ` [PATCH v3 bpf-next 2/2] libbpf: remove qidconf and better support external bpf programs Jonathan Lemon 2019-06-01 23:05 ` Song Liu 2019-06-02 4:18 ` Jonathan Lemon 2019-06-02 6:55 ` Song Liu 2019-06-03 11:11 ` Björn Töpel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.