[PATCH bpf-next 0/3] "map_extra" and bloom filter fixups

[PATCH bpf-next 0/3] "map_extra" and bloom filter fixups

[Joanne Koong]

There are 3 patches in this patchset:

1/3 - Bloom filter naming fixups (kernel/bpf/bloom_filter.c)

2/3 - Add alignment padding for map_extra, rearrange fields in
bpf_map struct to consolidate holes

3/3 - Bloom filter tests (prog_tests/bloom_filter_map):
Add test for successful userspace calls, some refactoring to
use bpf_create_map instead of bpf_create_map_xattr

Joanne Koong (3):
  bpf: Bloom filter map naming fixups
  bpf: Add alignment padding for "map_extra" + consolidate holes
  selftests/bpf: Add bloom map success test for userspace calls

 include/linux/bpf.h                           |  6 +--
 include/uapi/linux/bpf.h                      |  1 +
 kernel/bpf/bloom_filter.c                     | 49 +++++++++--------
 tools/include/uapi/linux/bpf.h                |  1 +
 .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
 5 files changed, 64 insertions(+), 46 deletions(-)

-- 
2.30.2

[PATCH bpf-next 1/3] bpf: Bloom filter map naming fixups

[Joanne Koong]

This patch has two changes in the kernel bloom filter map
implementation:

1) Change the names of map-ops functions to include the
"bloom_map" prefix.

As Martin pointed out on a previous patchset, having generic
map-ops names may be confusing in tracing and in perf-report.

2) Drop the "& 0xF" when getting nr_hash_funcs, since we
already ascertain that no other bits in map_extra beyond the
first 4 bits can be set.

Signed-off-by: Joanne Koong <joannekoong@fb.com>
---
 kernel/bpf/bloom_filter.c | 49 +++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/kernel/bpf/bloom_filter.c b/kernel/bpf/bloom_filter.c
index 7c50232b7571..073c2f2cab8b 100644
--- a/kernel/bpf/bloom_filter.c
+++ b/kernel/bpf/bloom_filter.c
@@ -40,7 +40,7 @@ static u32 hash(struct bpf_bloom_filter *bloom, void *value,
 	return h & bloom->bitset_mask;
 }
 
-static int peek_elem(struct bpf_map *map, void *value)
+static int bloom_map_peek_elem(struct bpf_map *map, void *value)
 {
 	struct bpf_bloom_filter *bloom =
 		container_of(map, struct bpf_bloom_filter, map);
@@ -55,7 +55,7 @@ static int peek_elem(struct bpf_map *map, void *value)
 	return 0;
 }
 
-static int push_elem(struct bpf_map *map, void *value, u64 flags)
+static int bloom_map_push_elem(struct bpf_map *map, void *value, u64 flags)
 {
 	struct bpf_bloom_filter *bloom =
 		container_of(map, struct bpf_bloom_filter, map);
@@ -72,12 +72,12 @@ static int push_elem(struct bpf_map *map, void *value, u64 flags)
 	return 0;
 }
 
-static int pop_elem(struct bpf_map *map, void *value)
+static int bloom_map_pop_elem(struct bpf_map *map, void *value)
 {
 	return -EOPNOTSUPP;
 }
 
-static struct bpf_map *map_alloc(union bpf_attr *attr)
+static struct bpf_map *bloom_map_alloc(union bpf_attr *attr)
 {
 	u32 bitset_bytes, bitset_mask, nr_hash_funcs, nr_bits;
 	int numa_node = bpf_map_attr_numa_node(attr);
@@ -90,11 +90,13 @@ static struct bpf_map *map_alloc(union bpf_attr *attr)
 	    attr->max_entries == 0 ||
 	    attr->map_flags & ~BLOOM_CREATE_FLAG_MASK ||
 	    !bpf_map_flags_access_ok(attr->map_flags) ||
+	    /* The lower 4 bits of map_extra (0xF) specify the number
+	     * of hash functions
+	     */
 	    (attr->map_extra & ~0xF))
 		return ERR_PTR(-EINVAL);
 
