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

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

[Nathan Chancellor]

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 :)

Cheers,
Nathan

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

[Jens Axboe]

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?

-- 
Jens Axboe

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

[Nathan Chancellor]

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/

Cheers,
Nathan

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

[Jens Axboe]

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.

-- 
Jens Axboe

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

[Nathan Chancellor]

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

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

[Jens Axboe]

On 3/10/21 1:52 PM, Nathan Chancellor wrote:
> 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.

Right, that's what I was getting at, but I needed to page that context
back in, it had long since been purged :-)

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

That's a good question, I'm not sure what the best approach here would
be. Funnily enough, on my build, it just so happens to be 32-byte
aligned anyway, but that's by mere chance.

-- 
Jens Axboe

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

[Nathan Chancellor]

On Wed, Mar 10, 2021 at 02:03:56PM -0700, Jens Axboe wrote:
> On 3/10/21 1:52 PM, Nathan Chancellor wrote:
> > 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.
> 
> Right, that's what I was getting at, but I needed to page that context
> back in, it had long since been purged :-)
> 
> > 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).
> 
> That's a good question, I'm not sure what the best approach here would
> be. Funnily enough, on my build, it just so happens to be 32-byte
> aligned anyway, but that's by mere chance.

As far as I can tell, there are two options.

1. Objectively smallest option is to just disable -Walign-mismatch for
   the whole translation unit. The benefit of this route is one small
   and simple patch. The downside is that if there are any more
   instances of this added in the future, they won't be caught. May or
   may not actually happen or be a big deal.

diff --git a/block/Makefile b/block/Makefile
index 8d841f5f986f..432d0329fb58 100644
--- a/block/Makefile
+++ b/block/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_BLOCK) := bio.o elevator.o blk-core.o blk-sysfs.o \
 			blk-lib.o blk-mq.o blk-mq-tag.o blk-stat.o \
 			blk-mq-sysfs.o blk-mq-cpumap.o blk-mq-sched.o ioctl.o \
 			genhd.o ioprio.o badblocks.o partitions/ blk-rq-qos.o
+CFLAGS_blk-mq.o := $(call cc-disable-warning, align-mismatch)
 
 obj-$(CONFIG_BOUNCE)		+= bounce.o
 obj-$(CONFIG_BLK_SCSI_REQUEST)	+= scsi_ioctl.o

2. Use the __diag() infrastructure, which would allow us to locally
   disable the warning while adding a comment. The benefit of this
   approach is that the warning is only disabled for the problematic
   line so other instances can be caught. The downside is there is a
   little churn as it will involve a patch for the initial __diag()
   support for clang (as it has not needed it yet) and a few more lines
   in block/blk-mq.c. Additionally, the reason for the warning can be
   documented (the comment can obviously be improved).

diff --git a/block/blk-mq.c b/block/blk-mq.c
index d4d7c1caa439..2781c04d06bc 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -627,7 +627,10 @@ static void blk_mq_complete_send_ipi(struct request *rq)
 	list = &per_cpu(blk_cpu_done, cpu);
 	if (llist_add(&rq->ipi_list, list)) {
 		INIT_CSD(&rq->csd, __blk_mq_complete_request_remote, rq);
+		__diag_push();
+		__diag_ignore(clang, 13, "-Walign-mismatch", "There is no issue with misalignment here");
 		smp_call_function_single_async(cpu, &rq->csd);
+		__diag_pop();
 	}
 }
 
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h
index 04c0a5a717f7..0a20fddc1c30 100644
--- a/include/linux/compiler-clang.h
+++ b/include/linux/compiler-clang.h
@@ -55,3 +55,25 @@
 #if __has_feature(shadow_call_stack)
 # define __noscs	__attribute__((__no_sanitize__("shadow-call-stack")))
 #endif
+
+/*
+ *  * Turn individual warnings and errors on and off locally, depending
+ *   * on version.
+ *    */
+#define __diag_clang(version, severity, s) \
+		__diag_clang_ ## version(__diag_clang_ ## severity s)
+
+/* Severity used in pragma directives */
+#define __diag_clang_ignore	ignored
+#define __diag_clang_warn	warning
+#define __diag_clang_error	error
+
+#define __diag_str1(s)		#s
+#define __diag_str(s)		__diag_str1(s)
+#define __diag(s)		_Pragma(__diag_str(clang diagnostic s))
+
+#if CONFIG_CLANG_VERSION >= 130000
+#define __diag_clang_13(s)		__diag(s)
+#else
+#define __diag_clang_13(s)
+#endif
diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index e5dd5a4ae946..a505d8a4302d 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -328,6 +328,10 @@ struct ftrace_likely_data {
 #define __diag(string)
 #endif
 
+#ifndef __diag_clang
+#define __diag_clang(version, severity, string)
+#endif
+
 #ifndef __diag_GCC
 #define __diag_GCC(version, severity, string)
 #endif

I would say the preference is ultimately up to the maintainer, unless my
fellow ClangBuiltLinux maintainers/contributors have any further
comments/objections.

Cheers,
Nathan

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

[David Laight]

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)