* [PATCH v3 bpf-next 2/2] selftests/bpf: handle batch operations for map-in-map bpf-maps
2022-04-25 18:41 [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps Takshak Chahande
@ 2022-04-25 18:41 ` Takshak Chahande
2022-04-27 20:14 ` Andrii Nakryiko
2022-04-28 20:59 ` Yonghong Song
2022-04-27 20:15 ` [PATCH v3 bpf-next 1/2] bpf: Extend " Andrii Nakryiko
2022-04-28 20:21 ` Yonghong Song
2 siblings, 2 replies; 6+ messages in thread
From: Takshak Chahande @ 2022-04-25 18:41 UTC (permalink / raw)
To: netdev, bpf; +Cc: andrii, ast, ctakshak, ndixit, kafai, andriin, daniel
This patch adds up test cases that handles 4 combinations:
a) outer map: BPF_MAP_TYPE_ARRAY_OF_MAPS
inner maps: BPF_MAP_TYPE_ARRAY and BPF_MAP_TYPE_HASH
b) outer map: BPF_MAP_TYPE_HASH_OF_MAPS
inner maps: BPF_MAP_TYPE_ARRAY and BPF_MAP_TYPE_HASH
v2->v3:
- Handled transient ENOSPC correctly, bug was found in BPF CI (Daniel)
v1->v2:
- Fixed no format arguments error (Andrii)
Signed-off-by: Takshak Chahande <ctakshak@fb.com>
---
.../bpf/map_tests/map_in_map_batch_ops.c | 239 ++++++++++++++++++
1 file changed, 239 insertions(+)
create mode 100644 tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c
diff --git a/tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c b/tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c
new file mode 100644
index 000000000000..f1eee580ba2e
--- /dev/null
+++ b/tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <stdio.h>
+#include <errno.h>
+#include <string.h>
+#include <unistd.h>
+
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include <test_maps.h>
+
+#define OUTER_MAP_ENTRIES 10
+
+static __u32 get_map_id_from_fd(int map_fd)
+{
+ struct bpf_map_info map_info = {};
+ uint32_t info_len = sizeof(map_info);
+ int ret;
+
+ ret = bpf_obj_get_info_by_fd(map_fd, &map_info, &info_len);
+ CHECK(ret < 0, "Finding map info failed", "error:%s\n",
+ strerror(errno));
+
+ return map_info.id;
+}
+
+/* This creates number of OUTER_MAP_ENTRIES maps that will be stored
+ * in outer map and return the created map_fds
+ */
+static void create_inner_maps(enum bpf_map_type map_type,
+ __u32 *inner_map_fds)
+{
+ int map_fd, map_index, ret;
+ __u32 map_key = 0, map_id;
+ char map_name[15];
+
+ for (map_index = 0; map_index < OUTER_MAP_ENTRIES; map_index++) {
+ memset(map_name, 0, sizeof(map_name));
+ sprintf(map_name, "inner_map_fd_%d", map_index);
+ map_fd = bpf_map_create(map_type, map_name, sizeof(__u32),
+ sizeof(__u32), 1, NULL);
+ CHECK(map_fd < 0,
+ "inner bpf_map_create() failed",
+ "map_type=(%d) map_name(%s), error:%s\n",
+ map_type, map_name, strerror(errno));
+
+ /* keep track of the inner map fd as it is required
+ * to add records in outer map
+ */
+ inner_map_fds[map_index] = map_fd;
+
+ /* Add entry into this created map
+ * eg: map1 key = 0, value = map1's map id
+ * map2 key = 0, value = map2's map id
+ */
+ map_id = get_map_id_from_fd(map_fd);
+ ret = bpf_map_update_elem(map_fd, &map_key, &map_id, 0);
+ CHECK(ret != 0,
+ "bpf_map_update_elem failed",
+ "map_type=(%d) map_name(%s), error:%s\n",
+ map_type, map_name, strerror(errno));
+ }
+}
+
+static int create_outer_map(enum bpf_map_type map_type, __u32 inner_map_fd)
+{
+ int outer_map_fd;
+
+ LIBBPF_OPTS(bpf_map_create_opts, attr);
+ attr.inner_map_fd = inner_map_fd;
+ outer_map_fd = bpf_map_create(map_type, "outer_map", sizeof(__u32),
+ sizeof(__u32), OUTER_MAP_ENTRIES,
+ &attr);
+ CHECK(outer_map_fd < 0,
+ "outer bpf_map_create()",
+ "map_type=(%d), error:%s\n",
+ map_type, strerror(errno));
+
+ return outer_map_fd;
+}
+
+static void validate_fetch_results(int outer_map_fd, __u32 *inner_map_fds,
+ __u32 *fetched_keys, __u32 *fetched_values,
+ __u32 max_entries_fetched)
+{
+ __u32 inner_map_key, inner_map_value;
+ int inner_map_fd, entry, err;
+ __u32 outer_map_value;
+
+ for (entry = 0; entry < max_entries_fetched; ++entry) {
+ outer_map_value = fetched_values[entry];
+ inner_map_fd = bpf_map_get_fd_by_id(outer_map_value);
+ CHECK(inner_map_fd < 0,
+ "Failed to get inner map fd",
+ "from id(%d), error=%s\n",
+ outer_map_value, strerror(errno));
+ err = bpf_map_get_next_key(inner_map_fd, NULL, &inner_map_key);
+ CHECK(err != 0,
+ "Failed to get inner map key",
+ "error=%s\n", strerror(errno));
+
+ err = bpf_map_lookup_elem(inner_map_fd, &inner_map_key,
+ &inner_map_value);
+ CHECK(err != 0,
+ "Failed to get inner map value",
+ "for key(%d), error=%s\n",
+ inner_map_key, strerror(errno));
+
+ /* Actual value validation */
+ CHECK(outer_map_value != inner_map_value,
+ "Failed to validate inner map value",
+ "fetched(%d) and lookedup(%d)!\n",
+ outer_map_value, inner_map_value);
+ }
+}
+
+static void fetch_and_validate(int outer_map_fd,
+ __u32 *inner_map_fds,
+ struct bpf_map_batch_opts *opts,
+ __u32 batch_size, bool delete_entries)
+{
+ __u32 *fetched_keys, *fetched_values, total_fetched = 0;
+ __u32 batch_key = 0, fetch_count, step_size;
+ int err, max_entries = OUTER_MAP_ENTRIES;
+ __u32 value_size = sizeof(__u32);
+
+ /* Total entries needs to be fetched */
+ fetched_keys = calloc(max_entries, value_size);
+ fetched_values = calloc(max_entries, value_size);
+
+ for (step_size = batch_size; step_size <= max_entries; step_size += batch_size) {
+ fetch_count = step_size;
+ err = delete_entries
+ ? bpf_map_lookup_and_delete_batch(outer_map_fd,
+ total_fetched ? &batch_key : NULL,
+ &batch_key,
+ fetched_keys + total_fetched,
+ fetched_values + total_fetched,
+ &fetch_count, opts)
+ : bpf_map_lookup_batch(outer_map_fd,
+ total_fetched ? &batch_key : NULL,
+ &batch_key,
+ fetched_keys + total_fetched,
+ fetched_values + total_fetched,
+ &fetch_count, opts);
+
+ if (err && errno == ENOSPC) {
+ /* Fetch again with higher batch size */
+ total_fetched = 0;
+ continue;
+ }
+
+ CHECK((err < 0 && (errno != ENOENT)),
+ "lookup with steps failed",
+ "error: %s\n", strerror(errno));
+
+ /* Update the total fetched number */
+ total_fetched += fetch_count;
+ if (err)
+ break;
+ }
+
+ CHECK((total_fetched != max_entries),
+ "Unable to fetch expected entries !",
+ "total_fetched(%d) and max_entries(%d) error: (%d):%s\n",
+ total_fetched, max_entries, errno, strerror(errno));
+
+ /* validate the fetched entries */
+ validate_fetch_results(outer_map_fd, inner_map_fds, fetched_keys,
+ fetched_values, total_fetched);
+ printf("batch_op(%s) is successful with batch_size(%d)\n",
+ delete_entries ? "LOOKUP_AND_DELETE" : "LOOKUP", batch_size);
+
+ free(fetched_keys);
+ free(fetched_values);
+}
+
+static void _map_in_map_batch_ops(enum bpf_map_type outer_map_type,
+ enum bpf_map_type inner_map_type)
+{
+ __u32 *outer_map_keys, *inner_map_fds;
+ __u32 max_entries = OUTER_MAP_ENTRIES;
+ __u32 value_size = sizeof(__u32);
+ int batch_size[2] = {5, 10};
+ __u32 map_index, op_index;
+ int outer_map_fd, ret;
+ DECLARE_LIBBPF_OPTS(bpf_map_batch_opts, opts,
+ .elem_flags = 0,
+ .flags = 0,
+ );
+
+ outer_map_keys = calloc(max_entries, value_size);
+ inner_map_fds = calloc(max_entries, value_size);
+ create_inner_maps(inner_map_type, inner_map_fds);
+
+ outer_map_fd = create_outer_map(outer_map_type, *inner_map_fds);
+ /* create outer map keys */
+ for (map_index = 0; map_index < max_entries; map_index++)
+ outer_map_keys[map_index] =
+ ((outer_map_type == BPF_MAP_TYPE_ARRAY_OF_MAPS)
+ ? 9 : 1000) - map_index;
+
+ /* batch operation - map_update */
+ ret = bpf_map_update_batch(outer_map_fd, outer_map_keys,
+ inner_map_fds, &max_entries, &opts);
+ CHECK(ret != 0,
+ "Failed to update the outer map batch ops",
+ "error=%s\n", strerror(errno));
+
+ /* batch operation - map_lookup */
+ for (op_index = 0; op_index < 2; ++op_index)
+ fetch_and_validate(outer_map_fd, inner_map_fds, &opts,
+ batch_size[op_index], false);
+
+ /* batch operation - map_lookup_delete */
+ if (outer_map_type == BPF_MAP_TYPE_HASH_OF_MAPS)
+ fetch_and_validate(outer_map_fd, inner_map_fds, &opts,
+ max_entries, true /*delete*/);
+
+ free(inner_map_fds);
+ free(outer_map_keys);
+}
+
+void test_map_in_map_batch_ops_array(void)
+{
+ _map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_ARRAY);
+ printf("%s:PASS with inner ARRAY map\n", __func__);
+ _map_in_map_batch_ops(BPF_MAP_TYPE_ARRAY_OF_MAPS, BPF_MAP_TYPE_HASH);
+ printf("%s:PASS with inner HASH map\n", __func__);
+}
+
+void test_map_in_map_batch_ops_hash(void)
+{
+ _map_in_map_batch_ops(BPF_MAP_TYPE_HASH_OF_MAPS, BPF_MAP_TYPE_ARRAY);
+ printf("%s:PASS with inner ARRAY map\n", __func__);
+ _map_in_map_batch_ops(BPF_MAP_TYPE_HASH_OF_MAPS, BPF_MAP_TYPE_HASH);
+ printf("%s:PASS with inner HASH map\n", __func__);
+}
--
2.30.2
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps
2022-04-25 18:41 [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps Takshak Chahande
2022-04-25 18:41 ` [PATCH v3 bpf-next 2/2] selftests/bpf: handle " Takshak Chahande
@ 2022-04-27 20:15 ` Andrii Nakryiko
2022-04-28 20:21 ` Yonghong Song
2 siblings, 0 replies; 6+ messages in thread
From: Andrii Nakryiko @ 2022-04-27 20:15 UTC (permalink / raw)
To: Takshak Chahande, Yonghong Song
Cc: Networking, bpf, Andrii Nakryiko, Alexei Starovoitov, ndixit,
Martin Lau, Andrii Nakryiko, Daniel Borkmann
On Mon, Apr 25, 2022 at 11:42 AM Takshak Chahande <ctakshak@fb.com> wrote:
>
> This patch extends batch operations support for map-in-map map-types:
> BPF_MAP_TYPE_HASH_OF_MAPS and BPF_MAP_TYPE_ARRAY_OF_MAPS
>
> A usecase where outer HASH map holds hundred of VIP entries and its
> associated reuse-ports per VIP stored in REUSEPORT_SOCKARRAY type
> inner map, needs to do batch operation for performance gain.
>
> This patch leverages the exiting generic functions for most of the batch
> operations. As map-in-map's value contains the actual reference of the inner map,
> for BPF_MAP_TYPE_HASH_OF_MAPS type, it needed an extra step to fetch the
> map_id from the reference value.
>
> selftests are added in next patch that has v1->v3 changes
>
> Signed-off-by: Takshak Chahande <ctakshak@fb.com>
> ---
cc'ing Yonghong who was involved in designing and implementing these
batch APIs. PTAL when you get a chance, thanks!
> kernel/bpf/arraymap.c | 2 ++
> kernel/bpf/hashtab.c | 12 ++++++++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 7f145aefbff8..f0852b6617cc 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -1344,6 +1344,8 @@ const struct bpf_map_ops array_of_maps_map_ops = {
> .map_fd_put_ptr = bpf_map_fd_put_ptr,
> .map_fd_sys_lookup_elem = bpf_map_fd_sys_lookup_elem,
> .map_gen_lookup = array_of_map_gen_lookup,
> + .map_lookup_batch = generic_map_lookup_batch,
> + .map_update_batch = generic_map_update_batch,
> .map_check_btf = map_check_no_btf,
> .map_btf_name = "bpf_array",
> .map_btf_id = &array_of_maps_map_btf_id,
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index c68fbebc8c00..fd537bfba84c 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -139,7 +139,7 @@ static inline bool htab_use_raw_lock(const struct bpf_htab *htab)
>
> static void htab_init_buckets(struct bpf_htab *htab)
> {
> - unsigned i;
> + unsigned int i;
>
> for (i = 0; i < htab->n_buckets; i++) {
> INIT_HLIST_NULLS_HEAD(&htab->buckets[i].head, i);
> @@ -1594,7 +1594,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> void __user *uvalues = u64_to_user_ptr(attr->batch.values);
> void __user *ukeys = u64_to_user_ptr(attr->batch.keys);
> void __user *ubatch = u64_to_user_ptr(attr->batch.in_batch);
> - u32 batch, max_count, size, bucket_size;
> + u32 batch, max_count, size, bucket_size, map_id;
> struct htab_elem *node_to_free = NULL;
> u64 elem_map_flags, map_flags;
> struct hlist_nulls_head *head;
> @@ -1719,6 +1719,13 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> }
> } else {
> value = l->key + roundup_key_size;
> + if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS) {
> + struct bpf_map **inner_map = value;
> + /* Actual value is the id of the inner map */
> + map_id = map->ops->map_fd_sys_lookup_elem(*inner_map);
> + value = &map_id;
> + }
> +
> if (elem_map_flags & BPF_F_LOCK)
> copy_map_value_locked(map, dst_val, value,
> true);
> @@ -2425,6 +2432,7 @@ const struct bpf_map_ops htab_of_maps_map_ops = {
> .map_fd_sys_lookup_elem = bpf_map_fd_sys_lookup_elem,
> .map_gen_lookup = htab_of_map_gen_lookup,
> .map_check_btf = map_check_no_btf,
> + BATCH_OPS(htab),
> .map_btf_name = "bpf_htab",
> .map_btf_id = &htab_of_maps_map_btf_id,
> };
> --
> 2.30.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps
2022-04-25 18:41 [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps Takshak Chahande
2022-04-25 18:41 ` [PATCH v3 bpf-next 2/2] selftests/bpf: handle " Takshak Chahande
2022-04-27 20:15 ` [PATCH v3 bpf-next 1/2] bpf: Extend " Andrii Nakryiko
@ 2022-04-28 20:21 ` Yonghong Song
2 siblings, 0 replies; 6+ messages in thread
From: Yonghong Song @ 2022-04-28 20:21 UTC (permalink / raw)
To: Takshak Chahande, netdev, bpf; +Cc: andrii, ast, ndixit, kafai, andriin, daniel
On 4/25/22 11:41 AM, Takshak Chahande wrote:
> This patch extends batch operations support for map-in-map map-types:
> BPF_MAP_TYPE_HASH_OF_MAPS and BPF_MAP_TYPE_ARRAY_OF_MAPS
>
> A usecase where outer HASH map holds hundred of VIP entries and its
> associated reuse-ports per VIP stored in REUSEPORT_SOCKARRAY type
> inner map, needs to do batch operation for performance gain.
>
> This patch leverages the exiting generic functions for most of the batch
> operations. As map-in-map's value contains the actual reference of the inner map,
> for BPF_MAP_TYPE_HASH_OF_MAPS type, it needed an extra step to fetch the
> map_id from the reference value.
>
> selftests are added in next patch that has v1->v3 changes
>
> Signed-off-by: Takshak Chahande <ctakshak@fb.com>
Ack with a minor issue below.
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> kernel/bpf/arraymap.c | 2 ++
> kernel/bpf/hashtab.c | 12 ++++++++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
> index 7f145aefbff8..f0852b6617cc 100644
> --- a/kernel/bpf/arraymap.c
> +++ b/kernel/bpf/arraymap.c
> @@ -1344,6 +1344,8 @@ const struct bpf_map_ops array_of_maps_map_ops = {
> .map_fd_put_ptr = bpf_map_fd_put_ptr,
> .map_fd_sys_lookup_elem = bpf_map_fd_sys_lookup_elem,
> .map_gen_lookup = array_of_map_gen_lookup,
> + .map_lookup_batch = generic_map_lookup_batch,
> + .map_update_batch = generic_map_update_batch,
> .map_check_btf = map_check_no_btf,
> .map_btf_name = "bpf_array",
> .map_btf_id = &array_of_maps_map_btf_id,
> diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> index c68fbebc8c00..fd537bfba84c 100644
> --- a/kernel/bpf/hashtab.c
> +++ b/kernel/bpf/hashtab.c
> @@ -139,7 +139,7 @@ static inline bool htab_use_raw_lock(const struct bpf_htab *htab)
>
> static void htab_init_buckets(struct bpf_htab *htab)
> {
> - unsigned i;
> + unsigned int i;
>
> for (i = 0; i < htab->n_buckets; i++) {
> INIT_HLIST_NULLS_HEAD(&htab->buckets[i].head, i);
> @@ -1594,7 +1594,7 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> void __user *uvalues = u64_to_user_ptr(attr->batch.values);
> void __user *ukeys = u64_to_user_ptr(attr->batch.keys);
> void __user *ubatch = u64_to_user_ptr(attr->batch.in_batch);
> - u32 batch, max_count, size, bucket_size;
> + u32 batch, max_count, size, bucket_size, map_id;
> struct htab_elem *node_to_free = NULL;
> u64 elem_map_flags, map_flags;
> struct hlist_nulls_head *head;
> @@ -1719,6 +1719,13 @@ __htab_map_lookup_and_delete_batch(struct bpf_map *map,
> }
> } else {
> value = l->key + roundup_key_size;
> + if (map->map_type == BPF_MAP_TYPE_HASH_OF_MAPS) {
> + struct bpf_map **inner_map = value;
Please leave a blank line here between var declaration and actual codes.
> + /* Actual value is the id of the inner map */
> + map_id = map->ops->map_fd_sys_lookup_elem(*inner_map);
> + value = &map_id;
> + }
> +
> if (elem_map_flags & BPF_F_LOCK)
> copy_map_value_locked(map, dst_val, value,
> true);
> @@ -2425,6 +2432,7 @@ const struct bpf_map_ops htab_of_maps_map_ops = {
> .map_fd_sys_lookup_elem = bpf_map_fd_sys_lookup_elem,
> .map_gen_lookup = htab_of_map_gen_lookup,
> .map_check_btf = map_check_no_btf,
> + BATCH_OPS(htab),
> .map_btf_name = "bpf_htab",
> .map_btf_id = &htab_of_maps_map_btf_id,
> };
^ permalink raw reply [flat|nested] 6+ messages in thread