-	/* The lower 4 bits of map_extra specify the number of hash functions */
-	nr_hash_funcs = attr->map_extra & 0xF;
+	nr_hash_funcs = attr->map_extra;
 	if (nr_hash_funcs == 0)
 		/* Default to using 5 hash functions if unspecified */
 		nr_hash_funcs = 5;
@@ -150,7 +152,7 @@ static struct bpf_map *map_alloc(union bpf_attr *attr)
 	return &bloom->map;
 }
 
-static void map_free(struct bpf_map *map)
+static void bloom_map_free(struct bpf_map *map)
 {
 	struct bpf_bloom_filter *bloom =
 		container_of(map, struct bpf_bloom_filter, map);
@@ -158,38 +160,39 @@ static void map_free(struct bpf_map *map)
 	bpf_map_area_free(bloom);
 }
 
-static void *lookup_elem(struct bpf_map *map, void *key)
+static void *bloom_map_lookup_elem(struct bpf_map *map, void *key)
 {
 	/* The eBPF program should use map_peek_elem instead */
 	return ERR_PTR(-EINVAL);
 }
 
-static int update_elem(struct bpf_map *map, void *key,
-		       void *value, u64 flags)
+static int bloom_map_update_elem(struct bpf_map *map, void *key,
+				 void *value, u64 flags)
 {
 	/* The eBPF program should use map_push_elem instead */
 	return -EINVAL;
 }
 
-static int check_btf(const struct bpf_map *map, const struct btf *btf,
-		     const struct btf_type *key_type,
-		     const struct btf_type *value_type)
+static int bloom_map_check_btf(const struct bpf_map *map,
+			       const struct btf *btf,
+			       const struct btf_type *key_type,
+			       const struct btf_type *value_type)
 {
 	/* Bloom filter maps are keyless */
 	return btf_type_is_void(key_type) ? 0 : -EINVAL;
 }
 
-static int bpf_bloom_btf_id;
+static int bpf_bloom_map_btf_id;
 const struct bpf_map_ops bloom_filter_map_ops = {
 	.map_meta_equal = bpf_map_meta_equal,
-	.map_alloc = map_alloc,
-	.map_free = map_free,
-	.map_push_elem = push_elem,
-	.map_peek_elem = peek_elem,
-	.map_pop_elem = pop_elem,
-	.map_lookup_elem = lookup_elem,
-	.map_update_elem = update_elem,
-	.map_check_btf = check_btf,
+	.map_alloc = bloom_map_alloc,
+	.map_free = bloom_map_free,
+	.map_push_elem = bloom_map_push_elem,
+	.map_peek_elem = bloom_map_peek_elem,
+	.map_pop_elem = bloom_map_pop_elem,
+	.map_lookup_elem = bloom_map_lookup_elem,
+	.map_update_elem = bloom_map_update_elem,
+	.map_check_btf = bloom_map_check_btf,
 	.map_btf_name = "bpf_bloom_filter",
-	.map_btf_id = &bpf_bloom_btf_id,
+	.map_btf_id = &bpf_bloom_map_btf_id,
 };
-- 
2.30.2

Re: [PATCH bpf-next 1/3] bpf: Bloom filter map naming fixups

[Yonghong Song]

On 10/29/21 10:01 AM, Joanne Koong wrote:
> This patch has two changes in the kernel bloom filter map
> implementation:
> 
> 1) Change the names of map-ops functions to include the
> "bloom_map" prefix.
> 
> As Martin pointed out on a previous patchset, having generic
> map-ops names may be confusing in tracing and in perf-report.
> 
> 2) Drop the "& 0xF" when getting nr_hash_funcs, since we
> already ascertain that no other bits in map_extra beyond the
> first 4 bits can be set.
> 
> Signed-off-by: Joanne Koong <joannekoong@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

[PATCH bpf-next 2/3] bpf: Add alignment padding for "map_extra" + consolidate holes

[Joanne Koong]

This patch makes 2 changes regarding alignment padding
for the "map_extra" field.

1) In the kernel header, "map_extra" and "btf_value_type_id"
are rearranged to consolidate the hole.

Before:
struct bpf_map {
	...
        u32		max_entries;	/*    36     4	*/
        u32		map_flags;	/*    40     4	*/

        /* XXX 4 bytes hole, try to pack */

