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

[Finn Thain <fthain@linux-m68k.org>]

On Tue, 30 Sep 2025, Arnd Bergmann wrote:

> On Tue, Sep 30, 2025, at 04:18, Finn Thain wrote:
> >
> > It turned out that the problem wasn't dynamic allocations, it was a 
> > local variable in the core locking code (kernel/locking/rwsem.c): a 
> > misaligned long used with an atomic operation (cmpxchg). To get 
> > natural alignment for 64-bit quantities, I had to align other local 
> > variables as well, such as the one in ktime_get_real_ts64_mg() that's 
> > used with atomic64_try_cmpxchg(). The atomic_t branch in my github 
> > repo has the patches I wrote for that.
> 
> It looks like the variable you get the warning for is not even the 
> atomic64_t but the 'old' argument to atomic64_try_cmpxchg(), at least in 
> some of the cases you found if not all of them.
> 
> I don't see where why there is a requirement to have that aligned at 
> all, even if we do require the atomic64_t to be naturally aligned, and I 
> would expect the same warning to hit on x86-32 and the other 
> architectures with 4-byte alignment of u64 variable on stack and .data.
> 

Right -- there's only one memory operand in a CAS instruction on m68k, and 
there's only one pointer in the C implementation in asm-generic.

> > To silence the misalignment WARN from CONFIG_DEBUG_ATOMIC, for 64-bit 
> > atomic operations, for my small m68k .config, it was also necesary to 
> > increase ARCH_SLAB_MINALIGN to 8. However, I'm not advocating a 
> > ARCH_SLAB_MINALIGN increase, as that wastes memory.
> 
> Have you tried to quantify the memory waste here?

I think it's entirely workload dependent. The memory efficiency question 
comes down to the misalignment distance as a proportion of the size of the 
allocation.

> I assume that most slab allocations are already 8-byte aligned, at least 
> kmalloc() with size>4, while custom caches are usually done for larger 
> structures where an extra average of 2 bytes per allocation may not be 
> that bad.
> 
> > diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> > index 402a999a0d6b..cd569a87c0a8 100644
> > --- a/include/linux/instrumented.h
> > +++ b/include/linux/instrumented.h
> > @@ -68,7 +68,7 @@ static __always_inline void 
> > instrument_atomic_read(const volatile void *v, size_
> >  {
> >  	kasan_check_read(v, size);
> >  	kcsan_check_atomic_read(v, size);
> > -	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & 
> > (size - 1)));
> > +	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & 
> > (size - 1) & 3));
> >  }
> 
> What is the alignment of stack variables on m68k? E.g. if you have a 
> function with two local variables, would that still be able to trigger 
> the check?
> 
> int f(atomic64_t *a)
> {
>      u16 pad;
>      u64 old;
>      
>      g(&pad);
>      atomic64_try_cmpxchg(a, &old, 0);
> }
> 

I assume so:

int foo(void) {
    short s;
    long long ll;
    return alignof(ll);
}

# Compilation provided by Compiler Explorer at https://godbolt.org/
foo():
        link.w %fp,#0
        moveq #2,%d0
        unlk %fp
        rts

> Since there is nothing telling the compiler that the 'old' argument to 
> atomic*_try_cmpcxchg() needs to be naturally aligned, maybe that check 
> should be changed to only test for the ABI-guaranteed alignment? I think 
> that would still be needed on x86-32.
>  

I don't know why we would check the alignment of the 'old' quantity. It's 
going to be loaded into a register before being used, right?