Align atomic storage

This series adds the __aligned attribute to atomic_t and atomic64_t definitions in include/asm-generic.

It also adds Kconfig options to enable a new runtime warning to help reveal misaligned atomic accesses on platforms which don't trap that.

This patch series is a Request For Comments because the alignment change is a time/space tradeoff. Its costs and benefits are expected to vary across platforms and workloads. More measurements are needed.

---

Changed since v3:
- Rebased on v6.17.
- New patch to resolve header dependency issue on parisc.
- Dropped documentation patch.

Changed since v2:
- Specify natural alignment for atomic64_t.
- CONFIG_DEBUG_ATOMIC checks for natural alignment again.
- New patch to add weakened alignment check.
- New patch for explicit alignment in BPF header.

---

Finn Thain (4):
bpf: Explicitly align bpf_res_spin_lock
parisc: Drop linux/kernel.h include from asm/bug.h header
atomic: Specify alignment for atomic_t and atomic64_t
atomic: Add option for weaker alignment check

Peter Zijlstra (1):
atomic: Add alignment check to instrumented atomic operations

 arch/parisc/include/asm/bug.h    |  2 --
 include/asm-generic/atomic64.h   |  2 +-
 include/asm-generic/rqspinlock.h |  2 +-
 include/linux/instrumented.h     | 15 +++++++++++++++
 include/linux/types.h            |  2 +-
 kernel/bpf/rqspinlock.c          |  1 -
 lib/Kconfig.debug                | 18 ++++++++++++++++++
 7 files changed, 36 insertions(+), 6 deletions(-)

2.49.1

Align bpf_res_spin_lock to avoid a BUILD_BUG_ON() when the alignment changes, as it will do on m68k when, in a subsequent patch, the minimum alignment of the atomic_t member of struct rqspinlock gets increased from 2 to 4. Drop the BUILD_BUG_ON() as it is now redundant.

---
 include/asm-generic/rqspinlock.h | 2 +-
 kernel/bpf/rqspinlock.c          | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

index 6d4244d643df..eac2dcd31b83 100644
@@ -28,7 +28,7 @@ struct rqspinlock {
  */
 struct bpf_res_spin_lock {
 	u32 val;
-};
+} __aligned(__alignof__(struct rqspinlock));
 
 struct qspinlock;
 #ifdef CONFIG_QUEUED_SPINLOCKS

index a00561b1d3e5..02f1f671e624 100644
@@ -692,7 +692,6 @@ __bpf_kfunc int bpf_res_spin_lock(struct bpf_res_spin_lock *lock)
 	int ret;
 
 	BUILD_BUG_ON(sizeof(rqspinlock_t) != sizeof(struct bpf_res_spin_lock));
-	BUILD_BUG_ON(__alignof__(rqspinlock_t) != __alignof__(struct bpf_res_spin_lock));
 
 	preempt_disable();
 	ret = res_spin_lock((rqspinlock_t *)lock);

2.49.1

This patch series will add WARN_ON_ONCE() calls to the header file linux/instrumented.h. That requires including linux/bug.h, but doing so causes the following compiler error on parisc:

In file included from ./include/linux/atomic/atomic-instrumented.h:17,

                 from ./include/linux/atomic.h:82,
                 from ./arch/parisc/include/asm/bitops.h:13,
                 from ./include/linux/bitops.h:67,
                 from ./include/linux/kernel.h:23,
                 from ./arch/parisc/include/asm/bug.h:5,
                 from ./include/linux/bug.h:5,
                 from ./include/linux/page-flags.h:10,
                 from kernel/bounds.c:10:
./include/linux/instrumented.h: In function 'instrument_atomic_alignment_check':
./include/linux/instrumented.h:69:9: error: implicit declaration of function 'WARN_ON_ONCE' [-Werror=implicit-function-declaration]
   69 |         WARN_ON_ONCE((unsigned long)v & (size - 1));
      |         ^~~~~~~~~~~~

cc1: some warnings being treated as errors
make[3]: *** [scripts/Makefile.build:182: kernel/bounds.s] Error 1

The problem is, asm/bug.h indirectly includes atomic-instrumented.h, which means a new cycle appeared in the graph of #includes. And because some headers in the cycle can't see all definitions, WARN_ON_ONCE() appears to be an undeclared function.

This only happens on parisc and it's easy to fix. In the error message above, linux/kernel.h is included by asm/bug.h. But it's no longer needed there, so remove it.

The comment about needing BUGFLAG_TAINT seems to be incorrect as of commit 19d436268dde ("debug: Add _ONCE() logic to report_bug()"). Also, a comment in linux/kernel.h strongly discourages its use here.

Compile-tested only.

---
 arch/parisc/include/asm/bug.h | 2 --
 1 file changed, 2 deletions(-)

index 833555f74ffa..dbf65623c513 100644
@@ -2,8 +2,6 @@
 #ifndef _PARISC_BUG_H
 #define _PARISC_BUG_H
 
