| Message ID | 20221216232951.3575596-1-martin.lau@linux.dev (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next] bpf: Reduce smap->elem_size | expand |
On 12/16/22 3:29 PM, Martin KaFai Lau wrote: > From: Martin KaFai Lau <martin.lau@kernel.org> > > 'struct bpf_local_storage_elem' has a 56 bytes padding at the end > which can be used for attr->value_size. The current smap->elem_size 'can be' => 'will be'? > calculation is unnecessarily inflated by 56 bytes. > > The patch is to fix it by calculating the smap->elem_size > with offsetof(). > > Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org> Acked-by: Yonghong Song <yhs@fb.com> > --- > kernel/bpf/bpf_local_storage.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c > index b39a46e8fb08..cb43e70613b1 100644 > --- a/kernel/bpf/bpf_local_storage.c > +++ b/kernel/bpf/bpf_local_storage.c > @@ -580,8 +580,8 @@ static struct bpf_local_storage_map *__bpf_local_storage_map_alloc(union bpf_att > raw_spin_lock_init(&smap->buckets[i].lock); > } > > - smap->elem_size = > - sizeof(struct bpf_local_storage_elem) + attr->value_size; > + smap->elem_size = offsetof(struct bpf_local_storage_elem, sdata) + > + offsetof(struct bpf_local_storage_data, data[attr->value_size]); > > return smap; > }
[ Cc: the bpf list back. I dropped it by mistake in my last reply. ] On 12/20/22 3:43 PM, Andrii Nakryiko wrote: > On Mon, Dec 19, 2022 at 11:47 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: >> >> On 12/16/22 5:23 PM, Yonghong Song wrote: >>> >>> >>> On 12/16/22 3:29 PM, Martin KaFai Lau wrote: >>>> From: Martin KaFai Lau <martin.lau@kernel.org> >>>> >>>> 'struct bpf_local_storage_elem' has a 56 bytes padding at the end >>>> which can be used for attr->value_size. The current smap->elem_size >>> >>> 'can be' => 'will be'? >> >> I used 'can be' to describe the current situation that the padding is not used >> for the map's value. I may have used the wrong tense? >> >> I can rephrase it to something like, >> >> 'struct bpf_local_storage_elem' has a 56 bytes padding at the end which is >> currently not used for the attr->value_size. > > I actually found the use of attr->value_size to mean "value content" > more confusing than can vs will be. > > As a suggestion something like the below? > > > 'struct bpf_local_storage_elem' has an unused 56 byte padding at the > end due to struct's cache-line alignment requirement. This padding > space is overlapped by storage value contents, so if we use sizeof() > to calculate the total size, we overinflate it by 56 bytes. Use > offsetof() instead to calculate more exact memory use. SGTM. > > > btw, I think you can calculate the same arguably a bit more > straightforwardly as: > > smap->elem_size = offsetofend(struct bpf_local_storage_elem, sdata) + > attr->value_size; Sure. will change. > > right? > > but TIL that offsetof(struct bpf_local_storage_data, > data[attr->value_size]) does the right thing Yeah, I think I have seen other places using it also. I found it more intuitive to read with array[size] to tell there is a flexible array at the end. I am not super attached to it and will change to the way above.
On Tue, Dec 20, 2022 at 4:56 PM Martin KaFai Lau <martin.lau@linux.dev> wrote: > > [ Cc: the bpf list back. I dropped it by mistake in my last reply. ] > > On 12/20/22 3:43 PM, Andrii Nakryiko wrote: > > On Mon, Dec 19, 2022 at 11:47 AM Martin KaFai Lau <martin.lau@linux.dev> wrote: > >> > >> On 12/16/22 5:23 PM, Yonghong Song wrote: > >>> > >>> > >>> On 12/16/22 3:29 PM, Martin KaFai Lau wrote: > >>>> From: Martin KaFai Lau <martin.lau@kernel.org> > >>>> > >>>> 'struct bpf_local_storage_elem' has a 56 bytes padding at the end > >>>> which can be used for attr->value_size. The current smap->elem_size > >>> > >>> 'can be' => 'will be'? > >> > >> I used 'can be' to describe the current situation that the padding is not used > >> for the map's value. I may have used the wrong tense? > >> > >> I can rephrase it to something like, > >> > >> 'struct bpf_local_storage_elem' has a 56 bytes padding at the end which is > >> currently not used for the attr->value_size. > > > > I actually found the use of attr->value_size to mean "value content" > > more confusing than can vs will be. > > > > As a suggestion something like the below? > > > > > > 'struct bpf_local_storage_elem' has an unused 56 byte padding at the > > end due to struct's cache-line alignment requirement. This padding > > space is overlapped by storage value contents, so if we use sizeof() > > to calculate the total size, we overinflate it by 56 bytes. Use > > offsetof() instead to calculate more exact memory use. > > SGTM. > > > > > > > btw, I think you can calculate the same arguably a bit more > > straightforwardly as: > > > > smap->elem_size = offsetofend(struct bpf_local_storage_elem, sdata) + > > attr->value_size; > > Sure. will change. > > > > > right? > > > > but TIL that offsetof(struct bpf_local_storage_data, > > data[attr->value_size]) does the right thing > > Yeah, I think I have seen other places using it also. I found it more intuitive > to read with array[size] to tell there is a flexible array at the end. I am not > super attached to it and will change to the way above. > I don't care either, was just surprised. But it just occurred to me that your change can be written equivalently (but now I do think it's cleaner) as: smap->elem_size = offsetof(struct bpf_local_storage_elem, sdata.data[attr->value_size]); sdata is embedded struct, no pointer dereference, so single offsetof() should be enough to peer inside it
diff --git a/kernel/bpf/bpf_local_storage.c b/kernel/bpf/bpf_local_storage.c index b39a46e8fb08..cb43e70613b1 100644 --- a/kernel/bpf/bpf_local_storage.c +++ b/kernel/bpf/bpf_local_storage.c @@ -580,8 +580,8 @@ static struct bpf_local_storage_map *__bpf_local_storage_map_alloc(union bpf_att raw_spin_lock_init(&smap->buckets[i].lock); } - smap->elem_size = - sizeof(struct bpf_local_storage_elem) + attr->value_size; + smap->elem_size = offsetof(struct bpf_local_storage_elem, sdata) + + offsetof(struct bpf_local_storage_data, data[attr->value_size]); return smap; }