[PATCH] atomic: Specify natural alignment for atomic_t

[PATCH] atomic: Specify natural alignment for atomic_t

[Finn Thain]

Some recent commits incorrectly assumed the natural alignment of locks.
That assumption fails on Linux/m68k (and, interestingly, would have failed
on Linux/cris also). This leads to spurious warnings from the hang check
code. Fix this bug by adding the necessary 'aligned' attribute.

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Lance Yang <lance.yang@linux.dev>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Eero Tamminen <oak@helsinkinet.fi>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Will Deacon <will@kernel.org>
Cc: stable@vger.kernel.org
Reported-by: Eero Tamminen <oak@helsinkinet.fi>
Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker")
Signed-off-by: Finn Thain <fthain@linux-m68k.org>
---
I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
the other architectures naturally align ints already so I'm expecting to
see no effect there.
---
 include/linux/types.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/types.h b/include/linux/types.h
index 6dfdb8e8e4c3..cd5b2b0f4b02 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -179,7 +179,7 @@ typedef phys_addr_t resource_size_t;
 typedef unsigned long irq_hw_number_t;
 
 typedef struct {
-	int counter;
+	int counter __aligned(sizeof(int));
 } atomic_t;
 
 #define ATOMIC_INIT(i) { (i) }
-- 
2.49.1

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

[Lance Yang]

Hi Finn,

Nice work, thanks for your patch!

On 2025/8/25 10:03, Finn Thain wrote:
> Some recent commits incorrectly assumed the natural alignment of locks.
> That assumption fails on Linux/m68k (and, interestingly, would have failed
> on Linux/cris also). This leads to spurious warnings from the hang check
> code. Fix this bug by adding the necessary 'aligned' attribute.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Lance Yang <lance.yang@linux.dev>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Eero Tamminen <oak@helsinkinet.fi>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> Reported-by: Eero Tamminen <oak@helsinkinet.fi>
> Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
> Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker")
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
> the other architectures naturally align ints already so I'm expecting to
> see no effect there.

Yeah, it is the correct approach for the spurious warnings on architectures
like m68k, where the natural alignment of types can be less than 4 bytes.

> ---
>   include/linux/types.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 6dfdb8e8e4c3..cd5b2b0f4b02 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -179,7 +179,7 @@ typedef phys_addr_t resource_size_t;
>   typedef unsigned long irq_hw_number_t;
>   
>   typedef struct {
> -	int counter;
> +	int counter __aligned(sizeof(int));
>   } atomic_t;
>   
>   #define ATOMIC_INIT(i) { (i) }

However, as we've seen from the kernel test robot's report on mt6660_chip,
this won't solve the cases where a lock is forced to be unaligned by
#pragma pack(1). That will still trigger warnings, IIUC.

Perhaps we should also apply the follwoing?

diff --git a/include/linux/hung_task.h b/include/linux/hung_task.h
index 34e615c76ca5..940f8f3558f6 100644
--- a/include/linux/hung_task.h
+++ b/include/linux/hung_task.h
@@ -45,7 +45,7 @@ static inline void hung_task_set_blocker(void *lock, unsigned long type)
 	 * If the lock pointer matches the BLOCKER_TYPE_MASK, return
 	 * without writing anything.
 	 */
-	if (WARN_ON_ONCE(lock_ptr & BLOCKER_TYPE_MASK))
+	if (lock_ptr & BLOCKER_TYPE_MASK)
 		return;

 	WRITE_ONCE(current->blocker, lock_ptr | type);
@@ -53,8 +53,6 @@ static inline void hung_task_set_blocker(void *lock, unsigned long type)

 static inline void hung_task_clear_blocker(void)
 {
-	WARN_ON_ONCE(!READ_ONCE(current->blocker));
-
 	WRITE_ONCE(current->blocker, 0UL);
 }

Let the feature gracefully do nothing on that ;)

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

[Finn Thain]

On Mon, 25 Aug 2025, Lance Yang wrote:

> 
> However, as we've seen from the kernel test robot's report on 
> mt6660_chip, this won't solve the cases where a lock is forced to be 
> unaligned by #pragma pack(1). That will still trigger warnings, IIUC.
> 

I think you've misunderstood the warning that your patch produced. (BTW, I 
have not seen any warnings from my own patch, so far.)

The mistake you made in your patch was to add an alignment attribute to a 
member of a packed struct. That's why I suggested that you should align 
the lock instead.

Is there some problem with my approach?

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

[Finn Thain]

On Mon, 25 Aug 2025, Lance Yang wrote:

> 
> Perhaps we should also apply the follwoing?
> 
> diff --git a/include/linux/hung_task.h b/include/linux/hung_task.h
> index 34e615c76ca5..940f8f3558f6 100644
> --- a/include/linux/hung_task.h
> +++ b/include/linux/hung_task.h
> @@ -45,7 +45,7 @@ static inline void hung_task_set_blocker(void *lock, unsigned long type)
>  	 * If the lock pointer matches the BLOCKER_TYPE_MASK, return
>  	 * without writing anything.
>  	 */
> -	if (WARN_ON_ONCE(lock_ptr & BLOCKER_TYPE_MASK))
> +	if (lock_ptr & BLOCKER_TYPE_MASK)
>  		return;
> 
>  	WRITE_ONCE(current->blocker, lock_ptr | type);
> @@ -53,8 +53,6 @@ static inline void hung_task_set_blocker(void *lock, unsigned long type)
> 
>  static inline void hung_task_clear_blocker(void)
>  {
> -	WARN_ON_ONCE(!READ_ONCE(current->blocker));
> -
>  	WRITE_ONCE(current->blocker, 0UL);
>  }
> 
> Let the feature gracefully do nothing on that ;)
> 

This is poor style indeed.

The conditional you've added to the hung task code has no real relevance 
to hung tasks. It doesn't belong there.

Of course, nobody wants that sort of logic to get duplicated at each site 
affected by the architectural quirk in question. Try to imagine if the 
whole kernel followed your example, and such unrelated conditionals were 
scattered across code base for a few decades. Now imagine trying to work 
on that code.

You can see special cases for architectural quirks in drivers, but we do 
try to avoid them. And this is not a driver.

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

[Lance Yang]

On 2025/8/25 11:59, Finn Thain wrote:
> 
> On Mon, 25 Aug 2025, Lance Yang wrote:
> 
>>
>> However, as we've seen from the kernel test robot's report on
>> mt6660_chip, this won't solve the cases where a lock is forced to be
>> unaligned by #pragma pack(1). That will still trigger warnings, IIUC.
>>
> 
> I think you've misunderstood the warning that your patch produced. (BTW, I
> have not seen any warnings from my own patch, so far.)
> 
> The mistake you made in your patch was to add an alignment attribute to a
> member of a packed struct. That's why I suggested that you should align
> the lock instead.

Apologies for the confusion. I was referring to the runtime warning from
WARN_ON_ONCE, not a compile-time warning.

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

[Lance Yang]