        u64		map_extra;	/*    48     8	*/
        int		spin_lock_off;	/*    56     4	*/
        int		timer_off;	/*    60     4	*/
        /* --- cacheline 1 boundary (64 bytes) --- */
        u32		id;		/*    64     4	*/
        int		numa_node;	/*    68     4	*/
	...
        bool		frozen;		/*   117     1	*/

        /* XXX 10 bytes hole, try to pack */

        /* --- cacheline 2 boundary (128 bytes) --- */
	...
        struct work_struct	work;	/*   144    72	*/

        /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */
	struct mutex	freeze_mutex;	/*   216   144 	*/

        /* --- cacheline 5 boundary (320 bytes) was 40 bytes ago --- */
        u64		writecnt; 	/*   360     8	*/

    /* size: 384, cachelines: 6, members: 26 */
    /* sum members: 354, holes: 2, sum holes: 14 */
    /* padding: 16 */
    /* forced alignments: 2, forced holes: 1, sum forced holes: 10 */

} __attribute__((__aligned__(64)));

After:
struct bpf_map {
	...
        u32		max_entries;	/*    36     4	*/
        u64		map_extra;	/*    40     8 	*/
        u32		map_flags;	/*    48     4	*/
        int		spin_lock_off;	/*    52     4	*/
        int		timer_off;	/*    56     4	*/
        u32		id;		/*    60     4	*/

        /* --- cacheline 1 boundary (64 bytes) --- */
        int		numa_node;	/*    64     4	*/
	...
	bool		frozen		/*   113     1  */

        /* XXX 14 bytes hole, try to pack */

        /* --- cacheline 2 boundary (128 bytes) --- */
	...
        struct work_struct	work;	/*   144    72	*/

        /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */
        struct mutex	freeze_mutex;	/*   216   144	*/

        /* --- cacheline 5 boundary (320 bytes) was 40 bytes ago --- */
        u64		writecnt;       /*   360     8	*/

    /* size: 384, cachelines: 6, members: 26 */
    /* sum members: 354, holes: 1, sum holes: 14 */
    /* padding: 16 */
    /* forced alignments: 2, forced holes: 1, sum forced holes: 14 */

} __attribute__((__aligned__(64)));

