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.
