Re: [PATCH bpf-next] bpf: pack struct bpf_fib_lookup

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Anton Protopopov <aspsk@isovalent.com>
Date: Thu, 4 Apr 2024 09:56:51 +0200

> On 24/04/04 12:31, Arnd Bergmann wrote:
>> On Wed, Apr 3, 2024, at 23:00, Daniel Borkmann wrote:
>>> On 4/3/24 10:09 PM, Arnd Bergmann wrote:
>>>> On Wed, Apr 3, 2024, at 14:33, Anton Protopopov wrote:
>>>>>
>>>>> Declare this inner union as __attribute__((packed, aligned(2))) such
>>>>> that it always is of size 2 and is aligned to 16 bits.
>>>>
>>>> I think you probably want 32-bit alignment for the structure,
>>>> to keep the ABI unchanged on all other architectures.
>>>
>>> Fwiw, on x86 nothing should change on this regard, see below pahole dump
>>> before/after. I think similar might be true for other archs as otherwise
>>> we should have seen a kbuild bot complaint on hitting the size assert.
>>
>> It's not the structure layout that changes, just its alignment.
>> Of course this is unlikely to cause actual bugs, but if there there
>> is no real need to change it, I would leave the alignment the same
>> as before.
> 
> I think the struct will now be automatically 4-byte aligned, as it
> has the following layout:
> 
>     struct {
>             u8 a;
>             u8 b;
>             u16 c;
>             u16 d;
>             union { u16 e; u16 f; } __aligned__(2);
>             ...
>     };
> 
> So if the union is 2-byte aligned, then the struct is automatically
> 4-byte aligned, because its address is 6 bytes less than the address
> of the union. In fact, as Daniel posted above, pahole shows that the
> struct actually has __aligned__(4) attribute in the patched version.
> I can add explicit __aligned__(4) to make this clear.

I think it would be fine to not introduce an explicit attribute as long
as pahole shows the structure is already 4-byte aligned.

Regarding aligning it to 32 or 64 bytes -- the actual benefit can be
checked via scripts/bloat-o-meter. Usually alignment bigger than 8 bytes
don't change anything in the object code, at least on x86_64.

> 
>>         Arnd

Thanks,
Olek