2) Add alignment padding to the bpf_map_info struct
More details can be found in commit 36f9814a494a ("bpf: fix uapi hole
for 32 bit compat applications")

Signed-off-by: Joanne Koong <joannekoong@fb.com>
---
 include/linux/bpf.h            | 6 +++---
 include/uapi/linux/bpf.h       | 1 +
 tools/include/uapi/linux/bpf.h | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 6deebf8bf78f..d695b2ef67d5 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -168,23 +168,23 @@ struct bpf_map {
 	u32 key_size;
 	u32 value_size;
 	u32 max_entries;
-	u32 map_flags;
 	u64 map_extra; /* any per-map-type extra fields */
+	u32 map_flags;
 	int spin_lock_off; /* >=0 valid offset, <0 error */
 	int timer_off; /* >=0 valid offset, <0 error */
 	u32 id;
 	int numa_node;
 	u32 btf_key_type_id;
 	u32 btf_value_type_id;
+	u32 btf_vmlinux_value_type_id;
 	struct btf *btf;
 #ifdef CONFIG_MEMCG_KMEM
 	struct mem_cgroup *memcg;
 #endif
 	char name[BPF_OBJ_NAME_LEN];
-	u32 btf_vmlinux_value_type_id;
 	bool bypass_spec_v1;
 	bool frozen; /* write-once; write-protected by freeze_mutex */
-	/* 22 bytes hole */
+	/* 14 bytes hole */
 
 	/* The 3rd and 4th cacheline with misc members to avoid false sharing
 	 * particularly with refcounting.
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bd0c9f0487f6..ba5af15e25f5 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5662,6 +5662,7 @@ struct bpf_map_info {
 	__u32 btf_id;
 	__u32 btf_key_type_id;
 	__u32 btf_value_type_id;
+	__u32 :32;	/* alignment pad */
 	__u64 map_extra;
 } __attribute__((aligned(8)));
 
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index bd0c9f0487f6..ba5af15e25f5 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5662,6 +5662,7 @@ struct bpf_map_info {
 	__u32 btf_id;
 	__u32 btf_key_type_id;
 	__u32 btf_value_type_id;
+	__u32 :32;	/* alignment pad */
 	__u64 map_extra;
 } __attribute__((aligned(8)));
 
-- 
2.30.2

Re: [PATCH bpf-next 2/3] bpf: Add alignment padding for "map_extra" + consolidate holes

[Yonghong Song]

On 10/29/21 10:01 AM, Joanne Koong wrote:
> This patch makes 2 changes regarding alignment padding
> for the "map_extra" field.
> 
> 1) In the kernel header, "map_extra" and "btf_value_type_id"
> are rearranged to consolidate the hole.
> 
> Before:
> struct bpf_map {
> 	...
>          u32		max_entries;	/*    36     4	*/
>          u32		map_flags;	/*    40     4	*/
> 
>          /* XXX 4 bytes hole, try to pack */
> 
>          u64		map_extra;	/*    48     8	*/
>          int		spin_lock_off;	/*    56     4	*/
>          int		timer_off;	/*    60     4	*/
>          /* --- cacheline 1 boundary (64 bytes) --- */
>          u32		id;		/*    64     4	*/
>          int		numa_node;	/*    68     4	*/
> 	...
>          bool		frozen;		/*   117     1	*/
> 
>          /* XXX 10 bytes hole, try to pack */
> 
>          /* --- cacheline 2 boundary (128 bytes) --- */
> 	...
>          struct work_struct	work;	/*   144    72	*/
> 
>          /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */
> 	struct mutex	freeze_mutex;	/*   216   144 	*/
> 
>          /* --- cacheline 5 boundary (320 bytes) was 40 bytes ago --- */
>          u64		writecnt; 	/*   360     8	*/
> 
>      /* size: 384, cachelines: 6, members: 26 */
>      /* sum members: 354, holes: 2, sum holes: 14 */
>      /* padding: 16 */
>      /* forced alignments: 2, forced holes: 1, sum forced holes: 10 */
> 
> } __attribute__((__aligned__(64)));
> 
> After:
> struct bpf_map {
> 	...
>          u32		max_entries;	/*    36     4	*/
>          u64		map_extra;	/*    40     8 	*/
>          u32		map_flags;	/*    48     4	*/
>          int		spin_lock_off;	/*    52     4	*/
>          int		timer_off;	/*    56     4	*/
>          u32		id;		/*    60     4	*/
> 
>          /* --- cacheline 1 boundary (64 bytes) --- */
>          int		numa_node;	/*    64     4	*/
> 	...
> 	bool		frozen		/*   113     1  */
> 
>          /* XXX 14 bytes hole, try to pack */
> 
>          /* --- cacheline 2 boundary (128 bytes) --- */
> 	...
>          struct work_struct	work;	/*   144    72	*/
> 
>          /* --- cacheline 3 boundary (192 bytes) was 24 bytes ago --- */
>          struct mutex	freeze_mutex;	/*   216   144	*/
> 
>          /* --- cacheline 5 boundary (320 bytes) was 40 bytes ago --- */
>          u64		writecnt;       /*   360     8	*/
> 
>      /* size: 384, cachelines: 6, members: 26 */
>      /* sum members: 354, holes: 1, sum holes: 14 */
>      /* padding: 16 */
>      /* forced alignments: 2, forced holes: 1, sum forced holes: 14 */
> 
> } __attribute__((__aligned__(64)));
> 
> 2) Add alignment padding to the bpf_map_info struct
> More details can be found in commit 36f9814a494a ("bpf: fix uapi hole
> for 32 bit compat applications")
> 
> Signed-off-by: Joanne Koong <joannekoong@fb.com>

Acked-by: Yonghong Song <yhs@fb.com>

[PATCH bpf-next 3/3] selftests/bpf: Add bloom map success test for userspace calls

[Joanne Koong]

This patch has two changes:
1) Adds a new function "test_success_cases" to test
successfully creating + adding + looking up a value
in a bloom filter map from the userspace side.

2) Use bpf_create_map instead of bpf_create_map_xattr in
the "test_fail_cases" to make the code look cleaner.

