[RFC v3 0/5] Align atomic storage

[RFC v3 0/5] Align atomic storage

[Finn Thain]

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.

Some people might assume scalars are aligned to 4-byte boundaries, while
others might assume natural alignment. Best not to encourage such
assumptions in the documentation.

Moreover, being that locks are performance sensitive, and being that
atomic operations tend to involve further assumptions, there seems to be
room for improvement here.

Pertinent to this discussion are the section "Memory Efficiency" in
Documentation/RCU/Design/Requirements/Requirements.rst
and the section "GUARANTEES" in Documentation/memory-barriers.txt

---
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 BFP header.

---

Finn Thain (4):
  documentation: Discourage alignment assumptions
  bpf: Explicitly align bpf_res_spin_lock
  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

 .../core-api/unaligned-memory-access.rst      |  7 -------
 include/asm-generic/atomic64.h                |  2 +-
 include/asm-generic/rqspinlock.h              |  2 +-
 include/linux/instrumented.h                  | 12 ++++++++++++
 include/linux/types.h                         |  2 +-
 kernel/bpf/rqspinlock.c                       |  1 -
 lib/Kconfig.debug                             | 19 +++++++++++++++++++
 7 files changed, 34 insertions(+), 11 deletions(-)

-- 
2.49.1

[RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Finn Thain]

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.
Drop the BUILD_BUG_ON() as it is now redundant.

Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Eduard Zingerman <eddyz87@gmail.com>
Cc: Song Liu <song@kernel.org>
Cc: Yonghong Song <yonghong.song@linux.dev>
Cc: John Fastabend <john.fastabend@gmail.com>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Stanislav Fomichev <sdf@fomichev.me>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
---
 include/asm-generic/rqspinlock.h | 2 +-
 kernel/bpf/rqspinlock.c          | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
index 6d4244d643df..eac2dcd31b83 100644
--- a/include/asm-generic/rqspinlock.h
+++ b/include/asm-generic/rqspinlock.h
@@ -28,7 +28,7 @@ struct rqspinlock {
  */
 struct bpf_res_spin_lock {
 	u32 val;
-};
+} __aligned(__alignof__(struct rqspinlock));
 
 struct qspinlock;
 #ifdef CONFIG_QUEUED_SPINLOCKS
diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
index 338305c8852c..a88a0e9749d7 100644
--- a/kernel/bpf/rqspinlock.c
+++ b/kernel/bpf/rqspinlock.c
@@ -671,7 +671,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

Re: [RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Alexei Starovoitov]

On Tue, Oct 7, 2025 at 4:50 PM Finn Thain <fthain@linux-m68k.org> wrote:
>
> 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.
> Drop the BUILD_BUG_ON() as it is now redundant.
>
> Cc: Martin KaFai Lau <martin.lau@linux.dev>
> Cc: Eduard Zingerman <eddyz87@gmail.com>
> Cc: Song Liu <song@kernel.org>
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Stanislav Fomichev <sdf@fomichev.me>
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> ---
>  include/asm-generic/rqspinlock.h | 2 +-
>  kernel/bpf/rqspinlock.c          | 1 -
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
> index 6d4244d643df..eac2dcd31b83 100644
> --- a/include/asm-generic/rqspinlock.h
> +++ b/include/asm-generic/rqspinlock.h
> @@ -28,7 +28,7 @@ struct rqspinlock {
>   */
>  struct bpf_res_spin_lock {
>         u32 val;
> -};
> +} __aligned(__alignof__(struct rqspinlock));

I don't follow.
In the next patch you do:

typedef struct {
- int counter;
+ int __aligned(sizeof(int)) counter;
} atomic_t;

so it was 4 and still 4 ?
Are you saying 'int' on m68k is not 4 byte aligned by default,
so you have to force 4 byte align?

>  struct qspinlock;
>  #ifdef CONFIG_QUEUED_SPINLOCKS
> diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
> index 338305c8852c..a88a0e9749d7 100644
> --- a/kernel/bpf/rqspinlock.c
> +++ b/kernel/bpf/rqspinlock.c
> @@ -671,7 +671,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));

Why delete it? Didn't you make them equal in the above hunk?

Re: [RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Finn Thain]

[-- Attachment #1: Type: text/plain, Size: 2391 bytes --]


On Wed, 8 Oct 2025, Alexei Starovoitov wrote:

> On Tue, Oct 7, 2025 at 4:50 PM Finn Thain <fthain@linux-m68k.org> wrote:
> >
> > 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.
> > Drop the BUILD_BUG_ON() as it is now redundant.
> >
> > Cc: Martin KaFai Lau <martin.lau@linux.dev>
> > Cc: Eduard Zingerman <eddyz87@gmail.com>
> > Cc: Song Liu <song@kernel.org>
> > Cc: Yonghong Song <yonghong.song@linux.dev>
> > Cc: John Fastabend <john.fastabend@gmail.com>
> > Cc: KP Singh <kpsingh@kernel.org>
> > Cc: Stanislav Fomichev <sdf@fomichev.me>
> > Cc: Hao Luo <haoluo@google.com>
> > Cc: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  include/asm-generic/rqspinlock.h | 2 +-
> >  kernel/bpf/rqspinlock.c          | 1 -
> >  2 files changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/include/asm-generic/rqspinlock.h b/include/asm-generic/rqspinlock.h
> > index 6d4244d643df..eac2dcd31b83 100644
> > --- a/include/asm-generic/rqspinlock.h
> > +++ b/include/asm-generic/rqspinlock.h
> > @@ -28,7 +28,7 @@ struct rqspinlock {
> >   */
> >  struct bpf_res_spin_lock {
> >         u32 val;
> > -};
> > +} __aligned(__alignof__(struct rqspinlock));
> 
> I don't follow.
> In the next patch you do:
> 
> typedef struct {
> - int counter;
> + int __aligned(sizeof(int)) counter;
> } atomic_t;
> 
> so it was 4 and still 4 ?
> Are you saying 'int' on m68k is not 4 byte aligned by default,
> so you have to force 4 byte align?
> 

Right. __alignof(int) == 2 on m68k.

> >  struct qspinlock;
> >  #ifdef CONFIG_QUEUED_SPINLOCKS
> > diff --git a/kernel/bpf/rqspinlock.c b/kernel/bpf/rqspinlock.c
> > index 338305c8852c..a88a0e9749d7 100644
> > --- a/kernel/bpf/rqspinlock.c
> > +++ b/kernel/bpf/rqspinlock.c
> > @@ -671,7 +671,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));
> 
> Why delete it? Didn't you make them equal in the above hunk?
> 

