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

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

[Martin KaFai Lau]

From: Martin KaFai Lau <martin.lau@kernel.org>

'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
offsetofend() instead to calculate more exact memory use.

Acked-by: Yonghong Song <yhs@fb.com>
Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
---
v2:
  Rephrase the commit message (Andrii and Yonghong)
  Use offsetofend instead of offsetof (Andrii)

 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..e73fc70071b0 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 = offsetofend(struct bpf_local_storage_elem, sdata) +
+		attr->value_size;
 
 	return smap;
 }
-- 
2.30.2

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

[Andrii Nakryiko]

On Tue, Dec 20, 2022 at 5:15 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>
> From: Martin KaFai Lau <martin.lau@kernel.org>
>
> '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
> offsetofend() instead to calculate more exact memory use.
>
> Acked-by: Yonghong Song <yhs@fb.com>
> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
> ---
> v2:
>   Rephrase the commit message (Andrii and Yonghong)
>   Use offsetofend instead of offsetof (Andrii)
>
>  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..e73fc70071b0 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 = offsetofend(struct bpf_local_storage_elem, sdata) +
> +               attr->value_size;

Heh, we raced down to a minute. Copy/pasting my latest reply from
original thread.

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


Whichever you prefer, both versions work for me:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>
>         return smap;
>  }
> --
> 2.30.2
>

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

[Martin KaFai Lau]

On 12/20/22 5:17 PM, Andrii Nakryiko wrote:
> On Tue, Dec 20, 2022 at 5:15 PM Martin KaFai Lau <martin.lau@linux.dev> wrote:
>>
>> From: Martin KaFai Lau <martin.lau@kernel.org>
>>
>> '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
>> offsetofend() instead to calculate more exact memory use.
>>
>> Acked-by: Yonghong Song <yhs@fb.com>
>> Signed-off-by: Martin KaFai Lau <martin.lau@kernel.org>
>> ---
>> v2:
>>    Rephrase the commit message (Andrii and Yonghong)
>>    Use offsetofend instead of offsetof (Andrii)
>>
>>   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..e73fc70071b0 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 = offsetofend(struct bpf_local_storage_elem, sdata) +
>> +               attr->value_size;
> 
> Heh, we raced down to a minute. Copy/pasting my latest reply from
> original thread.

lol.  email is more parallel than most people thought :)

> 
> 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]);

Sure. will post v3.

> 
> 
> sdata is embedded struct, no pointer dereference, so single offsetof()
> should be enough to peer inside it
> 
> 
> Whichever you prefer, both versions work for me:
> 
> Acked-by: Andrii Nakryiko <andrii@kernel.org>

Thanks for the review.