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

Finn Thain posted 5 patches 4 months ago
There is a newer version of this series
[RFC v3 2/5] bpf: Explicitly align bpf_res_spin_lock
Posted by Finn Thain 4 months ago
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
Posted by Alexei Starovoitov 4 months ago
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
Posted by Peter Zijlstra 4 months ago
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
Posted by Alexei Starovoitov 4 months ago
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
Posted by Finn Thain 4 months ago
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.