I deleted it because it's tautological. I think "do not repeat yourself" 
applies here.

Re: [RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Peter Zijlstra]

On Wed, Oct 08, 2025 at 07:10:13PM -0700, Alexei Starovoitov wrote:

> Are you saying 'int' on m68k is not 4 byte aligned by default,
> so you have to force 4 byte align?

This; m68k has u16 alignment, just to keep life interesting I suppose
:-)

Re: [RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Alexei Starovoitov]

On Thu, Oct 9, 2025 at 12:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Oct 08, 2025 at 07:10:13PM -0700, Alexei Starovoitov wrote:
>
> > Are you saying 'int' on m68k is not 4 byte aligned by default,
> > so you have to force 4 byte align?
>
> This; m68k has u16 alignment, just to keep life interesting I suppose
> :-)

It's not "interesting". It adds burden to the rest of the kernel
for this architectural quirk.
Linus put the foot down for big-endian on arm64 and riscv.
We should do the same here.
x86 uses -mcmodel=kernel for 64-bit and -mregparm=3 for 32-bit.
m68k can do the same.
They can adjust the compiler to make 'int' 4 byte aligned under some
compiler flag. The kernel is built standalone, so it doesn't have
to conform to native calling convention or anything else.

Re: [RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Arnd Bergmann]

On Thu, Oct 9, 2025, at 17:17, Alexei Starovoitov wrote:
> On Thu, Oct 9, 2025 at 12:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>
>> On Wed, Oct 08, 2025 at 07:10:13PM -0700, Alexei Starovoitov wrote:
>>
>> > Are you saying 'int' on m68k is not 4 byte aligned by default,
>> > so you have to force 4 byte align?
>>
>> This; m68k has u16 alignment, just to keep life interesting I suppose
>> :-)
>
> It's not "interesting". It adds burden to the rest of the kernel
> for this architectural quirk.
> Linus put the foot down for big-endian on arm64 and riscv.
> We should do the same here.
> x86 uses -mcmodel=kernel for 64-bit and -mregparm=3 for 32-bit.
> m68k can do the same.
> They can adjust the compiler to make 'int' 4 byte aligned under some
> compiler flag. The kernel is built standalone, so it doesn't have
> to conform to native calling convention or anything else.

I agree that building the kernel with -malign-int makes a lot
of sense here, there is even a project to rebuild the entire
user space with the same flag.

However, changing either the kernel or userspace to build with
-malign-int also has its cost, since for ABI compatibility
reasons any include/uapi/*/*.h header that defines a structure
with a misaligned word needs a custom annotation in order to
still define the layout to be the same as before, and the
annotations do complicate the common headers.

See 
https://lore.kernel.org/all/534e8ff8-70cb-4b78-b0b4-f88645bd180a@app.fastmail.com/
for a list of structures that likely need to be annotated,
and the thread around it for more of the nasty details that
make this nontrivial.

       Arnd

Re: [RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock

[Alexei Starovoitov]

On Thu, Oct 9, 2025 at 9:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Thu, Oct 9, 2025, at 17:17, Alexei Starovoitov wrote:
> > On Thu, Oct 9, 2025 at 12:02 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >>
> >> On Wed, Oct 08, 2025 at 07:10:13PM -0700, Alexei Starovoitov wrote:
> >>
> >> > Are you saying 'int' on m68k is not 4 byte aligned by default,
> >> > so you have to force 4 byte align?
> >>
> >> This; m68k has u16 alignment, just to keep life interesting I suppose
> >> :-)
> >
> > It's not "interesting". It adds burden to the rest of the kernel
> > for this architectural quirk.
> > Linus put the foot down for big-endian on arm64 and riscv.
> > We should do the same here.
> > x86 uses -mcmodel=kernel for 64-bit and -mregparm=3 for 32-bit.
> > m68k can do the same.
> > They can adjust the compiler to make 'int' 4 byte aligned under some
> > compiler flag. The kernel is built standalone, so it doesn't have
> > to conform to native calling convention or anything else.
>
> I agree that building the kernel with -malign-int makes a lot
> of sense here, there is even a project to rebuild the entire
> user space with the same flag.
>
> However, changing either the kernel or userspace to build with
> -malign-int also has its cost, since for ABI compatibility
> reasons any include/uapi/*/*.h header that defines a structure
> with a misaligned word needs a custom annotation in order to
> still define the layout to be the same as before, and the
> annotations do complicate the common headers.
>
> See
> https://lore.kernel.org/all/534e8ff8-70cb-4b78-b0b4-f88645bd180a@app.fastmail.com/
> for a list of structures that likely need to be annotated,
> and the thread around it for more of the nasty details that
> make this nontrivial.

I see. So this is a lesser evil.

Acked-by: Alexei Starovoitov <ast@kernel.org>

for the patch then.