Signed-off-by: Joanne Koong <joannekoong@fb.com>
---
 .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
 1 file changed, 33 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
index 9aa3fbed918b..dbc0035e43e5 100644
--- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
+++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
@@ -7,44 +7,32 @@
 
 static void test_fail_cases(void)
 {
-	struct bpf_create_map_attr xattr = {
-		.name = "bloom_filter_map",
-		.map_type = BPF_MAP_TYPE_BLOOM_FILTER,
-		.max_entries = 100,
-		.value_size = 11,
-	};
 	__u32 value;
 	int fd, err;
 
 	/* Invalid key size */
-	xattr.key_size = 4;
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 4, sizeof(value), 100, 0);
 	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid key size"))
 		close(fd);
-	xattr.key_size = 0;
 
 	/* Invalid value size */
-	xattr.value_size = 0;
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, 0, 100, 0);
 	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid value size 0"))
 		close(fd);
-	xattr.value_size = 11;
 
 	/* Invalid max entries size */
-	xattr.max_entries = 0;
-	fd = bpf_create_map_xattr(&xattr);
-	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid max entries size"))
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 0, 0);
+	if (!ASSERT_LT(fd, 0,
+		       "bpf_create_map bloom filter invalid max entries size"))
 		close(fd);
-	xattr.max_entries = 100;
 
 	/* Bloom filter maps do not support BPF_F_NO_PREALLOC */
-	xattr.map_flags = BPF_F_NO_PREALLOC;
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100,
+			    BPF_F_NO_PREALLOC);
 	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid flags"))
 		close(fd);
-	xattr.map_flags = 0;
 
-	fd = bpf_create_map_xattr(&xattr);
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100, 0);
 	if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter"))
 		return;
 
@@ -67,6 +55,30 @@ static void test_fail_cases(void)
 	close(fd);
 }
 
+static void test_success_cases(void)
+{
+	char value[11];
+	int fd, err;
+
+	/* Create a map */
+	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100,
+			    BPF_F_ZERO_SEED | BPF_F_NUMA_NODE);
+	if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter success case"))
+		return;
+
+	/* Add a value to the bloom filter */
+	err = bpf_map_update_elem(fd, NULL, &value, 0);
+	if (!ASSERT_OK(err, "bpf_map_update_elem bloom filter success case"))
+		goto done;
+
+	 /* Lookup a value in the bloom filter */
+	err = bpf_map_lookup_elem(fd, NULL, &value);
+	ASSERT_OK(err, "bpf_map_update_elem bloom filter success case");
+
+done:
+	close(fd);
+}
+
 static void check_bloom(struct bloom_filter_map *skel)
 {
 	struct bpf_link *link;
@@ -190,6 +202,7 @@ void test_bloom_filter_map(void)
 	int err;
 
 	test_fail_cases();
+	test_success_cases();
 
 	err = setup_progs(&skel, &rand_vals, &nr_rand_vals);
 	if (err)
-- 
2.30.2

Re: [PATCH bpf-next 3/3] selftests/bpf: Add bloom map success test for userspace calls

[Yonghong Song]

On 10/29/21 10:01 AM, Joanne Koong wrote:
> This patch has two changes:
> 1) Adds a new function "test_success_cases" to test
> successfully creating + adding + looking up a value
> in a bloom filter map from the userspace side.
> 
> 2) Use bpf_create_map instead of bpf_create_map_xattr in
> the "test_fail_cases" to make the code look cleaner.
> 
> Signed-off-by: Joanne Koong <joannekoong@fb.com>

LGTM with one minor comment below.

Acked-by: Yonghong Song <yhs@fb.com>

