Re: [PATCH bpf-next] bpf: Reduce smap->elem_size

[Martin KaFai Lau <martin.lau@linux.dev>]

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