> On Jun 1, 2019, at 9:18 PM, Jonathan Lemon <jonathan.le...@gmail.com> wrote:
>
>
>
> On 1 Jun 2019, at 16:05, Song Liu wrote:
>
>>> On May 31, 2019, at 11:57 AM, Jonathan Lemon <jonathan.le...@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.le...@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 <songliubrav...@fb.com>
