Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20191107212023.171208-1-brianvv@google.com> <20191107212023.171208-3-brianvv@google.com>
In-Reply-To: <20191107212023.171208-3-brianvv@google.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Fri, 15 Nov 2019 14:42:24 -0800
Message-ID: <CAEf4Bzbn6J79+_wK2+JtQaGRiiM9hCGyOsuE9DeSqa9ngemTWw@mail.gmail.com>
Subject: Re: [RFC bpf-next 2/3] tools/bpf: test bpf_map_lookup_and_delete_batch()
To:     Brian Vazquez <brianvv@google.com>
Cc:     Brian Vazquez <brianvv.kernel@gmail.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        "David S . Miller" <davem@davemloft.net>,
        Stanislav Fomichev <sdf@google.com>,
        open list <linux-kernel@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        Yonghong Song <yhs@fb.com>, Petar Penkov <ppenkov@google.com>,
        Willem de Bruijn <willemb@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Nov 7, 2019 at 1:23 PM Brian Vazquez <brianvv@google.com> wrote:
>
> From: Yonghong Song <yhs@fb.com>
>
> Added four libbpf API functions to support map batch operations:
>   . int bpf_map_delete_batch( ... )
>   . int bpf_map_lookup_batch( ... )
>   . int bpf_map_lookup_and_delete_batch( ... )
>   . int bpf_map_update_batch( ... )
>
> Tested bpf_map_lookup_and_delete_batch() and bpf_map_update_batch()
> functionality.
>   $ ./test_maps
>   ...
>   test_map_lookup_and_delete_batch:PASS
>   ...
>
> Note that I clumped uapi header sync patch, libbpf patch
> and tests patch together considering this is a RFC patch.
> Will do proper formating once it is out of RFC stage.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---

[...]

>
> +       struct { /* struct used by BPF_MAP_*_BATCH commands */
> +               __u64           batch;  /* input/output:
> +                                        * input: start batch,
> +                                        *        0 to start from beginning.
> +                                        * output: next start batch,
> +                                        *         0 to end batching.
> +                                        */
> +               __aligned_u64   keys;
> +               __aligned_u64   values;
> +               __u32           count;  /* input/output:
> +                                        * input: # of elements keys/values.
> +                                        * output: # of filled elements.
> +                                        */
> +               __u32           map_fd;
> +               __u64           elem_flags;
> +               __u64           flags;
> +       } batch;
> +

Describe what elem_flags and flags are here?

[...]

> +LIBBPF_API int bpf_map_delete_batch(int fd, __u64 *batch, __u32 *count,
> +                                   __u64 elem_flags, __u64 flags);
> +LIBBPF_API int bpf_map_lookup_batch(int fd, __u64 *batch, void *keys,
> +                                   void *values, __u32 *count,
> +                                   __u64 elem_flags, __u64 flags);
> +LIBBPF_API int bpf_map_lookup_and_delete_batch(int fd, __u64 *batch,
> +                                              void *keys, void *values,
> +                                              __u32 *count, __u64 elem_flags,
> +                                              __u64 flags);
> +LIBBPF_API int bpf_map_update_batch(int fd, void *keys, void *values,
> +                                   __u32 *count, __u64 elem_flags,
> +                                   __u64 flags);

Should we start using the same approach as with bpf_object__open_file
(see LIBBPF_OPTS), so that we can keep adding extra fields without
breaking ABI? The gist is: use function arguments for mandatory fields
(as of right now, at least), and put all the optional fields into a
xxx_opts struct, which can be NULL. Please see
bpf_object__open_{file,mem} for details.

> +
>  LIBBPF_API int bpf_obj_pin(int fd, const char *pathname);
>  LIBBPF_API int bpf_obj_get(const char *pathname);
>  LIBBPF_API int bpf_prog_attach(int prog_fd, int attachable_fd,
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 86173cbb159d3..0529a770a04eb 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -189,6 +189,10 @@ LIBBPF_0.0.4 {
>  LIBBPF_0.0.5 {
>         global:
>                 bpf_btf_get_next_id;
> +               bpf_map_delete_batch;
> +               bpf_map_lookup_and_delete_batch;
> +               bpf_map_lookup_batch;
> +               bpf_map_update_batch;
>  } LIBBPF_0.0.4;

This should be in 0.0.6 section now.

>

[...]