> ---
>   .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
>   1 file changed, 33 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> index 9aa3fbed918b..dbc0035e43e5 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
> @@ -7,44 +7,32 @@
>   
>   static void test_fail_cases(void)
>   {
> -	struct bpf_create_map_attr xattr = {
> -		.name = "bloom_filter_map",
> -		.map_type = BPF_MAP_TYPE_BLOOM_FILTER,
> -		.max_entries = 100,
> -		.value_size = 11,
> -	};
>   	__u32 value;
>   	int fd, err;
>   
>   	/* Invalid key size */
> -	xattr.key_size = 4;
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 4, sizeof(value), 100, 0);
>   	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid key size"))
>   		close(fd);
> -	xattr.key_size = 0;
>   
>   	/* Invalid value size */
> -	xattr.value_size = 0;
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, 0, 100, 0);
>   	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid value size 0"))
>   		close(fd);
> -	xattr.value_size = 11;
>   
>   	/* Invalid max entries size */
> -	xattr.max_entries = 0;
> -	fd = bpf_create_map_xattr(&xattr);
> -	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid max entries size"))
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 0, 0);
> +	if (!ASSERT_LT(fd, 0,
> +		       "bpf_create_map bloom filter invalid max entries size"))

It is OK to have "bpf_create_map ..." in the same line as ASSERT_LT
for better readability and consistent with other ASSERT_LT. It is over 
80 but less than 100 char's per line.

>   		close(fd);
> -	xattr.max_entries = 100;
>   
>   	/* Bloom filter maps do not support BPF_F_NO_PREALLOC */
> -	xattr.map_flags = BPF_F_NO_PREALLOC;
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100,
> +			    BPF_F_NO_PREALLOC);
>   	if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid flags"))
>   		close(fd);
> -	xattr.map_flags = 0;
>   
> -	fd = bpf_create_map_xattr(&xattr);
> +	fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 100, 0);
>   	if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter"))
>   		return;
>   
> @@ -67,6 +55,30 @@ static void test_fail_cases(void)
>   	close(fd);
>   }
[...]

Re: [PATCH bpf-next 3/3] selftests/bpf: Add bloom map success test for userspace calls

[Joanne Koong]

On 10/29/21 3:04 PM, Yonghong Song wrote:

> On 10/29/21 10:01 AM, Joanne Koong wrote:
>> This patch has two changes:
>> 1) Adds a new function "test_success_cases" to test
>> successfully creating + adding + looking up a value
>> in a bloom filter map from the userspace side.
>>
>> 2) Use bpf_create_map instead of bpf_create_map_xattr in
>> the "test_fail_cases" to make the code look cleaner.
>>
>> Signed-off-by: Joanne Koong <joannekoong@fb.com>
>
> LGTM with one minor comment below.
>
> Acked-by: Yonghong Song <yhs@fb.com>
>
>> ---
>>   .../bpf/prog_tests/bloom_filter_map.c         | 53 ++++++++++++-------
>>   1 file changed, 33 insertions(+), 20 deletions(-)
>>
>> diff --git 
>> a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c 
>> b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
>> index 9aa3fbed918b..dbc0035e43e5 100644
>> --- a/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
>> +++ b/tools/testing/selftests/bpf/prog_tests/bloom_filter_map.c
>> @@ -7,44 +7,32 @@
>>     static void test_fail_cases(void)
>>   {
>> -    struct bpf_create_map_attr xattr = {
>> -        .name = "bloom_filter_map",
>> -        .map_type = BPF_MAP_TYPE_BLOOM_FILTER,
>> -        .max_entries = 100,
>> -        .value_size = 11,
>> -    };
>>       __u32 value;
>>       int fd, err;
>>         /* Invalid key size */
>> -    xattr.key_size = 4;
>> -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 4, sizeof(value), 
>> 100, 0);
>>       if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid key 
>> size"))
>>           close(fd);
>> -    xattr.key_size = 0;
>>         /* Invalid value size */
>> -    xattr.value_size = 0;
>> -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, 0, 100, 0);
>>       if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid 
>> value size 0"))
>>           close(fd);
>> -    xattr.value_size = 11;
>>         /* Invalid max entries size */
>> -    xattr.max_entries = 0;
>> -    fd = bpf_create_map_xattr(&xattr);
>> -    if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid max 
>> entries size"))
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 
>> 0, 0);
>> +    if (!ASSERT_LT(fd, 0,
>> +               "bpf_create_map bloom filter invalid max entries size"))
>
> It is OK to have "bpf_create_map ..." in the same line as ASSERT_LT
> for better readability and consistent with other ASSERT_LT. It is over 
> 80 but less than 100 char's per line.
>
Great, I will send out v2 of this patchset where this line break is 
removed.
Thanks for reviewing the patchset!
>>           close(fd);
>> -    xattr.max_entries = 100;
>>         /* Bloom filter maps do not support BPF_F_NO_PREALLOC */
>> -    xattr.map_flags = BPF_F_NO_PREALLOC;
>> -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 
>> 100,
>> +                BPF_F_NO_PREALLOC);
>>       if (!ASSERT_LT(fd, 0, "bpf_create_map bloom filter invalid 
>> flags"))
>>           close(fd);
>> -    xattr.map_flags = 0;
>>   -    fd = bpf_create_map_xattr(&xattr);
>> +    fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(value), 
>> 100, 0);
>>       if (!ASSERT_GE(fd, 0, "bpf_create_map bloom filter"))
>>           return;
>>   @@ -67,6 +55,30 @@ static void test_fail_cases(void)
>>       close(fd);
>>   }
> [...]

