Re: [PATCH] atomic: Specify natural alignment for atomic_t

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

On Wed, 27 Aug 2025, Peter Zijlstra wrote:

> On Wed, Aug 27, 2025 at 05:17:19PM +1000, Finn Thain wrote:
> > 
> > On Mon, 25 Aug 2025, Peter Zijlstra wrote:
> > 
> > > On Mon, Aug 25, 2025 at 06:03:23PM +1000, Finn Thain wrote:
> > > > 
> > > > On Mon, 25 Aug 2025, Peter Zijlstra wrote:
> > > > 
> > > > > 
> > > > > And your architecture doesn't trap on unaligned atomic access ?!!?!
> > > > > 
> > > > 
> > > > Right. This port doesn't do SMP.
> > > 
> > > There is RMW_INSN which seems to imply a compare-and-swap instruction of 
> > > sorts. That is happy to work on unaligned storage?
> > > 
> > 
> > Yes, the TAS and CAS instructions are happy to work on unaligned storage. 
> > 
> > However, these operations involve an indivisible bus cycle that hogs the 
> > bus to the detriment of other processors, DMA controllers etc. So I 
> > suspect lock alignment would tend to shorten read-modify-write cycles, and 
> > improve efficiency, when CONFIG_RMW_INSN is enabled.
> > 
> > Most m68k platforms will have CONFIG_RMW_INSN disabled, or else simply 
> > don't implement TAS and CAS. In this case, lock alignment might still 
> > help, just because L1 cache entries are long words. I've not tried to 
> > measure this.
> 
> Fair enough; this sounds a little like the x86 LOCK prefix, it will work
> on unaligned memory, but at tremendous cost (recent chips have an
> optional exception on unaligned).
> 
> Anyway, I'm not opposed to adding an explicit alignment to atomic_t.
> Isn't s32 or __s32 already having this?
> 

For Linux/m68k, __alignof__(__s32) == 2 and __alignof__(s32) == 2.

> But I think it might make sense to have a DEBUG alignment check right
> along with adding that alignment, just to make sure things are indeed /
> stay aligned.
> 

A run-time assertion seems surperfluous as long as other architectures 
already trap for misaligned locks. For m68k, perhaps we could have a 
compile-time check:

--- a/arch/m68k/kernel/setup_mm.c
+++ b/arch/m68k/kernel/setup_mm.c
@@ -371,6 +371,12 @@ void __init setup_arch(char **cmdline_p)
        }
 #endif
 #endif
+
+       /*
+        * 680x0 CPUs don't require aligned storage for atomic ops.
+        * However, alignment assumptions may appear in core kernel code.
+        */
+       BUILD_BUG_ON(__alignof__(atomic_t) < sizeof(atomic_t));
 }

But I'm not sure that arch/m68k is a good place for that kind of thing -- 
my inclination would be to place such compile-time assertions closer to 
the code that rests on that assertion, like in hung_task.c or mutex.c. 
E.g.

--- a/kernel/locking/mutex.c
+++ b/kernel/locking/mutex.c
@@ -54,8 +54,6 @@ __mutex_init(struct mutex *lock, const char *name, 
struct lock_class_key *key)
 #endif
 
        debug_mutex_init(lock, name, key);
+
+       BUILD_BUG_ON(__alignof__(lock->owner) < sizeof(lock->owner));
 }
 EXPORT_SYMBOL(__mutex_init);


Is that the kind of check you had in mind? I'm open to suggestions.