On 2025/8/25 12:07, Finn Thain wrote:
> 
> On Mon, 25 Aug 2025, Lance Yang wrote:
> 
>>
>> Perhaps we should also apply the follwoing?
>>
>> diff --git a/include/linux/hung_task.h b/include/linux/hung_task.h
>> index 34e615c76ca5..940f8f3558f6 100644
>> --- a/include/linux/hung_task.h
>> +++ b/include/linux/hung_task.h
>> @@ -45,7 +45,7 @@ static inline void hung_task_set_blocker(void *lock, unsigned long type)
>>   	 * If the lock pointer matches the BLOCKER_TYPE_MASK, return
>>   	 * without writing anything.
>>   	 */
>> -	if (WARN_ON_ONCE(lock_ptr & BLOCKER_TYPE_MASK))
>> +	if (lock_ptr & BLOCKER_TYPE_MASK)
>>   		return;
>>
>>   	WRITE_ONCE(current->blocker, lock_ptr | type);
>> @@ -53,8 +53,6 @@ static inline void hung_task_set_blocker(void *lock, unsigned long type)
>>
>>   static inline void hung_task_clear_blocker(void)
>>   {
>> -	WARN_ON_ONCE(!READ_ONCE(current->blocker));
>> -
>>   	WRITE_ONCE(current->blocker, 0UL);
>>   }
>>
>> Let the feature gracefully do nothing on that ;)
>>
> 
> This is poor style indeed.

Thanks for the lesson!

> 
> The conditional you've added to the hung task code has no real relevance
> to hung tasks. It doesn't belong there.

