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

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

[Martin KaFai Lau]

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
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>
---
 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;
 }
-- 
2.30.2

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

[Yonghong Song]

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;
>   }

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

[Martin KaFai Lau]

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

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

[Andrii Nakryiko]

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