Re: -Walign-mismatch in block/blk-mq.c

[Nathan Chancellor <nathan@kernel.org>]

On Wed, Mar 10, 2021 at 01:40:25PM -0700, Jens Axboe wrote:
> On 3/10/21 1:33 PM, Nathan Chancellor wrote:
> > On Wed, Mar 10, 2021 at 01:21:52PM -0700, Jens Axboe wrote:
> >> On 3/10/21 11:23 AM, Nathan Chancellor wrote:
> >>> Hi Jens,
> >>>
> >>> There is a new clang warning added in the development branch,
> >>> -Walign-mismatch, which shows an instance in block/blk-mq.c:
> >>>
> >>> block/blk-mq.c:630:39: warning: passing 8-byte aligned argument to
> >>> 32-byte aligned parameter 2 of 'smp_call_function_single_async' may
> >>> result in an unaligned pointer access [-Walign-mismatch]
> >>>                 smp_call_function_single_async(cpu, &rq->csd);
> >>>                                                     ^
> >>> 1 warning generated.
> >>>
> >>> There appears to be some history here as I can see that this member was
> >>> purposefully unaligned in commit 4ccafe032005 ("block: unalign
> >>> call_single_data in struct request"). However, I later see a change in
> >>> commit 7c3fb70f0341 ("block: rearrange a few request fields for better
> >>> cache layout") that seems somewhat related. Is it possible to get back
> >>> the alignment by rearranging the structure again? This seems to be the
> >>> only solution for the warning aside from just outright disabling it,
> >>> which would be a shame since it seems like it could be useful for
> >>> architectures that cannot handle unaligned accesses well, unless I am
> >>> missing something obvious :)
> >>
> >> It should not be hard to ensure that alignment without re-introducing
> >> the bloat. Is there some background on why 32-byte alignment is
> >> required?
> >>
> > 
> > This alignment requirement was introduced in commit 966a967116e6 ("smp:
> > Avoid using two cache lines for struct call_single_data") and it looks
> > like there was a thread between you and Peter Zijlstra that has some
> > more information on this:
> > https://lore.kernel.org/r/a9beb452-7344-9e2d-fc80-094d8f5a0394@kernel.dk/
> 
> Ah now I remember - so it's not that it _needs_ to be 32-byte aligned,
> it's just a handy way to ensure that it doesn't straddle two cachelines.
> In fact, there's no real alignment concern, outside of performance
> reasons we don't want it touching two cachelines.
> 
> So... what exactly is your concern? Just silencing that warning? Because

Yes, dealing with the warning in some way is my only motivation. My
apologies, I should have led with that. I had assumed that this would
potentially be an issue due to the warning's text and that rearranging
the structure might allow the alignment to be added back but if there is
not actually a problem, then the warning should be silenced in some way.

I am not sure if there is a preferred way to silence it (CFLAGS_... or
some of the __diag() infrastructure in include/linux/compiler_types.h).

> there doesn't seem to be an issue with just having it wherever in struct
> request.
> 

Cheers,
Nathan