You're right! The original pointer-encoding was a deliberate trade-off to
save a field in task_struct, but as we're seeing now, that assumption is
fragile and causing issues :(

> 
> Of course, nobody wants that sort of logic to get duplicated at each site
> affected by the architectural quirk in question. Try to imagine if the
> whole kernel followed your example, and such unrelated conditionals were
> scattered across code base for a few decades. Now imagine trying to work
> on that code.

I agree with you completely: scattering more alignment checks into core 
logic
isn't the right long-term solution. It's not a clean design :(

> 
> You can see special cases for architectural quirks in drivers, but we do
> try to avoid them. And this is not a driver.

So, how about this?

What if we squash the runtime check fix into your patch? That would create a
single, complete fix that can be cleanly backported to stop all the spurious
warnings at once.

Then, as a follow-up, we can work on the proper long-term solution: changing
the pointer-encoding and re-introducing a dedicated field for the 
blocker type.

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

[Finn Thain]

On Mon, 25 Aug 2025, Lance Yang wrote:

> 
> What if we squash the runtime check fix into your patch? 

Did my patch not solve the problem?

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

[Peter Zijlstra]

On Mon, Aug 25, 2025 at 12:03:05PM +1000, Finn Thain wrote:
> Some recent commits incorrectly assumed the natural alignment of locks.
> That assumption fails on Linux/m68k (and, interestingly, would have failed
> on Linux/cris also). This leads to spurious warnings from the hang check
> code. Fix this bug by adding the necessary 'aligned' attribute.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Lance Yang <lance.yang@linux.dev>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Eero Tamminen <oak@helsinkinet.fi>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> Reported-by: Eero Tamminen <oak@helsinkinet.fi>
> Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
> Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker")

I don't see how this patch it at all relevant. Let along how its fixed
by the below.

> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
> the other architectures naturally align ints already so I'm expecting to
> see no effect there.
> ---
>  include/linux/types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 6dfdb8e8e4c3..cd5b2b0f4b02 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -179,7 +179,7 @@ typedef phys_addr_t resource_size_t;
>  typedef unsigned long irq_hw_number_t;
>  
>  typedef struct {
> -	int counter;
> +	int counter __aligned(sizeof(int));
>  } atomic_t;
>  
>  #define ATOMIC_INIT(i) { (i) }

And your architecture doesn't trap on unaligned atomic access ?!!?!

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

[Lance Yang]

On 2025/8/25 14:17, Finn Thain wrote:
> 
> On Mon, 25 Aug 2025, Lance Yang wrote:
> 
>>
>> What if we squash the runtime check fix into your patch?
> 
> Did my patch not solve the problem?

Hmm... it should solve the problem for natural alignment, which is a
critical fix.

But it cannot solve the problem of forced misalignment from drivers using
#pragma pack(1). The runtime warning will still trigger in those cases.

I built a simple test module on a kernel with your patch applied:

```
#include <linux/module.h>
#include <linux/init.h>

struct __attribute__((packed)) test_container {
     char padding[49];
     struct mutex io_lock;
};

static int __init alignment_init(void)
{
     struct test_container cont;
     pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
long)&cont.io_lock % 4);
     return 0;
}

static void __exit alignment_exit(void)
{
     pr_info("Module unloaded\n");
}

module_init(alignment_init);
module_exit(alignment_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("x");
MODULE_DESCRIPTION("x");
```

Result from dmesg:
[Mon Aug 25 15:44:50 2025] io_lock address offset mod 4: 1

As we can see, a packed struct can still force the entire mutex object
to an unaligned address. With an address like this, the WARN_ON_ONCE
can still be triggered.

That's why I proposed squashing the runtime check fix into your patch.
Then it can be cleanly backported to stop all the spurious warnings at
once.

I hope this clarifies things.

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

[Finn Thain]

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.

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

[Finn Thain]

[Belated Cc linux-m68k...]

On Mon, 25 Aug 2025, Lance Yang wrote:

> 
> On 2025/8/25 14:17, Finn Thain wrote:
> > 
> > On Mon, 25 Aug 2025, Lance Yang wrote:
> > 
> >>
> >> What if we squash the runtime check fix into your patch?
> > 
> > Did my patch not solve the problem?
> 
> Hmm... it should solve the problem for natural alignment, which is a 
> critical fix.
> 
> But it cannot solve the problem of forced misalignment from drivers 
> using #pragma pack(1). The runtime warning will still trigger in those 
> cases.
> 
> I built a simple test module on a kernel with your patch applied:
> 
> ```
> #include <linux/module.h>
> #include <linux/init.h>
> 
> struct __attribute__((packed)) test_container {
>     char padding[49];
>     struct mutex io_lock;
> };
> 
> static int __init alignment_init(void)
> {
>     struct test_container cont;
>     pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
>     return 0;
> }
> 
> static void __exit alignment_exit(void)
> {
>     pr_info("Module unloaded\n");
> }
> 
> module_init(alignment_init);
> module_exit(alignment_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("x");
> MODULE_DESCRIPTION("x");
> ```
> 
> Result from dmesg:
> [Mon Aug 25 15:44:50 2025] io_lock address offset mod 4: 1
> 

Thanks for sending code to illustrate your point. Unfortunately, I was not 
able to reproduce your results. Tested on x86, your test module shows no 
misalignment:

[131840.349042] io_lock address offset mod 4: 0

Tested on m68k I also get 0, given the patch at the top of this thread:

[    0.400000] io_lock address offset mod 4: 0

> 
> As we can see, a packed struct can still force the entire mutex object 
> to an unaligned address. With an address like this, the WARN_ON_ONCE can 
> still be triggered.

I don't think so. But there is something unexpected going on here -- the 
output from pahole appears to say io_lock is at offset 49, which seems to 
contradict the printk() output above.

struct test_container {
        char                       padding[49];          /*     0    49 */
        struct mutex               io_lock __attribute__((__aligned__(1))); /*    49    12 */

        /* size: 61, cachelines: 1, members: 2 */
        /* forced alignments: 1 */
        /* last cacheline: 61 bytes */
} __attribute__((__packed__));

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

[Lance Yang]

Thanks for digging deeper!

On 2025/8/25 18:49, Finn Thain wrote:
> 
> [Belated Cc linux-m68k...]
> 
> On Mon, 25 Aug 2025, Lance Yang wrote:
> 
>>
>> On 2025/8/25 14:17, Finn Thain wrote:
>>>
>>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>>
>>>>
>>>> What if we squash the runtime check fix into your patch?
>>>
>>> Did my patch not solve the problem?
>>
>> Hmm... it should solve the problem for natural alignment, which is a
>> critical fix.
>>
>> But it cannot solve the problem of forced misalignment from drivers
>> using #pragma pack(1). The runtime warning will still trigger in those
>> cases.
>>
>> I built a simple test module on a kernel with your patch applied:
>>
>> ```
>> #include <linux/module.h>
>> #include <linux/init.h>
>>
>> struct __attribute__((packed)) test_container {
>>      char padding[49];
>>      struct mutex io_lock;
>> };
>>
>> static int __init alignment_init(void)
>> {
>>      struct test_container cont;
>>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
>>      return 0;
>> }
>>
>> static void __exit alignment_exit(void)
>> {
>>      pr_info("Module unloaded\n");
>> }
>>
>> module_init(alignment_init);
>> module_exit(alignment_exit);
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("x");
>> MODULE_DESCRIPTION("x");
>> ```
>>
>> Result from dmesg:
>> [Mon Aug 25 15:44:50 2025] io_lock address offset mod 4: 1
>>
> 
> Thanks for sending code to illustrate your point. Unfortunately, I was not
> able to reproduce your results. Tested on x86, your test module shows no
> misalignment:
> 
> [131840.349042] io_lock address offset mod 4: 0
> 
> Tested on m68k I also get 0, given the patch at the top of this thread:
> 
> [    0.400000] io_lock address offset mod 4: 0
> 
>>
>> As we can see, a packed struct can still force the entire mutex object
>> to an unaligned address. With an address like this, the WARN_ON_ONCE can
>> still be triggered.

> 
> I don't think so. But there is something unexpected going on here -- the
> output from pahole appears to say io_lock is at offset 49, which seems to
> contradict the printk() output above.

Interesting! That contradiction is the key. It seems we're seeing different
compiler behaviors.

I'm on GCC 14.2.0 (Debian 14.2.0-19), and it appears to be strictly 
respecting
the packed attribute.

So let's print something more:

```
#include <linux/module.h>
#include <linux/init.h>

struct __attribute__((packed)) test_container {
     char padding[49];
     struct mutex io_lock;
};

static int __init alignment_init(void)
{
     struct test_container cont;
     pr_info("Container base address      : %px\n", &cont);
     pr_info("io_lock member address      : %px\n", &cont.io_lock);
     pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
long)&cont.io_lock % 4);
     return 0;
}

static void __exit alignment_exit(void)
{
     pr_info("Module unloaded\n");
}

module_init(alignment_init);
module_exit(alignment_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("x");
MODULE_DESCRIPTION("x");
```

Result from dmesg:

```
[Mon Aug 25 19:15:33 2025] Container base address      : ff1100063570f840
[Mon Aug 25 19:15:33 2025] io_lock member address      : ff1100063570f871
[Mon Aug 25 19:15:33 2025] io_lock address offset mod 4: 1
```

io_lock is exactly base + 49, resulting in misalignment.

Seems like your compiler is cleverly re-aligning the whole struct on the
stack, but we can't rely on that behavior, as it's not guaranteed across
all compilers or versions. wdyt?

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

[Lance Yang]

On 2025/8/25 19:19, Lance Yang wrote:
> Thanks for digging deeper!
> 
> On 2025/8/25 18:49, Finn Thain wrote:
>>
>> [Belated Cc linux-m68k...]
>>
>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>
>>>
>>> On 2025/8/25 14:17, Finn Thain wrote:
>>>>
>>>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>>>
>>>>>
>>>>> What if we squash the runtime check fix into your patch?
>>>>
>>>> Did my patch not solve the problem?
>>>
>>> Hmm... it should solve the problem for natural alignment, which is a
>>> critical fix.
>>>
>>> But it cannot solve the problem of forced misalignment from drivers
>>> using #pragma pack(1). The runtime warning will still trigger in those
>>> cases.
>>>
>>> I built a simple test module on a kernel with your patch applied:
>>>
>>> ```
>>> #include <linux/module.h>
>>> #include <linux/init.h>
>>>
>>> struct __attribute__((packed)) test_container {
>>>      char padding[49];
>>>      struct mutex io_lock;
>>> };
>>>
>>> static int __init alignment_init(void)
>>> {
>>>      struct test_container cont;
>>>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
>>> long)&cont.io_lock % 4);
>>>      return 0;
>>> }
>>>
>>> static void __exit alignment_exit(void)
>>> {
>>>      pr_info("Module unloaded\n");
>>> }
>>>
>>> module_init(alignment_init);
>>> module_exit(alignment_exit);
>>> MODULE_LICENSE("GPL");
>>> MODULE_AUTHOR("x");
>>> MODULE_DESCRIPTION("x");
>>> ```
>>>
>>> Result from dmesg:
>>> [Mon Aug 25 15:44:50 2025] io_lock address offset mod 4: 1
>>>
>>
>> Thanks for sending code to illustrate your point. Unfortunately, I was 
>> not
>> able to reproduce your results. Tested on x86, your test module shows no
>> misalignment:
>>
>> [131840.349042] io_lock address offset mod 4: 0
>>
>> Tested on m68k I also get 0, given the patch at the top of this thread:
>>
>> [    0.400000] io_lock address offset mod 4: 0
>>
>>>
>>> As we can see, a packed struct can still force the entire mutex object
>>> to an unaligned address. With an address like this, the WARN_ON_ONCE can
>>> still be triggered.
> 
>>
>> I don't think so. But there is something unexpected going on here -- the
>> output from pahole appears to say io_lock is at offset 49, which seems to
>> contradict the printk() output above.
> 
> Interesting! That contradiction is the key. It seems we're seeing different
> compiler behaviors.
> 
> I'm on GCC 14.2.0 (Debian 14.2.0-19), and it appears to be strictly 
> respecting
> the packed attribute.
> 
> So let's print something more:
> 
> ```
> #include <linux/module.h>
> #include <linux/init.h>
> 
> struct __attribute__((packed)) test_container {
>      char padding[49];
>      struct mutex io_lock;
> };
> 
> static int __init alignment_init(void)
> {
>      struct test_container cont;
>      pr_info("Container base address      : %px\n", &cont);
>      pr_info("io_lock member address      : %px\n", &cont.io_lock);
>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
> long)&cont.io_lock % 4);
>      return 0;
> }
> 
> static void __exit alignment_exit(void)
> {
>      pr_info("Module unloaded\n");
> }
> 
> module_init(alignment_init);
> module_exit(alignment_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("x");
> MODULE_DESCRIPTION("x");
> ```
> 
> Result from dmesg:
> 
> ```
> [Mon Aug 25 19:15:33 2025] Container base address      : ff1100063570f840
> [Mon Aug 25 19:15:33 2025] io_lock member address      : ff1100063570f871
> [Mon Aug 25 19:15:33 2025] io_lock address offset mod 4: 1
> ```
> 
> io_lock is exactly base + 49, resulting in misalignment.
> 
> Seems like your compiler is cleverly re-aligning the whole struct on the
> stack, but we can't rely on that behavior, as it's not guaranteed across
> all compilers or versions. wdyt?


Same here, using a global static variable instead of a local one. The result
is consistently misaligned.

```
#include <linux/module.h>
#include <linux/init.h>

static struct __attribute__((packed)) test_container {
     char padding[49];
     struct mutex io_lock;
} cont;

static int __init alignment_init(void)
{
     pr_info("Container base address      : %px\n", &cont);
     pr_info("io_lock member address      : %px\n", &cont.io_lock);
     pr_info("io_lock address offset mod 4: %lu\n", (unsigned 
long)&cont.io_lock % 4);
     return 0;
}

static void __exit alignment_exit(void)
{
     pr_info("Module unloaded\n");
}

module_init(alignment_init);
module_exit(alignment_exit);
MODULE_LICENSE("GPL");
MODULE_AUTHOR("x");
MODULE_DESCRIPTION("x");
```

Result from dmesg:

```
[Mon Aug 25 19:33:28 2025] Container base address      : ffffffffc28f0940
[Mon Aug 25 19:33:28 2025] io_lock member address      : ffffffffc28f0971
[Mon Aug 25 19:33:28 2025] io_lock address offset mod 4: 1
```

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

[Peter Zijlstra]

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?

Anyway, it might make sense to add an alignment check to
arch/m68k/include/asm/atomic.h somewhere, perhaps dependent on
some DEBUG flag or other.

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

[David Laight]

On Mon, 25 Aug 2025 15:46:42 +0800
Lance Yang <lance.yang@linux.dev> wrote:

> On 2025/8/25 14:17, Finn Thain wrote:
> > 
> > On Mon, 25 Aug 2025, Lance Yang wrote:
> >   
> >>
> >> What if we squash the runtime check fix into your patch?  
> > 
> > Did my patch not solve the problem?  
> 
> Hmm... it should solve the problem for natural alignment, which is a
> critical fix.
> 
> But it cannot solve the problem of forced misalignment from drivers using
> #pragma pack(1). The runtime warning will still trigger in those cases.
> 
> I built a simple test module on a kernel with your patch applied:
> 
> ```
> #include <linux/module.h>
> #include <linux/init.h>
> 
> struct __attribute__((packed)) test_container {
>      char padding[49];
>      struct mutex io_lock;
> };
> 
> static int __init alignment_init(void)
> {
>      struct test_container cont;
>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);

Doesn't that give a compilation warning from 'taking the address of a packed member'?
Ignore that at your peril.

More problematic is that, IIRC, m68k kmalloc() allocates 16bit aligned memory.
This has broken other things in the past.
I doubt that increasing the alignment to 32bits would make much difference
to the kernel memory footprint.

	David


>      return 0;
> }
> 
> static void __exit alignment_exit(void)
> {
>      pr_info("Module unloaded\n");
> }
> 
> module_init(alignment_init);
> module_exit(alignment_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("x");
> MODULE_DESCRIPTION("x");
> ```
> 
> Result from dmesg:
> [Mon Aug 25 15:44:50 2025] io_lock address offset mod 4: 1
> 
> As we can see, a packed struct can still force the entire mutex object
> to an unaligned address. With an address like this, the WARN_ON_ONCE
> can still be triggered.
> 
> That's why I proposed squashing the runtime check fix into your patch.
> Then it can be cleanly backported to stop all the spurious warnings at
> once.
> 
> I hope this clarifies things.
> 
> 

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

[Lance Yang]

On 2025/8/25 20:07, David Laight wrote:
> On Mon, 25 Aug 2025 15:46:42 +0800
> Lance Yang <lance.yang@linux.dev> wrote:
> 
>> On 2025/8/25 14:17, Finn Thain wrote:
>>>
>>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>>    
>>>>
>>>> What if we squash the runtime check fix into your patch?
>>>
>>> Did my patch not solve the problem?
>>
>> Hmm... it should solve the problem for natural alignment, which is a
>> critical fix.
>>
>> But it cannot solve the problem of forced misalignment from drivers using
>> #pragma pack(1). The runtime warning will still trigger in those cases.
>>
>> I built a simple test module on a kernel with your patch applied:
>>
>> ```
>> #include <linux/module.h>
>> #include <linux/init.h>
>>
>> struct __attribute__((packed)) test_container {
>>       char padding[49];
>>       struct mutex io_lock;
>> };
>>
>> static int __init alignment_init(void)
>> {
>>       struct test_container cont;
>>       pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
> 
> Doesn't that give a compilation warning from 'taking the address of a packed member'?
> Ignore that at your peril.

Hmm, I don't see that acctully ...

> 
> More problematic is that, IIRC, m68k kmalloc() allocates 16bit aligned memory.
> This has broken other things in the past.
> I doubt that increasing the alignment to 32bits would make much difference
> to the kernel memory footprint.

@Finn Given this new information, how about we just apply the runtime check
fix for now?

Since we plan to remove the entire pointer-encoding scheme later anyway, a
minimal and targeted change could be the logical choice. It's easy and safe
to backport, and it cleanly stops the warnings from all sources without
introducing new risks - exactly what we need for stable kernels.

Cheers,
Lance

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

[Eero Tamminen]

Hi Finn & Lance,

On 25.8.2025 5.03, Finn Thain wrote:
> Some recent commits incorrectly assumed the natural alignment of locks.
> That assumption fails on Linux/m68k (and, interestingly, would have failed
> on Linux/cris also). This leads to spurious warnings from the hang check
> code. Fix this bug by adding the necessary 'aligned' attribute.
[...]
> Reported-by: Eero Tamminen <oak@helsinkinet.fi>
> Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
> Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker")
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> ---
> I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
> the other architectures naturally align ints already so I'm expecting to
> see no effect there.

Yes, it fixes both of the issues (warnings & broken console):
	Tested-by: Eero Tamminen <oak@helsinkinet.fi>

(Emulated Atari Falcon) boot up performance with this is within normal 
variation.


On 23.8.2025 10.49, Lance Yang wrote:
 > Anyway, I've prepared two patches for discussion, either of which should
 > fix the alignment issue :)
 >
 > Patch A[1] adjusts the runtime checks to handle unaligned pointers.
 > Patch B[2] enforces 4-byte alignment on the core lock structures.
 >
 > Both tested on x86-64.
 >
 > [1] 
https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
 > [2] https://lore.kernel.org/lkml/20250823074048.92498-1-
 > lance.yang@linux.dev

Same goes for both of these, except that removing warnings makes minimal 
kernel boot 1-2% faster than 4-aligning the whole struct.


	- Eero

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

[Lance Yang]

Hi Eero,

On 2025/8/26 23:22, Eero Tamminen wrote:
> Hi Finn & Lance,
> 
> On 25.8.2025 5.03, Finn Thain wrote:
>> Some recent commits incorrectly assumed the natural alignment of locks.
>> That assumption fails on Linux/m68k (and, interestingly, would have 
>> failed
>> on Linux/cris also). This leads to spurious warnings from the hang check
>> code. Fix this bug by adding the necessary 'aligned' attribute.
> [...]
>> Reported-by: Eero Tamminen <oak@helsinkinet.fi>
>> Closes: https://lore.kernel.org/lkml/ 
>> CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
>> Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded 
>> blocker")
>> Signed-off-by: Finn Thain <fthain@linux-m68k.org>
>> ---
>> I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
>> the other architectures naturally align ints already so I'm expecting to
>> see no effect there.
> 
> Yes, it fixes both of the issues (warnings & broken console):
>      Tested-by: Eero Tamminen <oak@helsinkinet.fi>
> 
> (Emulated Atari Falcon) boot up performance with this is within normal 
> variation.
> 
> 
> On 23.8.2025 10.49, Lance Yang wrote:
>  > Anyway, I've prepared two patches for discussion, either of which should
>  > fix the alignment issue :)
>  >
>  > Patch A[1] adjusts the runtime checks to handle unaligned pointers.
>  > Patch B[2] enforces 4-byte alignment on the core lock structures.
>  >
>  > Both tested on x86-64.
>  >
>  > [1] https://lore.kernel.org/lkml/20250823050036.7748-1- 
> lance.yang@linux.dev
>  > [2] https://lore.kernel.org/lkml/20250823074048.92498-1-
>  > lance.yang@linux.dev
> 
> Same goes for both of these, except that removing warnings makes minimal 
> kernel boot 1-2% faster than 4-aligning the whole struct.

Thanks a lot for testing!

Cheers,
Lance

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

[Masami Hiramatsu]

On Mon, 25 Aug 2025 12:03:05 +1000
Finn Thain <fthain@linux-m68k.org> wrote:

> Some recent commits incorrectly assumed the natural alignment of locks.
> That assumption fails on Linux/m68k (and, interestingly, would have failed
> on Linux/cris also). This leads to spurious warnings from the hang check
> code. Fix this bug by adding the necessary 'aligned' attribute.
> 
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Lance Yang <lance.yang@linux.dev>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>
> Cc: Eero Tamminen <oak@helsinkinet.fi>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Will Deacon <will@kernel.org>
> Cc: stable@vger.kernel.org
> Reported-by: Eero Tamminen <oak@helsinkinet.fi>
> Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
> Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker")
> Signed-off-by: Finn Thain <fthain@linux-m68k.org>

This seems good anyway because unaligned atomic memory access
sounds insane. So looks good to me.

Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

Anyway, Lance's patch[1] is still needed. We'd better gracefully
ignore if the blocker is not aligned, because hung_task blocker
detection is an optional for debugging and not necessary for
the kernel operation.

[1] https://lore.kernel.org/all/20250823050036.7748-1-lance.yang@linux.dev/ 


Thank you,

> ---
> I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
> the other architectures naturally align ints already so I'm expecting to
> see no effect there.
> ---
>  include/linux/types.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 6dfdb8e8e4c3..cd5b2b0f4b02 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -179,7 +179,7 @@ typedef phys_addr_t resource_size_t;
>  typedef unsigned long irq_hw_number_t;
>  
>  typedef struct {
> -	int counter;
> +	int counter __aligned(sizeof(int));
>  } atomic_t;
>  
>  #define ATOMIC_INIT(i) { (i) }
> -- 
> 2.49.1
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>

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

[Finn Thain]

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.

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

[Finn Thain]

On Mon, 25 Aug 2025, Lance Yang wrote:

> > 
> > More problematic is that, IIRC, m68k kmalloc() allocates 16bit aligned 
> > memory. This has broken other things in the past. I doubt that 
> > increasing the alignment to 32bits would make much difference to the 
> > kernel memory footprint.
> 
> @Finn Given this new information, how about we just apply the runtime 
> check fix for now?

New information? No, that's just hear-say.

> Since we plan to remove the entire pointer-encoding scheme later anyway, 
> a minimal and targeted change could be the logical choice. It's easy and 
> safe to backport, and it cleanly stops the warnings from all sources 
> without introducing new risks - exactly what we need for stable kernels.
> 

Well, that's up to you, of course. If you want my comment, I'd only ask 
whether or not the bug is theoretical (outside of m68k).

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

[Lance Yang]

On 2025/8/27 16:00, Finn Thain wrote:
> 
> On Mon, 25 Aug 2025, Lance Yang wrote:
> 
>>>
>>> More problematic is that, IIRC, m68k kmalloc() allocates 16bit aligned
>>> memory. This has broken other things in the past. I doubt that
>>> increasing the alignment to 32bits would make much difference to the
>>> kernel memory footprint.
>>
>> @Finn Given this new information, how about we just apply the runtime
>> check fix for now?
> 
> New information? No, that's just hear-say.

Emm... I jumped the gun there ;p

> 
>> Since we plan to remove the entire pointer-encoding scheme later anyway,
>> a minimal and targeted change could be the logical choice. It's easy and
>> safe to backport, and it cleanly stops the warnings from all sources
>> without introducing new risks - exactly what we need for stable kernels.
>>
> 
> Well, that's up to you, of course. If you want my comment, I'd only ask
> whether or not the bug is theoretical (outside of m68k).

Well, let's apply both this fix and the runtime check fix[1] as Masami
suggested ;)

[1] https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev/

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

[Peter Zijlstra]

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?

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.

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

[Finn Thain]

On Mon, 25 Aug 2025, Lance Yang wrote:

> 
> Same here, using a global static variable instead of a local one. The 
> result is consistently misaligned.
> 
> ```
> #include <linux/module.h>
> #include <linux/init.h>
> 
> static struct __attribute__((packed)) test_container {
>     char padding[49];
>     struct mutex io_lock;
> } cont;
> 
> static int __init alignment_init(void)
> {
>     pr_info("Container base address      : %px\n", &cont);
>     pr_info("io_lock member address      : %px\n", &cont.io_lock);
>     pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
>     return 0;
> }
> 
> static void __exit alignment_exit(void)
> {
>     pr_info("Module unloaded\n");
> }
> 
> module_init(alignment_init);
> module_exit(alignment_exit);
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("x");
> MODULE_DESCRIPTION("x");
> ```
> 
> Result from dmesg:
> 
> ```
> [Mon Aug 25 19:33:28 2025] Container base address      : ffffffffc28f0940
> [Mon Aug 25 19:33:28 2025] io_lock member address      : ffffffffc28f0971
> [Mon Aug 25 19:33:28 2025] io_lock address offset mod 4: 1
> ```
> 

FTR, I was able to reproduce that result (i.e. static storage):

[    0.320000] Container base address      : 0055d9d0
[    0.320000] io_lock member address      : 0055da01
[    0.320000] io_lock address offset mod 4: 1

I think the experiments you sent previously would have demonstrated the 
same result, except for the unpredictable base address that you sensibly 
logged in this version.

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

[Lance Yang]

On 2025/8/28 07:43, Finn Thain wrote:
> 
> On Mon, 25 Aug 2025, Lance Yang wrote:
> 
>>
>> Same here, using a global static variable instead of a local one. The
>> result is consistently misaligned.
>>
>> ```
>> #include <linux/module.h>
>> #include <linux/init.h>
>>
>> static struct __attribute__((packed)) test_container {
>>      char padding[49];
>>      struct mutex io_lock;
>> } cont;
>>
>> static int __init alignment_init(void)
>> {
>>      pr_info("Container base address      : %px\n", &cont);
>>      pr_info("io_lock member address      : %px\n", &cont.io_lock);
>>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
>>      return 0;
>> }
>>
>> static void __exit alignment_exit(void)
>> {
>>      pr_info("Module unloaded\n");
>> }
>>
>> module_init(alignment_init);
>> module_exit(alignment_exit);
>> MODULE_LICENSE("GPL");
>> MODULE_AUTHOR("x");
>> MODULE_DESCRIPTION("x");
>> ```
>>
>> Result from dmesg:
>>
>> ```
>> [Mon Aug 25 19:33:28 2025] Container base address      : ffffffffc28f0940
>> [Mon Aug 25 19:33:28 2025] io_lock member address      : ffffffffc28f0971
>> [Mon Aug 25 19:33:28 2025] io_lock address offset mod 4: 1
>> ```
>>
> 
> FTR, I was able to reproduce that result (i.e. static storage):
> 
> [    0.320000] Container base address      : 0055d9d0
> [    0.320000] io_lock member address      : 0055da01
> [    0.320000] io_lock address offset mod 4: 1
> 
> I think the experiments you sent previously would have demonstrated the
> same result, except for the unpredictable base address that you sensibly
> logged in this version.

Thanks for taking the time to reproduce it!

This proves the problem can happen in practice (e.g., with packed structs),
so we need to ignore the unaligned pointers on the architectures that don't
trap for now.

Cheers,
Lance

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

[Finn Thain]

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.

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

[Geert Uytterhoeven]

Hi Lance,

On Thu, 28 Aug 2025 at 04:05, Lance Yang <lance.yang@linux.dev> wrote:
> On 2025/8/28 07:43, Finn Thain wrote:
> > On Mon, 25 Aug 2025, Lance Yang wrote:
> >> Same here, using a global static variable instead of a local one. The
> >> result is consistently misaligned.
> >>
> >> ```
> >> #include <linux/module.h>
> >> #include <linux/init.h>
> >>
> >> static struct __attribute__((packed)) test_container {
> >>      char padding[49];
> >>      struct mutex io_lock;
> >> } cont;
> >>
> >> static int __init alignment_init(void)
> >> {
> >>      pr_info("Container base address      : %px\n", &cont);
> >>      pr_info("io_lock member address      : %px\n", &cont.io_lock);
> >>      pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
> >>      return 0;
> >> }
> >>
> >> static void __exit alignment_exit(void)
> >> {
> >>      pr_info("Module unloaded\n");
> >> }
> >>
> >> module_init(alignment_init);
> >> module_exit(alignment_exit);
> >> MODULE_LICENSE("GPL");
> >> MODULE_AUTHOR("x");
> >> MODULE_DESCRIPTION("x");
> >> ```
> >>
> >> Result from dmesg:
> >>
> >> ```
> >> [Mon Aug 25 19:33:28 2025] Container base address      : ffffffffc28f0940
> >> [Mon Aug 25 19:33:28 2025] io_lock member address      : ffffffffc28f0971
> >> [Mon Aug 25 19:33:28 2025] io_lock address offset mod 4: 1
> >> ```
> >>
> >
> > FTR, I was able to reproduce that result (i.e. static storage):
> >
> > [    0.320000] Container base address      : 0055d9d0
> > [    0.320000] io_lock member address      : 0055da01
> > [    0.320000] io_lock address offset mod 4: 1
> >
> > I think the experiments you sent previously would have demonstrated the
> > same result, except for the unpredictable base address that you sensibly
> > logged in this version.
>
> Thanks for taking the time to reproduce it!
>
> This proves the problem can happen in practice (e.g., with packed structs),
> so we need to ignore the unaligned pointers on the architectures that don't
> trap for now.

Putting locks inside a packed struct is definitely a Very Bad Idea
and a No Go.  Packed structs are meant to describe memory data and
MMIO register layouts, and must not contain control data for critical
sections.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

Hi David,

On Mon, 25 Aug 2025 at 14:07, David Laight <david.laight.linux@gmail.com> wrote:
> On Mon, 25 Aug 2025 15:46:42 +0800
> Lance Yang <lance.yang@linux.dev> wrote:
> > On 2025/8/25 14:17, Finn Thain wrote:
> > > On Mon, 25 Aug 2025, Lance Yang wrote:
> > >> What if we squash the runtime check fix into your patch?
> > >
> > > Did my patch not solve the problem?
> >
> > Hmm... it should solve the problem for natural alignment, which is a
> > critical fix.
> >
> > But it cannot solve the problem of forced misalignment from drivers using
> > #pragma pack(1). The runtime warning will still trigger in those cases.
> >
> > I built a simple test module on a kernel with your patch applied:
> >
> > ```
> > #include <linux/module.h>
> > #include <linux/init.h>
> >
> > struct __attribute__((packed)) test_container {
> >      char padding[49];
> >      struct mutex io_lock;
> > };
> >
> > static int __init alignment_init(void)
> > {
> >      struct test_container cont;
> >      pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
>
> Doesn't that give a compilation warning from 'taking the address of a packed member'?
> Ignore that at your peril.
>
> More problematic is that, IIRC, m68k kmalloc() allocates 16bit aligned memory.
> This has broken other things in the past.

Really? AFAIK it always returns memory that is at least aligned to the
cache line size (i.e. 16 bytes on m68k). So perhaps you are confusing
"16bit" with "16byte"?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Geert Uytterhoeven]

Hi Eero,

On Tue, 26 Aug 2025 at 17:22, Eero Tamminen <oak@helsinkinet.fi> wrote:
> On 25.8.2025 5.03, Finn Thain wrote:
> > Some recent commits incorrectly assumed the natural alignment of locks.
> > That assumption fails on Linux/m68k (and, interestingly, would have failed
> > on Linux/cris also). This leads to spurious warnings from the hang check
> > code. Fix this bug by adding the necessary 'aligned' attribute.
> [...]
> > Reported-by: Eero Tamminen <oak@helsinkinet.fi>
> > Closes: https://lore.kernel.org/lkml/CAMuHMdW7Ab13DdGs2acMQcix5ObJK0O2dG_Fxzr8_g58Rc1_0g@mail.gmail.com/
> > Fixes: e711faaafbe5 ("hung_task: replace blocker_mutex with encoded blocker")
> > Signed-off-by: Finn Thain <fthain@linux-m68k.org>
> > ---
> > I tested this on m68k using GCC and it fixed the problem for me. AFAIK,
> > the other architectures naturally align ints already so I'm expecting to
> > see no effect there.
>
> Yes, it fixes both of the issues (warnings & broken console):
>         Tested-by: Eero Tamminen <oak@helsinkinet.fi>
>
> (Emulated Atari Falcon) boot up performance with this is within normal
> variation.
>
> On 23.8.2025 10.49, Lance Yang wrote:
>  > Anyway, I've prepared two patches for discussion, either of which should
>  > fix the alignment issue :)
>  >
>  > Patch A[1] adjusts the runtime checks to handle unaligned pointers.
>  > Patch B[2] enforces 4-byte alignment on the core lock structures.
>  >
>  > Both tested on x86-64.
>  >
>  > [1]
> https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
>  > [2] https://lore.kernel.org/lkml/20250823074048.92498-1-
>  > lance.yang@linux.dev
>
> Same goes for both of these, except that removing warnings makes minimal
> kernel boot 1-2% faster than 4-aligning the whole struct.

That is an interesting outcome! So the gain of naturally-aligning the
lock is more than offset by the increased cache pressure due to wasting
(a bit?) more memory.

Do you know what was the impact on total kernel size?
Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Peter Zijlstra]

