Re: [PATCH] blk-mq: fix alignment mismatch.
Sounds good! Thanks for the help and the link. On Thu, Apr 8, 2021 at 11:12 AM Nathan Chancellor wrote: > > Hi Jian, > > On Thu, Apr 08, 2021 at 10:57:54AM -0700, Jian Cai wrote: > > So this issue is blocking the LLVM upgrading on ChromeOS. Nathan, do > > you mind sending out the smaller patch like Nick suggested just to see > > what feedback we could get? I could send it for you if you are busy, > > and please let me know what tags I should use in that case. > > > > Thanks, > > Jian > > I will go ahead and send the smaller patch at some point today. > > For what it's worth, Nick brought up https://reviews.llvm.org/D100037, > which may be relevant here. > > Cheers, > Nathan > > > On Wed, Mar 31, 2021 at 3:06 PM Nick Desaulniers > > wrote: > > > > > > On Wed, Mar 31, 2021 at 2:58 PM Nathan Chancellor > > > wrote: > > > > > > > > On Wed, Mar 31, 2021 at 02:27:03PM -0700, Jian Cai wrote: > > > > > > > > > > I just realized you already proposed solutions for skipping the check > > > > > in > > > > > https://lore.kernel.org/linux-block/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/#t. > > > > > Do you have any plans to send them for review? > > > > > > > > I was hoping to gather some feedback on which option would be preferred > > > > by Jens and the other ClangBuiltLinux folks before I sent them along. I > > > > can send the first just to see what kind of feedback I can gather. > > > > > > Either approach is fine by me. The smaller might be easier to get > > > accepted into stable. The larger approach will probably become more > > > useful in the future (having the diag infra work properly with clang). > > > I think the ball is kind of in Jens' court to decide. Would doing > > > both be appropriate, Jens? Have the smaller patch tagged for stable > > > disabling it for the whole file, then another commit on top not tagged > > > for stable that adds the diag infra, and a third on top replacing the > > > file level warning disablement with local diags to isolate this down > > > to one case? It's a fair but small amount of churn IMO; but if Jens > > > is not opposed it seems fine? > > > -- > > > Thanks, > > > ~Nick Desaulniers
Re: [PATCH] blk-mq: fix alignment mismatch.
Hi Jian, On Thu, Apr 08, 2021 at 10:57:54AM -0700, Jian Cai wrote: > So this issue is blocking the LLVM upgrading on ChromeOS. Nathan, do > you mind sending out the smaller patch like Nick suggested just to see > what feedback we could get? I could send it for you if you are busy, > and please let me know what tags I should use in that case. > > Thanks, > Jian I will go ahead and send the smaller patch at some point today. For what it's worth, Nick brought up https://reviews.llvm.org/D100037, which may be relevant here. Cheers, Nathan > On Wed, Mar 31, 2021 at 3:06 PM Nick Desaulniers > wrote: > > > > On Wed, Mar 31, 2021 at 2:58 PM Nathan Chancellor wrote: > > > > > > On Wed, Mar 31, 2021 at 02:27:03PM -0700, Jian Cai wrote: > > > > > > > > I just realized you already proposed solutions for skipping the check > > > > in > > > > https://lore.kernel.org/linux-block/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/#t. > > > > Do you have any plans to send them for review? > > > > > > I was hoping to gather some feedback on which option would be preferred > > > by Jens and the other ClangBuiltLinux folks before I sent them along. I > > > can send the first just to see what kind of feedback I can gather. > > > > Either approach is fine by me. The smaller might be easier to get > > accepted into stable. The larger approach will probably become more > > useful in the future (having the diag infra work properly with clang). > > I think the ball is kind of in Jens' court to decide. Would doing > > both be appropriate, Jens? Have the smaller patch tagged for stable > > disabling it for the whole file, then another commit on top not tagged > > for stable that adds the diag infra, and a third on top replacing the > > file level warning disablement with local diags to isolate this down > > to one case? It's a fair but small amount of churn IMO; but if Jens > > is not opposed it seems fine? > > -- > > Thanks, > > ~Nick Desaulniers
Re: [PATCH] blk-mq: fix alignment mismatch.
So this issue is blocking the LLVM upgrading on ChromeOS. Nathan, do you mind sending out the smaller patch like Nick suggested just to see what feedback we could get? I could send it for you if you are busy, and please let me know what tags I should use in that case. Thanks, Jian On Wed, Mar 31, 2021 at 3:06 PM Nick Desaulniers wrote: > > On Wed, Mar 31, 2021 at 2:58 PM Nathan Chancellor wrote: > > > > On Wed, Mar 31, 2021 at 02:27:03PM -0700, Jian Cai wrote: > > > > > > I just realized you already proposed solutions for skipping the check > > > in > > > https://lore.kernel.org/linux-block/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/#t. > > > Do you have any plans to send them for review? > > > > I was hoping to gather some feedback on which option would be preferred > > by Jens and the other ClangBuiltLinux folks before I sent them along. I > > can send the first just to see what kind of feedback I can gather. > > Either approach is fine by me. The smaller might be easier to get > accepted into stable. The larger approach will probably become more > useful in the future (having the diag infra work properly with clang). > I think the ball is kind of in Jens' court to decide. Would doing > both be appropriate, Jens? Have the smaller patch tagged for stable > disabling it for the whole file, then another commit on top not tagged > for stable that adds the diag infra, and a third on top replacing the > file level warning disablement with local diags to isolate this down > to one case? It's a fair but small amount of churn IMO; but if Jens > is not opposed it seems fine? > -- > Thanks, > ~Nick Desaulniers
Re: [PATCH] blk-mq: fix alignment mismatch.
On Wed, Mar 31, 2021 at 2:58 PM Nathan Chancellor wrote: > > On Wed, Mar 31, 2021 at 02:27:03PM -0700, Jian Cai wrote: > > > > I just realized you already proposed solutions for skipping the check > > in > > https://lore.kernel.org/linux-block/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/#t. > > Do you have any plans to send them for review? > > I was hoping to gather some feedback on which option would be preferred > by Jens and the other ClangBuiltLinux folks before I sent them along. I > can send the first just to see what kind of feedback I can gather. Either approach is fine by me. The smaller might be easier to get accepted into stable. The larger approach will probably become more useful in the future (having the diag infra work properly with clang). I think the ball is kind of in Jens' court to decide. Would doing both be appropriate, Jens? Have the smaller patch tagged for stable disabling it for the whole file, then another commit on top not tagged for stable that adds the diag infra, and a third on top replacing the file level warning disablement with local diags to isolate this down to one case? It's a fair but small amount of churn IMO; but if Jens is not opposed it seems fine? -- Thanks, ~Nick Desaulniers
Re: [PATCH] blk-mq: fix alignment mismatch.
Hi Jian,
On Wed, Mar 31, 2021 at 02:27:03PM -0700, Jian Cai wrote:
> Hi Nathan,
>
> I just realized you already proposed solutions for skipping the check
> in
> https://lore.kernel.org/linux-block/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/#t.
> Do you have any plans to send them for review?
>
> Thanks,
> Jian
I was hoping to gather some feedback on which option would be preferred
by Jens and the other ClangBuiltLinux folks before I sent them along. I
can send the first just to see what kind of feedback I can gather.
Cheers,
Nathan
> On Tue, Mar 30, 2021 at 6:31 PM Jian Cai wrote:
> >
> > Thanks for all the information. I'll check for similar instances and
> > send an updated version.
> >
> >
> > On Tue, Mar 30, 2021 at 5:26 PM Guenter Roeck wrote:
> > >
> > > On 3/30/21 4:29 PM, Nathan Chancellor wrote:
> > > > Hi Jian,
> > > >
> > > > On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
> > > >> This fixes the mismatch of alignments between csd and its use as an
> > > >> argument to smp_call_function_single_async, which causes build failure
> > > >> when -Walign-mismatch in Clang is used.
> > > >>
> > > >> Link:
> > > >> http://crrev.com/c/1193732
> > > >>
> > > >> Suggested-by: Guenter Roeck
> > > >> Signed-off-by: Jian Cai
> > > >
> > > > Thanks for the patch. This is effectively a revert of commit
> > > > 4ccafe032005 ("block: unalign call_single_data in struct request"),
> > > > which I had brought up in this thread:
> > > >
> > > > https://lore.kernel.org/r/20210310182307.zzcbi5w5jrmveld4@archlinux-ax161/
> > > >
> > > > This is obviously a correct fix, I am not just sure what the impact to
> > > > 'struct request' will be.
> > > >
> > >
> > > As commit 4ccafe032005 states, it increases the request structure size.
> > > Given the exchange referenced above, I think we'll need to disable
> > > the warning in the block code.
> > >
> > > Thanks,
> > > Guenter
> > >
> > > > Cheers,
> > > > Nathan
> > > >
> > > >> ---
> > > >> include/linux/blkdev.h | 2 +-
> > > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > > >> index bc6bc8383b43..3b92330d95ad 100644
> > > >> --- a/include/linux/blkdev.h
> > > >> +++ b/include/linux/blkdev.h
> > > >> @@ -231,7 +231,7 @@ struct request {
> > > >> unsigned long deadline;
> > > >>
> > > >> union {
> > > >> -struct __call_single_data csd;
> > > >> +call_single_data_t csd;
> > > >> u64 fifo_time;
> > > >> };
> > > >>
> > > >> --
> > > >> 2.31.0.291.g576ba9dcdaf-goog
> > > >>
> > >
Re: [PATCH] blk-mq: fix alignment mismatch.
Hi Nathan,
I just realized you already proposed solutions for skipping the check
in
https://lore.kernel.org/linux-block/20210310225240.4epj2mdmzt4vurr3@archlinux-ax161/#t.
Do you have any plans to send them for review?
Thanks,
Jian
On Tue, Mar 30, 2021 at 6:31 PM Jian Cai wrote:
>
> Thanks for all the information. I'll check for similar instances and
> send an updated version.
>
>
> On Tue, Mar 30, 2021 at 5:26 PM Guenter Roeck wrote:
> >
> > On 3/30/21 4:29 PM, Nathan Chancellor wrote:
> > > Hi Jian,
> > >
> > > On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
> > >> This fixes the mismatch of alignments between csd and its use as an
> > >> argument to smp_call_function_single_async, which causes build failure
> > >> when -Walign-mismatch in Clang is used.
> > >>
> > >> Link:
> > >> http://crrev.com/c/1193732
> > >>
> > >> Suggested-by: Guenter Roeck
> > >> Signed-off-by: Jian Cai
> > >
> > > Thanks for the patch. This is effectively a revert of commit
> > > 4ccafe032005 ("block: unalign call_single_data in struct request"),
> > > which I had brought up in this thread:
> > >
> > > https://lore.kernel.org/r/20210310182307.zzcbi5w5jrmveld4@archlinux-ax161/
> > >
> > > This is obviously a correct fix, I am not just sure what the impact to
> > > 'struct request' will be.
> > >
> >
> > As commit 4ccafe032005 states, it increases the request structure size.
> > Given the exchange referenced above, I think we'll need to disable
> > the warning in the block code.
> >
> > Thanks,
> > Guenter
> >
> > > Cheers,
> > > Nathan
> > >
> > >> ---
> > >> include/linux/blkdev.h | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > >> index bc6bc8383b43..3b92330d95ad 100644
> > >> --- a/include/linux/blkdev.h
> > >> +++ b/include/linux/blkdev.h
> > >> @@ -231,7 +231,7 @@ struct request {
> > >> unsigned long deadline;
> > >>
> > >> union {
> > >> -struct __call_single_data csd;
> > >> +call_single_data_t csd;
> > >> u64 fifo_time;
> > >> };
> > >>
> > >> --
> > >> 2.31.0.291.g576ba9dcdaf-goog
> > >>
> >
RE: [PATCH] blk-mq: fix alignment mismatch.
From: Nathan Chancellor
> Sent: 31 March 2021 00:30
>
> Hi Jian,
>
> On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
> > This fixes the mismatch of alignments between csd and its use as an
> > argument to smp_call_function_single_async, which causes build failure
> > when -Walign-mismatch in Clang is used.
> >
> > Link:
> > http://crrev.com/c/1193732
> >
> > Suggested-by: Guenter Roeck
> > Signed-off-by: Jian Cai
>
> Thanks for the patch. This is effectively a revert of commit
> 4ccafe032005 ("block: unalign call_single_data in struct request"),
> which I had brought up in this thread:
>
> https://lore.kernel.org/r/20210310182307.zzcbi5w5jrmveld4@archlinux-ax161/
>
> This is obviously a correct fix, I am not just sure what the impact to
> 'struct request' will be.
If the structure is allocated on-stack then aligning it
requires the compiler generate the rather horrid
'double stack frame' for the function.
Possibly the unaligned 'struct' should be used by all
the code except for a few places where it makes sense
to allocate an aligned item?
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT,
UK
Registration No: 1397386 (Wales)
Re: [PATCH] blk-mq: fix alignment mismatch.
Thanks for all the information. I'll check for similar instances and
send an updated version.
On Tue, Mar 30, 2021 at 5:26 PM Guenter Roeck wrote:
>
> On 3/30/21 4:29 PM, Nathan Chancellor wrote:
> > Hi Jian,
> >
> > On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
> >> This fixes the mismatch of alignments between csd and its use as an
> >> argument to smp_call_function_single_async, which causes build failure
> >> when -Walign-mismatch in Clang is used.
> >>
> >> Link:
> >> http://crrev.com/c/1193732
> >>
> >> Suggested-by: Guenter Roeck
> >> Signed-off-by: Jian Cai
> >
> > Thanks for the patch. This is effectively a revert of commit
> > 4ccafe032005 ("block: unalign call_single_data in struct request"),
> > which I had brought up in this thread:
> >
> > https://lore.kernel.org/r/20210310182307.zzcbi5w5jrmveld4@archlinux-ax161/
> >
> > This is obviously a correct fix, I am not just sure what the impact to
> > 'struct request' will be.
> >
>
> As commit 4ccafe032005 states, it increases the request structure size.
> Given the exchange referenced above, I think we'll need to disable
> the warning in the block code.
>
> Thanks,
> Guenter
>
> > Cheers,
> > Nathan
> >
> >> ---
> >> include/linux/blkdev.h | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> >> index bc6bc8383b43..3b92330d95ad 100644
> >> --- a/include/linux/blkdev.h
> >> +++ b/include/linux/blkdev.h
> >> @@ -231,7 +231,7 @@ struct request {
> >> unsigned long deadline;
> >>
> >> union {
> >> -struct __call_single_data csd;
> >> +call_single_data_t csd;
> >> u64 fifo_time;
> >> };
> >>
> >> --
> >> 2.31.0.291.g576ba9dcdaf-goog
> >>
>
Re: [PATCH] blk-mq: fix alignment mismatch.
On 3/30/21 4:29 PM, Nathan Chancellor wrote:
> Hi Jian,
>
> On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
>> This fixes the mismatch of alignments between csd and its use as an
>> argument to smp_call_function_single_async, which causes build failure
>> when -Walign-mismatch in Clang is used.
>>
>> Link:
>> http://crrev.com/c/1193732
>>
>> Suggested-by: Guenter Roeck
>> Signed-off-by: Jian Cai
>
> Thanks for the patch. This is effectively a revert of commit
> 4ccafe032005 ("block: unalign call_single_data in struct request"),
> which I had brought up in this thread:
>
> https://lore.kernel.org/r/20210310182307.zzcbi5w5jrmveld4@archlinux-ax161/
>
> This is obviously a correct fix, I am not just sure what the impact to
> 'struct request' will be.
>
As commit 4ccafe032005 states, it increases the request structure size.
Given the exchange referenced above, I think we'll need to disable
the warning in the block code.
Thanks,
Guenter
> Cheers,
> Nathan
>
>> ---
>> include/linux/blkdev.h | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
>> index bc6bc8383b43..3b92330d95ad 100644
>> --- a/include/linux/blkdev.h
>> +++ b/include/linux/blkdev.h
>> @@ -231,7 +231,7 @@ struct request {
>> unsigned long deadline;
>>
>> union {
>> -struct __call_single_data csd;
>> +call_single_data_t csd;
>> u64 fifo_time;
>> };
>>
>> --
>> 2.31.0.291.g576ba9dcdaf-goog
>>
Re: [PATCH] blk-mq: fix alignment mismatch.
Hi Jian,
On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
> This fixes the mismatch of alignments between csd and its use as an
> argument to smp_call_function_single_async, which causes build failure
> when -Walign-mismatch in Clang is used.
>
> Link:
> http://crrev.com/c/1193732
>
> Suggested-by: Guenter Roeck
> Signed-off-by: Jian Cai
Thanks for the patch. This is effectively a revert of commit
4ccafe032005 ("block: unalign call_single_data in struct request"),
which I had brought up in this thread:
https://lore.kernel.org/r/20210310182307.zzcbi5w5jrmveld4@archlinux-ax161/
This is obviously a correct fix, I am not just sure what the impact to
'struct request' will be.
Cheers,
Nathan
> ---
> include/linux/blkdev.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index bc6bc8383b43..3b92330d95ad 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -231,7 +231,7 @@ struct request {
> unsigned long deadline;
>
> union {
> - struct __call_single_data csd;
> + call_single_data_t csd;
> u64 fifo_time;
> };
>
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
Re: [PATCH] blk-mq: fix alignment mismatch.
On Tue, Mar 30, 2021 at 04:02:49PM -0700, Jian Cai wrote:
> This fixes the mismatch of alignments between csd and its use as an
> argument to smp_call_function_single_async, which causes build failure
> when -Walign-mismatch in Clang is used.
>
> Link:
> http://crrev.com/c/1193732
That link does not work. You probably meant
http://crbug.com/1193732
You'll probably want drop the link and to copy the relevant information
directly into the patch description. Generate the error with an upstream
kernel and cut-and-paste the output here.
For others, the observed error message is:
block/blk-mq.c:622:44: error: passing 8-byte aligned argument to 32-byte
aligned parameter 2 of 'smp_call_function_single_async' may result
in an unaligned pointer access [-Werror,-Walign-mismatch]
Thanks,
Guenter
>
> Suggested-by: Guenter Roeck
> Signed-off-by: Jian Cai
> ---
> include/linux/blkdev.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index bc6bc8383b43..3b92330d95ad 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -231,7 +231,7 @@ struct request {
> unsigned long deadline;
>
> union {
> - struct __call_single_data csd;
> + call_single_data_t csd;
> u64 fifo_time;
> };
>
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
[PATCH] blk-mq: fix alignment mismatch.
This fixes the mismatch of alignments between csd and its use as an
argument to smp_call_function_single_async, which causes build failure
when -Walign-mismatch in Clang is used.
Link:
http://crrev.com/c/1193732
Suggested-by: Guenter Roeck
Signed-off-by: Jian Cai
---
include/linux/blkdev.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index bc6bc8383b43..3b92330d95ad 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -231,7 +231,7 @@ struct request {
unsigned long deadline;
union {
- struct __call_single_data csd;
+ call_single_data_t csd;
u64 fifo_time;
};
--
2.31.0.291.g576ba9dcdaf-goog
