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

[Lance Yang <lance.yang@linux.dev>]

On 2025/8/24 12:18, Finn Thain wrote:
> 
> 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.

I think there might be a misunderstanding about the level of abstraction
at which the blocker tracking operates.

The blocker tracking mechanism operates on pointers to higher-level
locks (like struct mutex), as that is what is stored in the
task_struct->blocker field. It does not operate on the lower-level
arch_spinlock_t inside it.

While we could track the internal arch_spinlock_t, that would break
encapsulation. The hung task detector should remain generic and not
depend on lock-specific implementation details ;)

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


Completely agreed that correctness and portability are the goals.

Please, feel free to send a patch.