[PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps

[PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps

[Takshak Chahande]

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>
---
 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

[PATCH v3 bpf-next 2/2] selftests/bpf: handle batch operations for map-in-map bpf-maps

[Takshak Chahande]

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

Re: [PATCH v3 bpf-next 2/2] selftests/bpf: handle batch operations for map-in-map bpf-maps

[Andrii Nakryiko]

On Mon, Apr 25, 2022 at 11:42 AM Takshak Chahande <ctakshak@fb.com> wrote:
>
> 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>
> ---

Is there any extra benefit in putting these test under test_maps
instead of test_progs? test_progs has better "testing infra", it's
easier to isolate and debug tests, skip them or run just the ones you
want, better logging, better ASSERT_xxx() macros for testing, etc.

I see that you create a fixed amount of inner maps, etc. It's all
actually simpler to do in test_progs using BPF-side code. See other
examples under progs/ that show how to create and initialize
map-in-maps.


>  .../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
>

[...]

> +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);

LIBBPF_OPTS() is declaring a variable, it should go together with
other variables

> +       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 _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,

nit: prefere shorter LIBBPF_OPTS(). as for zero initialization of
elem_flags and flags, they are zero-initialized by default by
LIBBPF_OPTS, so you can just drop two lines below

> +                           .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);
> +

[...]

Re: [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps

[Andrii Nakryiko]

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
>

Re: [PATCH v3 bpf-next 1/2] bpf: Extend batch operations for map-in-map bpf-maps

[Yonghong Song]

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,
>   };

Re: [PATCH v3 bpf-next 2/2] selftests/bpf: handle batch operations for map-in-map bpf-maps

[Yonghong Song]

On 4/25/22 11:41 AM, Takshak Chahande wrote:
> 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)

Please put the above version changes between
    create mode 100644 
tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c
and
    diff --git 
a/tools/testing/selftests/bpf/map_tests/map_in_map_batch_ops.c ...

So they won't appear in the commit message when the patch
is merged. The same for patch #1.

> 
> Signed-off-by: Takshak Chahande <ctakshak@fb.com>

Ack with a few nits below.

Acked-by: Yonghong Song <yhs@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;
> +

Remove the empty line in the above.

> +	LIBBPF_OPTS(bpf_map_create_opts, attr);

LIBBPF_OPTS is a declaration. So put an empty line here.

> +	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);

Just for completeness, should we check whether either of
fetched_keys or fetched_values is NULL or not?

> +
> +	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);

indentation issue?

> +
> +	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,
> +	);

Do we need this opts, why not just NULL pointer for opts?

> +
> +	outer_map_keys = calloc(max_entries, value_size);
> +	inner_map_fds = calloc(max_entries, value_size);

check whether outer_map_keys or inner_map_fds is NULL or not?

> +	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);
> +}
[...]