[PATCH bpf-next 0/3] "map_extra" and bloom filter fixups

[Joanne Koong]

There are 3 patches in this patchset:

1/3 - Bloom filter naming fixups (kernel/bpf/bloom_filter.c)

2/3 - Add alignment padding for map_extra, rearrange fields in
bpf_map struct to consolidate holes

3/3 - Bloom filter tests (prog_tests/bloom_filter_map):
Add test for successful userspace calls, some refactoring to
use bpf_create_map instead of bpf_create_map_xattr

v1 -> v2:
    * In prog_tests/bloom_filter_map: remove unneeded line break,
	also change the inner_map_test to use bpf_create_map instead
	of bpf_create_map_xattr.
    * Add acked-bys to commit messages

Joanne Koong (3):
  bpf: Bloom filter map naming fixups
  bpf: Add alignment padding for "map_extra" + consolidate holes
  selftests/bpf: Add bloom map success test for userspace calls

 include/linux/bpf.h                           |  6 +-
 include/uapi/linux/bpf.h                      |  1 +
 kernel/bpf/bloom_filter.c                     | 49 +++++++--------
 tools/include/uapi/linux/bpf.h                |  1 +
 .../bpf/prog_tests/bloom_filter_map.c         | 59 +++++++++++--------
 5 files changed, 64 insertions(+), 52 deletions(-)

-- 
2.30.2

[PATCH bpf-next 1/3] bpf: Bloom filter map naming fixups

[Joanne Koong]

This patch has two changes in the kernel bloom filter map
implementation:

1) Change the names of map-ops functions to include the
"bloom_map" prefix.

As Martin pointed out on a previous patchset, having generic
map-ops names may be confusing in tracing and in perf-report.

2) Drop the "& 0xF" when getting nr_hash_funcs, since we
already ascertain that no other bits in map_extra beyond the
first 4 bits can be set.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Joanne Koong <joannekoong@fb.com>
---
 kernel/bpf/bloom_filter.c | 49 +++++++++++++++++++++------------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/kernel/bpf/bloom_filter.c b/kernel/bpf/bloom_filter.c
index 7c50232b7571..073c2f2cab8b 100644
--- a/kernel/bpf/bloom_filter.c
+++ b/kernel/bpf/bloom_filter.c
@@ -40,7 +40,7 @@ static u32 hash(struct bpf_bloom_filter *bloom, void *value,
 	return h & bloom->bitset_mask;
 }
 
-static int peek_elem(struct bpf_map *map, void *value)
+static int bloom_map_peek_elem(struct bpf_map *map, void *value)
 {
 	struct bpf_bloom_filter *bloom =
 		container_of(map, struct bpf_bloom_filter, map);
@@ -55,7 +55,7 @@ static int peek_elem(struct bpf_map *map, void *value)
 	return 0;
 }
 
-static int push_elem(struct bpf_map *map, void *value, u64 flags)
+static int bloom_map_push_elem(struct bpf_map *map, void *value, u64 flags)
 {
 	struct bpf_bloom_filter *bloom =
 		container_of(map, struct bpf_bloom_filter, map);
@@ -72,12 +72,12 @@ static int push_elem(struct bpf_map *map, void *value, u64 flags)
 	return 0;
 }
 
