Re: [PATCH 1/1] hung_task: fix warnings by enforcing alignment on lock structures

[Finn Thain <fthain@linux-m68k.org>]

On Sun, 24 Aug 2025, Lance Yang wrote:

> On 2025/8/24 08:47, Finn Thain wrote:
> > 
> > On Sun, 24 Aug 2025, kernel test robot wrote:
> > 
> >> All warnings (new ones prefixed by >>):
> >>
> >>     In file included from sound/soc/codecs/mt6660.c:15:
> >>>> sound/soc/codecs/mt6660.h:28:1: warning: alignment 1 of 'struct
> >>>> mt6660_chip' is less than 8 [-Wpacked-not-aligned]
> >>        28 | };
> >>           | ^
> >>>> sound/soc/codecs/mt6660.h:25:22: warning: 'io_lock' offset 49 in 'struct
> >>>> mt6660_chip' isn't aligned to 8 [-Wpacked-not-aligned]
> >>        25 |         struct mutex io_lock;
> >>           |                      ^~~~~~~
> >>
> > 
> > Misalignment warnings like this one won't work if you just pick an
> > alignment arbitrarily i.e. to suit whatever bitfield you happen to need.
> 
> Yes.
> 
> The build warnings reported by the test robot are exactly the kind of
> unintended side effect I was concerned about. It confirms that forcing
> alignment on a core structure like struct mutex breaks other parts of
> the kernel that rely on packed structures ;)
> 

Sure, your patch broke the build. So why not write a better patch? You 
don't need to align the struct, you need to align the lock, like I said 
already.

> > 
> > Instead, I think I would naturally align the actual locks, that is, 
> > arch_spinlock_t and arch_rwlock_t in include/linux/spinlock_types*.h.
> 
> That's an interesting point. The blocker tracking mechanism currently 
> operates on higher-level structures like struct mutex. Moving the type 
> encoding down to the lowest-level locks would be a more complex and 
> invasive change, likely beyond the scope of fixing this particular 
> issue.
> 

I don't see why changing kernel struct layouts on m68k is particularly 
invasive. Perhaps I'm missing something (?)

> Looking further ahead, a better long-term solution might be to stop 
> repurposing pointer bits altogether. We could add an explicit 
> blocker_type field to task_struct to be used alongside the blocker 
> field. That would be a much cleaner design. TODO +1 for that idea :)
> 
> So, let's drop the patch[1] that enforces alignment and go back to my 
> initial proposal[2], which adjusts the runtime checks to gracefully 
> handle unaligned pointers. That one is self-contained, has minimal 
> impact, and is clearly the safer solution for now.
> 
> [1] https://lore.kernel.org/lkml/20250823074048.92498-1-lance.yang@linux.dev
> [2] https://lore.kernel.org/lkml/20250823050036.7748-1-lance.yang@linux.dev
> 

I am willing to send a patch if it serves correctness and portability. So 
you may wish to refrain from crippling your blocker tracking algorithm for 
now.