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

[David Laight <David.Laight@ACULAB.COM>]

From: Jens Axboe
> Sent: 10 March 2021 20:40
> 
> 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
> there doesn't seem to be an issue with just having it wherever in struct
> request.

Can you silence it by adding an extra layer of 'struct'?
Something like:

struct [
	....
	struct {
		rq_rype rq:
	} __aligned(8);
	...
};

So that 'rq' will be aligned, but itself doesn't have
the alignment constraint?

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)