-static int pop_elem(struct bpf_map *map, void *value)
+static int bloom_map_pop_elem(struct bpf_map *map, void *value)
 {
 	return -EOPNOTSUPP;
 }
 
-static struct bpf_map *map_alloc(union bpf_attr *attr)
+static struct bpf_map *bloom_map_alloc(union bpf_attr *attr)
 {
 	u32 bitset_bytes, bitset_mask, nr_hash_funcs, nr_bits;
 	int numa_node = bpf_map_attr_numa_node(attr);
@@ -90,11 +90,13 @@ static struct bpf_map *map_alloc(union bpf_attr *attr)
 	    attr->max_entries == 0 ||
 	    attr->map_flags & ~BLOOM_CREATE_FLAG_MASK ||
 	    !bpf_map_flags_access_ok(attr->map_flags) ||
+	    /* The lower 4 bits of map_extra (0xF) specify the number
+	     * of hash functions
+	     */
 	    (attr->map_extra & ~0xF))
 		return ERR_PTR(-EINVAL);
 
-	/* The lower 4 bits of map_extra specify the number of hash functions */
-	nr_hash_funcs = attr->map_extra & 0xF;
+	nr_hash_funcs = attr->map_extra;
 	if (nr_hash_funcs == 0)
 		/* Default to using 5 hash functions if unspecified */
 		nr_hash_funcs = 5;
@@ -150,7 +152,7 @@ static struct bpf_map *map_alloc(union bpf_attr *attr)
 	return &bloom->map;
 }
 
-static void map_free(struct bpf_map *map)
+static void bloom_map_free(struct bpf_map *map)
 {
 	struct bpf_bloom_filter *bloom =
 		container_of(map, struct bpf_bloom_filter, map);
@@ -158,38 +160,39 @@ static void map_free(struct bpf_map *map)
 	bpf_map_area_free(bloom);
 }
 
-static void *lookup_elem(struct bpf_map *map, void *key)
+static void *bloom_map_lookup_elem(struct bpf_map *map, void *key)
 {
 	/* The eBPF program should use map_peek_elem instead */
 	return ERR_PTR(-EINVAL);
 }
 
-static int update_elem(struct bpf_map *map, void *key,
-		       void *value, u64 flags)
+static int bloom_map_update_elem(struct bpf_map *map, void *key,
+				 void *value, u64 flags)
 {
 	/* The eBPF program should use map_push_elem instead */
 	return -EINVAL;
 }
 
-static int check_btf(const struct bpf_map *map, const struct btf *btf,
-		     const struct btf_type *key_type,
-		     const struct btf_type *value_type)
+static int bloom_map_check_btf(const struct bpf_map *map,
+			       const struct btf *btf,
+			       const struct btf_type *key_type,
+			       const struct btf_type *value_type)
 {
 	/* Bloom filter maps are keyless */
 	return btf_type_is_void(key_type) ? 0 : -EINVAL;
 }
 
-static int bpf_bloom_btf_id;
+static int bpf_bloom_map_btf_id;
 const struct bpf_map_ops bloom_filter_map_ops = {
 	.map_meta_equal = bpf_map_meta_equal,
-	.map_alloc = map_alloc,
-	.map_free = map_free,
-	.map_push_elem = push_elem,
-	.map_peek_elem = peek_elem,
-	.map_pop_elem = pop_elem,
-	.map_lookup_elem = lookup_elem,
-	.map_update_elem = update_elem,
-	.map_check_btf = check_btf,
+	.map_alloc = bloom_map_alloc,
+	.map_free = bloom_map_free,
+	.map_push_elem = bloom_map_push_elem,
+	.map_peek_elem = bloom_map_peek_elem,
+	.map_pop_elem = bloom_map_pop_elem,
+	.map_lookup_elem = bloom_map_lookup_elem,
+	.map_update_elem = bloom_map_update_elem,
+	.map_check_btf = bloom_map_check_btf,
 	.map_btf_name = "bpf_bloom_filter",
-	.map_btf_id = &bpf_bloom_btf_id,
+	.map_btf_id = &bpf_bloom_map_btf_id,
 };
-- 
2.30.2