On Tue, Dec 20, 2022 at 4:56 PM Martin KaFai Lau <martin.lau@xxxxxxxxx> 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@xxxxxxxxx> 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@xxxxxxxxxx> > >>>> > >>>> '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
