| Message ID | 20201101173105.1723648-1-nivedita@alum.mit.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | compiler.h: Move barrier() back into compiler-*.h | expand |
On Sun, Nov 01, 2020 at 12:31:05PM -0500, Arvind Sankar wrote: > Commit > b9de06783f01 ("compiler.h: fix barrier_data() on clang") > moved the definition of barrier() into compiler.h. That's not a real commit ID. It only exists in linux-next and will expire after a few weeks. The right way to fix a patch in Andrew's tree is to send an email asking him to apply it as a -fix patch. As part of Andrew's submission process, he folds all the -fix patches into the parent patch and it shows up pristine in Linus' tree. > This causes build failures at least on alpha, because there are files > that rely on barrier() being defined via the implicit include of > compiler_types.h. That seems like a bug that should be fixed rather than reverting this part of the patch?
On 11/1/20 9:38 AM, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 12:31:05PM -0500, Arvind Sankar wrote: >> Commit >> b9de06783f01 ("compiler.h: fix barrier_data() on clang") >> moved the definition of barrier() into compiler.h. > > That's not a real commit ID. It only exists in linux-next and > will expire after a few weeks. > > The right way to fix a patch in Andrew's tree is to send an email > asking him to apply it as a -fix patch. As part of Andrew's submission > process, he folds all the -fix patches into the parent patch and it > shows up pristine in Linus' tree. > >> This causes build failures at least on alpha, because there are files >> that rely on barrier() being defined via the implicit include of >> compiler_types.h. > > That seems like a bug that should be fixed rather than reverting this > part of the patch? > maybe: ? https://lore.kernel.org/lkml/20201101030159.15858-1-rdunlap@infradead.org/
On Sun, Nov 01, 2020 at 09:48:30AM -0800, Randy Dunlap wrote: > maybe: ? > > https://lore.kernel.org/lkml/20201101030159.15858-1-rdunlap@infradead.org/ Yes
On Sun, Nov 01, 2020 at 05:38:35PM +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 12:31:05PM -0500, Arvind Sankar wrote: > > Commit > > b9de06783f01 ("compiler.h: fix barrier_data() on clang") > > moved the definition of barrier() into compiler.h. > > That's not a real commit ID. It only exists in linux-next and > will expire after a few weeks. > > The right way to fix a patch in Andrew's tree is to send an email > asking him to apply it as a -fix patch. As part of Andrew's submission > process, he folds all the -fix patches into the parent patch and it > shows up pristine in Linus' tree. Ok. So I still send it as a separate patch and he does the folding, or should I send a revised patch that replaces the original one? > > > This causes build failures at least on alpha, because there are files > > that rely on barrier() being defined via the implicit include of > > compiler_types.h. > > That seems like a bug that should be fixed rather than reverting this > part of the patch? > I thought about that, but barrier() is used in a large number of places, so reverting seemed like the safer approach, in case more errors like this cropped up. I don't know if it's a bug, as it was defined in compiler_types.h before my original patch, I'm not sure if there's a reason only compiler_types.h rather than compiler.h is implicitly included via the Makefile. Cc'ing Masahiro Yamada in case he has comments on that part. Thanks.
On Sun, Nov 01, 2020 at 02:51:10PM -0500, Arvind Sankar wrote: > Ok. So I still send it as a separate patch and he does the folding, or > should I send a revised patch that replaces the original one? I think Randy's patch should be merged instead of this patch.
On Sun, Nov 01, 2020 at 07:52:15PM +0000, Matthew Wilcox wrote: > On Sun, Nov 01, 2020 at 02:51:10PM -0500, Arvind Sankar wrote: > > Ok. So I still send it as a separate patch and he does the folding, or > > should I send a revised patch that replaces the original one? > > I think Randy's patch should be merged instead of this patch. Ok, if that one works then it's better I agree.
On 11/1/20 11:59 AM, Arvind Sankar wrote: > On Sun, Nov 01, 2020 at 07:52:15PM +0000, Matthew Wilcox wrote: >> On Sun, Nov 01, 2020 at 02:51:10PM -0500, Arvind Sankar wrote: >>> Ok. So I still send it as a separate patch and he does the folding, or >>> should I send a revised patch that replaces the original one? >> >> I think Randy's patch should be merged instead of this patch. > > Ok, if that one works then it's better I agree. > Do I need to resend it to Andrew? -- ~Randy
Hi all, On Sun, 1 Nov 2020 17:38:35 +0000 Matthew Wilcox <willy@infradead.org> wrote: > > On Sun, Nov 01, 2020 at 12:31:05PM -0500, Arvind Sankar wrote: > > Commit > > b9de06783f01 ("compiler.h: fix barrier_data() on clang") > > moved the definition of barrier() into compiler.h. > > That's not a real commit ID. It only exists in linux-next and > will expire after a few weeks. Which also means that the Cc: <stable@vger.kernel.org> is also unnecessary.
Hi Randy, On Sun, 1 Nov 2020 12:40:19 -0800 Randy Dunlap <rdunlap@infradead.org> wrote: > > On 11/1/20 11:59 AM, Arvind Sankar wrote: > > On Sun, Nov 01, 2020 at 07:52:15PM +0000, Matthew Wilcox wrote: > >> On Sun, Nov 01, 2020 at 02:51:10PM -0500, Arvind Sankar wrote: > >>> Ok. So I still send it as a separate patch and he does the folding, or > >>> should I send a revised patch that replaces the original one? > >> > >> I think Randy's patch should be merged instead of this patch. > > > > Ok, if that one works then it's better I agree. > > > > Do I need to resend it to Andrew? Well, his SOB is on the original patch (as is mine) ... so, yes, please.
diff --git a/include/linux/compiler-clang.h b/include/linux/compiler-clang.h index dd7233c48bf3..230604e7f057 100644 --- a/include/linux/compiler-clang.h +++ b/include/linux/compiler-clang.h @@ -60,6 +60,12 @@ #define COMPILER_HAS_GENERIC_BUILTIN_OVERFLOW 1 #endif +/* The following are for compatibility with GCC, from compiler-gcc.h, + * and may be redefined here because they should not be shared with other + * compilers, like ICC. + */ +#define barrier() __asm__ __volatile__("" : : : "memory") + #if __has_feature(shadow_call_stack) # define __noscs __attribute__((__no_sanitize__("shadow-call-stack"))) #endif diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h index 50912ed00278..a572965c801a 100644 --- a/include/linux/compiler-gcc.h +++ b/include/linux/compiler-gcc.h @@ -15,6 +15,11 @@ # error Sorry, your version of GCC is too old - please use 4.9 or newer. #endif +/* Optimization barrier */ + +/* The "volatile" is due to gcc bugs */ +#define barrier() __asm__ __volatile__("": : :"memory") + /* * This macro obfuscates arithmetic on a variable address so that gcc * shouldn't recognize the original var, and make assumptions about it. diff --git a/include/linux/compiler.h b/include/linux/compiler.h index b8fe0c23cfff..25c803f4222f 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -80,8 +80,7 @@ void ftrace_likely_update(struct ftrace_likely_data *f, int val, /* Optimization barrier */ #ifndef barrier -/* The "volatile" is due to gcc bugs */ -# define barrier() __asm__ __volatile__("": : :"memory") +# define barrier() __memory_barrier() #endif #ifndef barrier_data
Commit b9de06783f01 ("compiler.h: fix barrier_data() on clang") moved the definition of barrier() into compiler.h. This causes build failures at least on alpha, because there are files that rely on barrier() being defined via the implicit include of compiler_types.h. Revert this portion of the commit to fix. Link: https://lore.kernel.org/linux-mm/202010312104.Dk9VQJYb-lkp@intel.com/ Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu> Fixes: b9de06783f01 ("compiler.h: fix barrier_data() on clang") Cc: <stable@vger.kernel.org> --- include/linux/compiler-clang.h | 6 ++++++ include/linux/compiler-gcc.h | 5 +++++ include/linux/compiler.h | 3 +-- 3 files changed, 12 insertions(+), 2 deletions(-)