[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

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

[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")

Acked-by: Yonghong Song <yhs@fb.com>
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

[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" and test_inner_map to make the
code look cleaner.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Joanne Koong <joannekoong@fb.com>
---
 .../bpf/prog_tests/bloom_filter_map.c         | 59 +++++++++++--------
 1 file changed, 33 insertions(+), 26 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..be73e3de6668 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,31 @@
 
 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);
+	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 +54,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;
@@ -86,16 +97,11 @@ static void test_inner_map(struct bloom_filter_map *skel, const __u32 *rand_vals
 			   __u32 nr_rand_vals)
 {
 	int outer_map_fd, inner_map_fd, err, i, key = 0;
-	struct bpf_create_map_attr xattr = {
-		.name = "bloom_filter_inner_map",
-		.map_type = BPF_MAP_TYPE_BLOOM_FILTER,
-		.value_size = sizeof(__u32),
-		.max_entries = nr_rand_vals,
-	};
 	struct bpf_link *link;
 
 	/* Create a bloom filter map that will be used as the inner map */
-	inner_map_fd = bpf_create_map_xattr(&xattr);
+	inner_map_fd = bpf_create_map(BPF_MAP_TYPE_BLOOM_FILTER, 0, sizeof(*rand_vals),
+				      nr_rand_vals, 0);
 	if (!ASSERT_GE(inner_map_fd, 0, "bpf_create_map bloom filter inner map"))
 		return;
 
@@ -190,6 +196,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 0/3] "map_extra" and bloom filter fixups

[Joanne Koong]

Apologies, I forgot to include "v2" in the email header -
this is the second revision of the patchset
(v1 is here: 
https://lore.kernel.org/bpf/20211029224909.1721024-1-joannekoong@fb.com/)

On 10/29/21 3:49 PM, Joanne Koong wrote:
> 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(-)
>

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

[Martin KaFai Lau]

On Fri, Oct 29, 2021 at 03:49:06PM -0700, Joanne Koong wrote:
> 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
Acked-by: Martin KaFai Lau <kafai@fb.com>

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