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.