Re: [RFC v2 2/3] atomic: Specify alignment for atomic_t and atomic64_t

[Arnd Bergmann]

On Tue, Sep 23, 2025, at 10:05, Finn Thain wrote:
> On Tue, 23 Sep 2025, Arnd Bergmann wrote:
>> On Tue, Sep 23, 2025, at 08:28, Finn Thain wrote:
>>
>> Maybe we should just change __alignof__(unsigned long long) to a plain 
>> '8' here and make that the minimum alignment everywhere, same as the 
>> atomic64_t alignment change.
>> 
>
> It would be better (less wasteful of memory) to specify the alignment 
> parameter to kmem_cache_create() only at those call sites where it 
> matters.

I think that's the idea: __alignof__(unsigned long long) is the
alignment that the ABI requires for allocating arbitrary data
structures that may contain 64-bit integers, so this is already
the minimum. Architectures that may have noncoherent DMA master
devices often need to raise the ARCH_KMALLOC_MINALIGN to the cache
line size but leave the ARCH_SLAB_MINALIGN at the ABI-required
minimum value. Any caller that needs a higher alignment than the
ABI minimum needs to specify that.

>> Alternatively, we can keep the __alignof__ here in order to reduce 
>> padding on architectures with a default 4-byte alignment for __u64, but 
>> then override ARCH_SLAB_MINALIGN and ARCH_KMALLOC_MINALIGN on m68k to be 
>> '4' instead of '2'.
>> 
>
> Raising that to 4 would probably have little or no effect (for m68k or any 
> other arch). Back when I prepared the RFC patch series, I instrumented 
> create_cache() in mm/slab_common.c, and those caches that were allocated 
> at boot (for my usual minimal m68k .config) were already aligned to 4 
> bytes or 16.

Ok

> Also, increasing ARCH_SLAB_MINALIGN to 8 didn't solve the VFS/TTY layer 
> problem I have with CONFIG_DEBUG_ATOMIC on m68k. So the culprit is not the 
> obvious suspect (a kmem cache of objects with atomic64_t members). There's 
> some other allocator at work and it's aligning objects to 2 bytes not 4.

In that case, my guess would be something that concatenates structures
in some variant of

struct s1 {
     int a;
     short b;
     int rest[];
};

struct s2 {
     atomic_t a;
};

     struct s1 *obj1 = kmalloc(sizeof(struct s1) + sizeof(struct s2), GFP_KERNEL);
     struct s2 *obj2 = (void*)&obj1[1];

which causes problems because s2 has a larger alignment than s1.
We've had problems with DMA to structures like this in the past.

The more common construct that does

struct s1 {
     short a;
     struct s2;
};

should not produce misaligned members in the inner struct,
unless the outer one has a __packed attribute. Unfortunately
we disabled -Wpacked-not-aligned, which would otherwise catch
that problem.

     Arnd