Re: [PATCH v6 bpf-next 1/5] bpf: Add bloom filter map implementation

[Martin KaFai Lau <kafai@fb.com>]

On Thu, Oct 28, 2021 at 10:52:22PM -0700, Joanne Koong wrote:
> On 10/28/21 9:49 PM, Martin KaFai Lau wrote:
> 
> > On Thu, Oct 28, 2021 at 08:17:23PM -0700, Joanne Koong wrote:
> > > On 10/28/21 2:14 PM, Martin KaFai Lau wrote:
> > > 
> > > On Wed, Oct 27, 2021 at 04:45:00PM -0700, Joanne Koong wrote:
> > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index
> > > 31421c74ba08..50105e0b8fcc 100644 > --- a/include/linux/bpf.h > +++
> > > b/include/linux/bpf.h > @@ -169,6 +169,7 @@ struct bpf_map { The
> > > earlier context is copied here:
> > > 
> > > 	struct bpf_map *inner_map_meta;
> > > #ifdef CONFIG_SECURITY
> > >          void *security;
> > > #endif
> > > 
> > > >  	u32 value_size; > u32 max_entries; > u32 map_flags; > + u64
> > > map_extra; /* any per-map-type extra fields */ There is a 4 byte
> > > hole before the new 'u64 map_extra'.  Try to move
> > > it before map_flags
> 
> Manually resuscitating your previous comment back into this email ^.
> 
> After rebasing to the latest master, I'm not seeing a significant difference
> anymore with map_extra before/after max_flags. This is what I see when
> running "pahole vmlinux.o":
> 
> With map_extra AFTER map_flags:
> 
> struct bpf_map {
>         const struct bpf_map_ops  * ops __attribute__((__aligned__(64)));
> /*     0     8 */
>         struct bpf_map *           inner_map_meta;       /* 8     8 */
>         void *                     security;             /* 16     8 */
>         enum bpf_map_type          map_type;             /* 24     4 */
>         u32                        key_size;             /* 28     4 */
>         u32                        value_size;           /* 32     4 */
>         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 */
>         u32                        btf_key_type_id;      /* 72     4 */
>         u32                        btf_value_type_id;    /* 76     4 */
>         struct btf *               btf;                  /* 80     8 */
>         struct mem_cgroup *        memcg;                /* 88     8 */
>         char                       name[16];             /* 96    16 */
>         u32                        btf_vmlinux_value_type_id; /* 112     4
> */
>         bool                       bypass_spec_v1;       /* 116     1 */
>         bool                       frozen;               /* 117     1 */
> 
>         /* XXX 10 bytes hole, try to pack */
> 
>         /* --- cacheline 2 boundary (128 bytes) --- */
>         atomic64_t                 refcnt __attribute__((__aligned__(64)));
> /*   128     8 */
>         atomic64_t                 usercnt;              /* 136     8 */
>         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)));
> 
> 
> With map_extra BEFORE map_flags:
> 
> struct bpf_map {
>         const struct bpf_map_ops  * ops __attribute__((__aligned__(64)));
> /*     0     8 */
>         struct bpf_map *           inner_map_meta;       /* 8     8 */
>         void *                     security;             /* 16     8 */
>         enum bpf_map_type          map_type;             /* 24     4 */
>         u32                        key_size;             /* 28     4 */
>         u32                        value_size;           /* 32     4 */
>         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 */
>         u32                        btf_key_type_id;      /* 68     4 */
>         u32                        btf_value_type_id;    /* 72     4 */
> 
>         /* XXX 4 bytes hole, try to pack */
> 
>         struct btf *               btf;                  /* 80     8 */
>         struct mem_cgroup *        memcg;                /* 88     8 */
>         char                       name[16];             /* 96    16 */
>         u32                        btf_vmlinux_value_type_id; /* 112     4
> */
>         bool                       bypass_spec_v1;       /* 116     1 */
>         bool                       frozen;               /* 117     1 */
> 
>         /* XXX 10 bytes hole, try to pack */
> 
>         /* --- cacheline 2 boundary (128 bytes) --- */
>         atomic64_t                 refcnt __attribute__((__aligned__(64)));
> /*   128     8 */
>         atomic64_t                 usercnt;              /* 136     8 */
>         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)));
> 
> 
> The main difference is that the "id" field is part of the 2nd cacheline when
> "map_extra" is after "map_flags", and is part of the 1st cacheline when
> "map_extra" is before "map_flags".
> 
> Do you think it's still worth it to move "map_extra" to before "map_flags"?
It looks like there is an existing 4 byte hole.  I would take this chance
to plunge it by using an existing 4 byte field.  Something like this:

diff --git i/include/linux/bpf.h w/include/linux/bpf.h
index 50105e0b8fcc..0e07c659acd4 100644
--- i/include/linux/bpf.h
+++ w/include/linux/bpf.h
@@ -169,22 +169,22 @@ struct bpf_map {
 	u32 value_size;
 	u32 max_entries;
 	u32 map_flags;
-	u64 map_extra; /* any per-map-type extra fields */
 	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;
+	u64 map_extra; /* any per-map-type extra fields */
 	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.