-#include <linux/kernel.h>	/* for BUGFLAG_TAINT */
-
 /*
  * Tell the user there is some problem.
  * The offending file and line are encoded in the __bug_table section.

2.49.1

Thanks!
Helge

Some recent commits incorrectly assumed 4-byte alignment of locks. That assumption fails on Linux/m68k (and, interestingly, would have failed on Linux/cris also). Specify the minimum alignment of atomic variables for fewer surprises and (hopefully) better performance.

On an m68k system with 14 MB of RAM, this patch reduces the available memory by a couple of percent. On a 64 MB system, the cost is under 1% but still significant. I don't know whether there is sufficient performance gain to justify the memory cost; it still has to be measured.

---
Changed since v2:
- Specify natural alignment for atomic64_t.
Changed since v1:
- atomic64_t now gets an __aligned attribute too.
- The 'Fixes' tag has been dropped because Lance sent a different fix for commit e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker") that's suitable for -stable.

---
 include/asm-generic/atomic64.h | 2 +-
 include/linux/types.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index 100d24b02e52..f22ccfc0df98 100644
@@ -10,7 +10,7 @@
 #include <linux/types.h>
 
 typedef struct {
-	s64 counter;
+	s64 __aligned(sizeof(s64)) counter;
 } atomic64_t;
 
 #define ATOMIC64_INIT(i)	{ (i) }

index 6dfdb8e8e4c3..a225a518c2c3 100644
@@ -179,7 +179,7 @@ typedef phys_addr_t resource_size_t;
 typedef unsigned long irq_hw_number_t;
 
 typedef struct {
-	int counter;
+	int __aligned(sizeof(int)) counter;
 } atomic_t;
 
 #define ATOMIC_INIT(i) { (i) }

2.49.1

Hi Finn,

FWIW I implemented jump labels for m68k and I think there is a problem with this in there too.
jump_label_init() calls static_key_set_entries() and setting key->entries in there is corrupting 'atomic_t enabled' at the start of key.

With this patch the problem goes away.

Cheers,

Daniel

That's interesting. I wonder whether the alignment requirements of machine instructions permitted the "appropriation" of the low bits from those pointers...

In anycase, a modified jump label algorithm that did not use/abuse pointer bits would need to execute as fast as the existing implementation. And that might be quite difficult (especially a portable algorithm).

Recently I had an opportunity to do some performance measurements on m68k for this atomic_t alignment patch. I tested some kernel stressors on an AWS 95 (33 MHz 68040, 128 MB RAM, 512 KiB L2$) and also on a Mac IIfx (40 MHz 68030, 80 MB RAM, 32 KiB L2$).

The patch makes the kernel faster or slower, depending the workload. For example, the fifo, futex and shm stressors were consistently faster whereas the splice, signal and msg stressors were consistently slower.

There are no hardware counters for cache misses that might account for part of the slowdown. OTOH, alignment also reduces instances of locks split across page boundaries, which might account for the speed-up. (I didn't look at VM performance counters.)

Finally, I should note that the stress-ng man page says "do NOT use" as a benchmark. OK, well, if anyone wishes to reproduce my results, I can send you the statically linked binary I used. The job file is attached.

I wonder whether others have done any throughput measurement for this patch, using their favourite workloads?

run sequential
metrics-brief
timeout 180s
no-rand-seed
oomable
temp-path /tmp

clone 1
clone-ops 4

dentry 1
dentry-ops 8192

#dev 1
#dev-ops 300

dev-shm 1
dev-shm-ops 20

dnotify 1
dnotify-ops 1200

fault 1
fault-ops 8000

fifo 1
fifo-ops 24000

file-ioctl 1
file-ioctl-ops 20000

futex 1
futex-ops 40000

get 1
get-ops 3000

getdent 1
getdent-ops 10000

icmp-flood 1
icmp-flood-ops 40000

inotify 1
inotify-ops 400

ioprio 1
ioprio-ops 8000

kill 1
kill-ops 150000

memfd 1
memfd-bytes 32m
memfd-ops 8

mmapfork 1
mmapfork-ops 4

msg 1
msg-ops 300000

nop 1
nop-ops 3000

poll 1
poll-ops 8000

ptrace 1
ptrace-ops 50000

pty 1
pty-ops 2

rawpkt 1
rawpkt-ops 80000

rawudp 1
rawudp-ops 15000

resources 1
resources-ops 300

revio 1
revio-ops 50000

seek 1
seek-ops 12000

#sem 1
#sem-ops 4000

sem-sysv 1
sem-sysv-ops 300000

sendfile 1
sendfile-ops 1500

set 1
set-ops 20000

shm 1
shm-ops 15

sigchld 1
sigchld-ops 5000

signal 1
signal-ops 150000

sigsegv 1
sigsegv-ops 100000

sock 1
sock-ops 50

splice 1
splice-ops 10000

tee 1
tee-ops 1500

udp 1
udp-ops 30000

utime 1
utime-ops 4000

vm 1
vm-ops 2500

From: Peter Zijlstra <[email protected]>

Add a Kconfig option for debug builds which logs a warning when an instrumented atomic operation takes place that's misaligned. Some platforms don't trap for this.

---
Changed since v2:
- Always check for natural alignment.
---
To make this useful on those architectures that don't naturally align scalars (x86-32, m68k and sh come to mind), please also use "[PATCH] atomic: skip alignment check for try_cmpxchg() old arg"
https://lore.kernel.org/all/[email protected]/

---
 include/linux/instrumented.h |  4 ++++
 lib/Kconfig.debug            | 10 ++++++++++
 2 files changed, 14 insertions(+)

index 711a1f0d1a73..402a999a0d6b 100644
@@ -7,6 +7,7 @@
 #ifndef _LINUX_INSTRUMENTED_H
 #define _LINUX_INSTRUMENTED_H
 
+#include <linux/bug.h>
 #include <linux/compiler.h>
 #include <linux/kasan-checks.h>
 #include <linux/kcsan-checks.h>

@@ -67,6 +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)));
 }
 
 /**

@@ -81,6 +83,7 @@ static __always_inline void instrument_atomic_write(const volatile void *v, size
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_write(v, size);
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
 }
 
 /**

@@ -95,6 +98,7 @@ static __always_inline void instrument_atomic_read_write(const volatile void *v,
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_read_write(v, size);
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
 }
 
 /**

index dc0e0c6ed075..1c7e30cdfe04 100644
@@ -1363,6 +1363,16 @@ config DEBUG_PREEMPT
 	  depending on workload as it triggers debugging routines for each
 	  this_cpu operation. It should only be used for debugging purposes.
 
+config DEBUG_ATOMIC
+	bool "Debug atomic variables"
+	depends on DEBUG_KERNEL
+	help
+	  If you say Y here then the kernel will add a runtime alignment check
+	  to atomic accesses. Useful for architectures that do not have trap on
+	  mis-aligned access.
+
+	  This option has potentially significant overhead.
+
 menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config LOCK_DEBUGGING_SUPPORT

2.49.1

Add a new Kconfig symbol to make CONFIG_DEBUG_ATOMIC more useful on those architectures which do not align dynamic allocations to 8-byte boundaries. Without this, CONFIG_DEBUG_ATOMIC produces excessive WARN splats.
---
Changed since v3:
- Dropped #include <linux/cache.h> to avoid a build failure on arm64.
- Rewrote Kconfig help text to better describe preferred alignment.
- Refactored to avoid line-wrapping and duplication.

---
 include/linux/instrumented.h | 17 ++++++++++++++---
 lib/Kconfig.debug            |  8 ++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

index 402a999a0d6b..ca946c5860be 100644
@@ -56,6 +56,17 @@ static __always_inline void instrument_read_write(const volatile void *v, size_t
 	kcsan_check_read_write(v, size);
 }
 
+static __always_inline void instrument_atomic_check_alignment(const volatile void *v, size_t size)
+{
+	if (IS_ENABLED(CONFIG_DEBUG_ATOMIC)) {
+		unsigned int mask = size - 1;
+
+		if (IS_ENABLED(CONFIG_DEBUG_ATOMIC_LARGEST_ALIGN))
+			mask &= sizeof(struct { long x; } __aligned_largest) - 1;
+		WARN_ON_ONCE((unsigned long)v & mask);
+	}
+}
+
 /**
  * instrument_atomic_read - instrument atomic read access
  * @v: address of access

@@ -68,7 +79,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)));
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

@@ -83,7 +94,7 @@ static __always_inline void instrument_atomic_write(const volatile void *v, size
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_write(v, size);
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

@@ -98,7 +109,7 @@ static __always_inline void instrument_atomic_read_write(const volatile void *v,
 {
 	kasan_check_write(v, size);
 	kcsan_check_atomic_read_write(v, size);
-	WARN_ON_ONCE(IS_ENABLED(CONFIG_DEBUG_ATOMIC) && ((unsigned long)v & (size - 1)));
+	instrument_atomic_check_alignment(v, size);
 }
 
 /**

index 1c7e30cdfe04..4d3ebe501d17 100644
@@ -1373,6 +1373,14 @@ config DEBUG_ATOMIC
 
 	  This option has potentially significant overhead.
 
+config DEBUG_ATOMIC_LARGEST_ALIGN
+	bool "Check alignment only up to __aligned_largest"
+	depends on DEBUG_ATOMIC
+	help
+	  If you say Y here then the check for natural alignment of
+	  atomic accesses will be constrained to the compiler's largest
+	  alignment for scalar types.
+
 menu "Lock Debugging (spinlocks, mutexes, etc...)"
 
 config LOCK_DEBUGGING_SUPPORT

2.49.1