On Thu, Aug 28, 2025 at 07:53:52PM +1000, Finn Thain wrote:

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

Hmm, somehow I thought one of those enforced natural alignment. Oh well.

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

Right, but those architectures have natural alignment. m68k is 'special'
in that it doesn't have this.

> For m68k, perhaps we could have a compile-time check:

I don't think build-time is sufficient. There is various code that casts
to atomic types and other funny stuff like that.

If you want to ensure 'atomics' are always naturally aligned, the only
sound way is to have a runtime test/trap.

Something like the completely untested below should do I suppose.

---
diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
index 711a1f0d1a73..e39cdfe5a59e 100644
--- a/include/linux/instrumented.h
+++ b/include/linux/instrumented.h
@@ -67,6 +67,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 & 3));
 }
 
 /**
@@ -81,6 +82,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 & 3));
 }
 
 /**
@@ -95,6 +97,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 & 3));
 }
 
 /**
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index dc0e0c6ed075..1c7e30cdfe04 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -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

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

[Peter Zijlstra]

On Mon, Sep 01, 2025 at 11:36:00AM +0200, Peter Zijlstra wrote:

> Something like the completely untested below should do I suppose.
> 
> ---
> diff --git a/include/linux/instrumented.h b/include/linux/instrumented.h
> index 711a1f0d1a73..e39cdfe5a59e 100644
> --- a/include/linux/instrumented.h
> +++ b/include/linux/instrumented.h
> @@ -67,6 +67,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 & 3));
>  }
>  
>  /**
> @@ -81,6 +82,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 & 3));
>  }
>  
>  /**
> @@ -95,6 +97,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 & 3));
>  }
>  
>  /**

Arguably, that should've been something like:

	((unsigned long)v & (size-1))

I suppose.

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index dc0e0c6ed075..1c7e30cdfe04 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -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

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

[Eero Tamminen]

Hi Geert,

On 1.9.2025 11.51, Geert Uytterhoeven wrote:
>> On 23.8.2025 10.49, Lance Yang wrote:
>>   > Anyway, I've prepared two patches for discussion, either of which should
>>   > fix the alignment issue :)
>>   >
>>   > Patch A[1] adjusts the runtime checks to handle unaligned pointers.
>>   > Patch B[2] enforces 4-byte alignment on the core lock structures.
>>   >
>>   > Both tested on x86-64.
>>   >
>>   > [1]
>> https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
>>   > [2] https://lore.kernel.org/lkml/20250823074048.92498-1-
>>   > lance.yang@linux.dev
>>
>> Same goes for both of these, except that removing warnings makes minimal
>> kernel boot 1-2% faster than 4-aligning the whole struct.

Note that above result was from (emulated) 68030 Falcon, i.e. something 
that has really small caches (256-byte i-/d-cache), *and* a kernel 
config using CONFIG_CC_OPTIMIZE_FOR_SIZE=y (with GCC 12.2).


> That is an interesting outcome! So the gain of naturally-aligning the
> lock is more than offset by the increased cache pressure due to wasting
> (a bit?) more memory.

Another reason could be those extra inlined warning checks in:
-----------------------------------------------------
$ git grep -e hung_task_set_blocker -e hung_task_clear_blocker kernel/
kernel/locking/mutex.c: hung_task_set_blocker(lock, BLOCKER_TYPE_MUTEX);
kernel/locking/mutex.c: hung_task_clear_blocker();
kernel/locking/rwsem.c:         hung_task_set_blocker(sem, 
BLOCKER_TYPE_RWSEM_READER);
kernel/locking/rwsem.c:         hung_task_clear_blocker();
kernel/locking/rwsem.c:         hung_task_set_blocker(sem, 
BLOCKER_TYPE_RWSEM_WRITER);
kernel/locking/rwsem.c:         hung_task_clear_blocker();
kernel/locking/semaphore.c:     hung_task_set_blocker(sem, 
BLOCKER_TYPE_SEM);
kernel/locking/semaphore.c:     hung_task_clear_blocker();
-----------------------------------------------------


> Do you know what was the impact on total kernel size?

As expected, kernel code size is smaller with the static inlined warn 
checks removed:
-----------------------------------------------------
$ size vmlinux-m68k-6.16-fix1 vmlinux-m68k-6.16-fix2
    text	   data	    bss	    dec	    hex	filename
3088520	 953532	  84224	4126276	 3ef644	vmlinux-m68k-6.16-fix1  [1]
3088730	 953564	  84192	4126486	 3ef716	vmlinux-m68k-6.16-fix2  [2]
-----------------------------------------------------

But could aligning of structs have caused 32 bytes moving from BSS to 
DATA section?


	- Eero

PS. I profiled these 3 kernels on emulated Falcon. According to (Hatari) 
profiler, main difference in the kernel with the warnings removed, is it 
doing less than half of the calls to  NCR5380_read() / 
atari_scsi_reg_read(), compared to the other 2 versions.

These additional 2x calls in the other two versions, seem to mostly come 
through chain originating from process_scheduled_works(), 
NCR5380_poll_politely*() functions and bus probing.

After quick look at the WARN_ON_ONCE()s and SCSI code, I have no idea 
how having those checks being inlined to locking functions, or not, 
would cause a difference like that.  I've tried patching & building 
kernels again, and repeating profiling, but result is same.

While Hatari call (graph) tracking might have some issue (due to kernel 
stack return address manipulation), I don't see how there could be a 
problem with the profiler instruction counts.  Kernel code at given 
address does not change during boot in monolithic kernel, (emulator) 
profiler tracks _every_ executed instruction/address, and it's clearly 
correct function:
------------------------------------
# disassembly with profile data: <instructions percentage>% (<sum of 
instructions>, <sum of cycles>, <sum of i-cache misses>, <sum of d-cache 
hits>)
...
atari_scsi_falcon_reg_read:
$001dd826  link.w    a6,#$0      0.43% (414942, 1578432, 44701, 0)
$001dd82a  move.w    sr,d1       0.43% (414942, 224, 8, 0)
$001dd82c  ori.w     #$700,sr    0.43% (414942, 414368, 44705, 0)
$001dd830  move.l    $8(a6),d0   0.43% (414942, 357922, 44705, 414911)
$001dd834  addi.l    #$88,d0     0.43% (414942, 1014804, 133917, 0)
$001dd83a  move.w    d0,$8606.w  0.43% (414942, 3618352, 89169, 0)
$001dd83e  move.w    $8604.w,d0  0.43% (414942, 3620646, 89162, 0)
$001dd842  move.w    d1,sr       0.43% (414942, 2148, 142, 0)
$001dd844  unlk      a6          0.43% (414942, 436, 0, 414893)
$001dd846  rts                   0.43% (414942, 1073934, 134123, 414942)
atari_scsi_falcon_reg_write:
$001dd848  link.w    a6,#$0      0.00% (81, 484, 29, 0)
$001dd84c  move.l    $c(a6),d0   0.00% (81, 326, 29, 73)
...
------------------------------------

Maybe those WARN_ON_ONCE() checks just happen to slow down something 
marginally so that things get interrupted & re-started more for the SCSI 
code?

PPS. emulated machine has no SCSI drives, only one IDE drive (with 4MB 
Busybox partition):
----------------------------------------------------
scsi host0: Atari native SCSI, irq 15, io_port 0x0, base 0x0, can_queue 
1, cmd_per_lun 2, sg_tablesize 1, this_id 7, flags { }
atari-falcon-ide atari-falcon-ide: Atari Falcon and Q40/Q60 PATA controller
scsi host1: pata_falcon
ata1: PATA max PIO4 cmd fff00000 ctl fff00038 data fff00000 no IRQ, 
using PIO polling
...
ata1: found unknown device (class 0)
ata1.00: ATA-7: Hatari  IDE disk 4M, 1.0, max UDMA/100
ata1.00: 8192 sectors, multi 16: LBA48
ata1.00: configured for PIO
...
scsi 1:0:0:0: Direct-Access     ATA      Hatari  IDE disk 1.0  PQ: 0 ANSI: 5
sd 1:0:0:0: [sda] 8192 512-byte logical blocks: (4.19 MB/4.00 MiB)
sd 1:0:0:0: [sda] Write Protect is off
sd 1:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 1:0:0:0: [sda] Write cache: disabled, read cache: enabled, doesn't 
support DPO or FUA
sd 1:0:0:0: [sda] Preferred minimum I/O size 512 bytes
sd 1:0:0:0: [sda] Attached SCSI disk
VFS: Mounted root (ext2 filesystem) readonly on device 8:0.
---------------------------------------------------

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

[Lance Yang]

Hi Geert,

On 2025/9/1 16:45, Geert Uytterhoeven wrote:
> Hi Lance,
> 
> On Thu, 28 Aug 2025 at 04:05, Lance Yang <lance.yang@linux.dev> wrote:
>> On 2025/8/28 07:43, Finn Thain wrote:
>>> On Mon, 25 Aug 2025, Lance Yang wrote:
>>>> Same here, using a global static variable instead of a local one. The
>>>> result is consistently misaligned.
>>>>
>>>> ```
>>>> #include <linux/module.h>
>>>> #include <linux/init.h>
>>>>
>>>> static struct __attribute__((packed)) test_container {
>>>>       char padding[49];
>>>>       struct mutex io_lock;
>>>> } cont;
>>>>
>>>> static int __init alignment_init(void)
>>>> {
>>>>       pr_info("Container base address      : %px\n", &cont);
>>>>       pr_info("io_lock member address      : %px\n", &cont.io_lock);
>>>>       pr_info("io_lock address offset mod 4: %lu\n", (unsigned long)&cont.io_lock % 4);
>>>>       return 0;
>>>> }
>>>>
>>>> static void __exit alignment_exit(void)
>>>> {
>>>>       pr_info("Module unloaded\n");
>>>> }
>>>>
>>>> module_init(alignment_init);
>>>> module_exit(alignment_exit);
>>>> MODULE_LICENSE("GPL");
>>>> MODULE_AUTHOR("x");
>>>> MODULE_DESCRIPTION("x");
>>>> ```
>>>>
>>>> Result from dmesg:
>>>>
>>>> ```
>>>> [Mon Aug 25 19:33:28 2025] Container base address      : ffffffffc28f0940
>>>> [Mon Aug 25 19:33:28 2025] io_lock member address      : ffffffffc28f0971
>>>> [Mon Aug 25 19:33:28 2025] io_lock address offset mod 4: 1
>>>> ```
>>>>
>>>
>>> FTR, I was able to reproduce that result (i.e. static storage):
>>>
>>> [    0.320000] Container base address      : 0055d9d0
>>> [    0.320000] io_lock member address      : 0055da01
>>> [    0.320000] io_lock address offset mod 4: 1
>>>
>>> I think the experiments you sent previously would have demonstrated the
>>> same result, except for the unpredictable base address that you sensibly
>>> logged in this version.
>>
>> Thanks for taking the time to reproduce it!
>>
>> This proves the problem can happen in practice (e.g., with packed structs),
>> so we need to ignore the unaligned pointers on the architectures that don't
>> trap for now.
> 
> Putting locks inside a packed struct is definitely a Very Bad Idea
> and a No Go.  Packed structs are meant to describe memory data and

Right. That's definitely not how packed structs should be used ;)

> MMIO register layouts, and must not contain control data for critical
> sections.

Unfortunately, this patten was found in an in-tree driver, as reported[1]
by kernel test robot, and there might be other undiscovered instances ...

[1] 
https://lore.kernel.org/oe-kbuild-all/202508240539.ARmC1Umu-lkp@intel.com

Cheers,
Lance

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

[Geert Uytterhoeven]

Hi Lance,

On Tue, 2 Sept 2025 at 15:31, Lance Yang <lance.yang@linux.dev> wrote:
> On 2025/9/1 16:45, Geert Uytterhoeven wrote:
> > On Thu, 28 Aug 2025 at 04:05, Lance Yang <lance.yang@linux.dev> wrote:
> >> This proves the problem can happen in practice (e.g., with packed structs),
> >> so we need to ignore the unaligned pointers on the architectures that don't
> >> trap for now.
> >
> > Putting locks inside a packed struct is definitely a Very Bad Idea
> > and a No Go.  Packed structs are meant to describe memory data and
>
> Right. That's definitely not how packed structs should be used ;)
>
> > MMIO register layouts, and must not contain control data for critical
> > sections.
>
> Unfortunately, this patten was found in an in-tree driver, as reported[1]
> by kernel test robot, and there might be other undiscovered instances ...
>
> [1]
> https://lore.kernel.org/oe-kbuild-all/202508240539.ARmC1Umu-lkp@intel.com

That one is completely bogus, and should be removed.
Currently it would crash on any platform that does not support
unaligned accesses.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds