* [PATCH v2 0/2] x86: Asm macros fixes @ 2018-11-14 20:43 Nadav Amit 2018-11-14 20:43 ` [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros Nadav Amit 2018-11-14 20:43 ` [PATCH v2 2/2] x86: set a dependency on macros.S Nadav Amit 0 siblings, 2 replies; 10+ messages in thread From: Nadav Amit @ 2018-11-14 20:43 UTC (permalink / raw) To: Masahiro Yamada, Ingo Molnar Cc: Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, x86, linux-kbuild, linux-kernel, Nadav Amit There has been a complaint that the recent use of assembly macros in C files broke distcc. The first patch fixes this issue. The second patch adds a dependency for all C files on macros.S, to trigger their recompilation when the relevant macros change. I think it will be better to get the patches through tip. Please advise if you disagree. v1->v2: * Remove whitespaces [Ingo] * Automatically enable split compilation when distcc or icecc are used Nadav Amit (2): Makefile: Fix distcc compilation with x86 macros x86: set a dependency on macros.S Makefile | 4 +++- arch/x86/Makefile | 7 +++++-- scripts/Makefile.build | 34 ++++++++++++++++++++++++++++++---- 3 files changed, 38 insertions(+), 7 deletions(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros 2018-11-14 20:43 [PATCH v2 0/2] x86: Asm macros fixes Nadav Amit @ 2018-11-14 20:43 ` Nadav Amit 2018-11-16 7:45 ` Masahiro Yamada 2018-11-14 20:43 ` [PATCH v2 2/2] x86: set a dependency on macros.S Nadav Amit 1 sibling, 1 reply; 10+ messages in thread From: Nadav Amit @ 2018-11-14 20:43 UTC (permalink / raw) To: Masahiro Yamada, Ingo Molnar Cc: Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, x86, linux-kbuild, linux-kernel, Nadav Amit Introducing the use of asm macros in c-code broke distcc, since it only sends the preprocessed source file. The solution is to break the compilation into two separate phases of compilation and assembly, and between the two concatenate the assembly macros and the compiled (yet not assembled) source file. Since this is less efficient, this compilation mode is only used when distcc or icecc are used. Note that the assembly stage should also be distributed, if distcc is configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE". Reported-by: Logan Gunthorpe <logang@deltatee.com> Signed-off-by: Nadav Amit <namit@vmware.com> --- Makefile | 4 +++- arch/x86/Makefile | 7 +++++-- scripts/Makefile.build | 30 ++++++++++++++++++++++++++++-- 3 files changed, 36 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 9fce8b91c15f..c07349fc38c7 100644 --- a/Makefile +++ b/Makefile @@ -743,7 +743,9 @@ KBUILD_CFLAGS += $(call cc-option, -gsplit-dwarf, -g) else KBUILD_CFLAGS += -g endif -KBUILD_AFLAGS += -Wa,-gdwarf-2 +AFLAGS_DEBUG_INFO = -Wa,-gdwarf-2 +export AFLAGS_DEBUG_INFO +KBUILD_AFLAGS += $(AFLAGS_DEBUG_INFO) endif ifdef CONFIG_DEBUG_INFO_DWARF4 KBUILD_CFLAGS += $(call cc-option, -gdwarf-4,) diff --git a/arch/x86/Makefile b/arch/x86/Makefile index f5d7f4134524..b5953cbcc9c8 100644 --- a/arch/x86/Makefile +++ b/arch/x86/Makefile @@ -235,10 +235,13 @@ archscripts: scripts_basic archheaders: $(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all +ASM_MACRO_FILE = arch/x86/kernel/macros.s +export ASM_MACRO_FILE + archmacros: - $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s + $(Q)$(MAKE) $(build)=arch/x86/kernel $(ASM_MACRO_FILE) -ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s +ASM_MACRO_FLAGS = -Wa,$(ASM_MACRO_FILE) export ASM_MACRO_FLAGS KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) diff --git a/scripts/Makefile.build b/scripts/Makefile.build index 6a6be9f440cf..b8d26bdf48b0 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -155,8 +155,34 @@ $(obj)/%.ll: $(src)/%.c FORCE quiet_cmd_cc_o_c = CC $(quiet_modtag) $@ +# If distcc (or icecc) are used, and when assembly macro files are needed, the +# compilation stage and the assembly stage needs to be separated. Providing the +# "IGNORE_DISTCC=y" option disables separate compilation and assembly. + +cmd_cc_o_c_direct = $(CC) $(c_flags) -c -o $(1) $< + +ifneq ($(if $(IGNORE_DISTCC),,$(shell $(CC) --version 2>&1 | head -n 1 | grep -E 'distcc|ICECC')),) +a_flags_no_debug = $(filter-out $(AFLAGS_DEBUG_INFO), $(a_flags)) +c_flags_no_macros = $(filter-out $(ASM_MACRO_FLAGS), $(c_flags)) + +cmd_cc_o_c_two_steps = \ + $(CC) $(c_flags_no_macros) $(DISABLE_LTO) -fverbose-asm -S \ + -o $(@D)/.$(@F:.o=.s) $<; \ + cat $(ASM_MACRO_FILE) $(@D)/.$(@F:.o=.s) > \ + $(@D)/.tmp_$(@F:.o=.s); \ + $(CC) $(a_flags_no_debug) -c -o $(1) $(@D)/.tmp_$(@F:.o=.s); \ + rm -f $(@D)/.$(@F:.o=.s) $(@D)/.tmp_$(@F:.o=.s) \ + +cmd_cc_o_c_helper = \ + $(if $(findstring $(ASM_MACRO_FLAGS),$(c_flags)), \ + $(call cmd_cc_o_c_two_steps, $(1)), \ + $(call cmd_cc_o_c_direct, $(1))) +else +cmd_cc_o_c_helper = $(call cmd_cc_o_c_direct, $(1)) +endif + ifndef CONFIG_MODVERSIONS -cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< +cmd_cc_o_c = $(call cmd_cc_o_c_helper,$@) else # When module versioning is enabled the following steps are executed: @@ -171,7 +197,7 @@ else # replace the unresolved symbols __crc_exported_symbol with # the actual value of the checksum generated by genksyms -cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $< +cmd_cc_o_c = $(call cmd_cc_o_c_helper,$(@D)/.tmp_$(@F)) cmd_modversions_c = \ if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \ -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros 2018-11-14 20:43 ` [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros Nadav Amit @ 2018-11-16 7:45 ` Masahiro Yamada 2018-11-16 21:01 ` Nadav Amit 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2018-11-16 7:45 UTC (permalink / raw) To: Nadav Amit Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: > > Introducing the use of asm macros in c-code broke distcc, since it only > sends the preprocessed source file. The solution is to break the > compilation into two separate phases of compilation and assembly, and > between the two concatenate the assembly macros and the compiled (yet > not assembled) source file. Since this is less efficient, this > compilation mode is only used when distcc or icecc are used. > > Note that the assembly stage should also be distributed, if distcc is > configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE". > > Reported-by: Logan Gunthorpe <logang@deltatee.com> > Signed-off-by: Nadav Amit <namit@vmware.com> Wow, this is so ugly. I realized how much I hated this by now. My question is, how long do we need to carry this? As far as I understood from the long discussion https://lkml.org/lkml/2018/10/7/92 people are trying to deal with it on the compiler side. Is it right? https://gcc.gnu.org/ml/gcc-patches/2018-10/msg01932.html Once it is supported, what would happen on the kernel side? > --- > Makefile | 4 +++- > arch/x86/Makefile | 7 +++++-- > scripts/Makefile.build | 30 ++++++++++++++++++++++++++++-- > 3 files changed, 36 insertions(+), 5 deletions(-) > > diff --git a/Makefile b/Makefile > index 9fce8b91c15f..c07349fc38c7 100644 > --- a/Makefile > +++ b/Makefile > @@ -743,7 +743,9 @@ KBUILD_CFLAGS += $(call cc-option, -gsplit-dwarf, -g) > else > KBUILD_CFLAGS += -g > endif > -KBUILD_AFLAGS += -Wa,-gdwarf-2 > +AFLAGS_DEBUG_INFO = -Wa,-gdwarf-2 > +export AFLAGS_DEBUG_INFO > +KBUILD_AFLAGS += $(AFLAGS_DEBUG_INFO) > endif > ifdef CONFIG_DEBUG_INFO_DWARF4 > KBUILD_CFLAGS += $(call cc-option, -gdwarf-4,) > diff --git a/arch/x86/Makefile b/arch/x86/Makefile > index f5d7f4134524..b5953cbcc9c8 100644 > --- a/arch/x86/Makefile > +++ b/arch/x86/Makefile > @@ -235,10 +235,13 @@ archscripts: scripts_basic > archheaders: > $(Q)$(MAKE) $(build)=arch/x86/entry/syscalls all > > +ASM_MACRO_FILE = arch/x86/kernel/macros.s > +export ASM_MACRO_FILE > + > archmacros: > - $(Q)$(MAKE) $(build)=arch/x86/kernel arch/x86/kernel/macros.s > + $(Q)$(MAKE) $(build)=arch/x86/kernel $(ASM_MACRO_FILE) > > -ASM_MACRO_FLAGS = -Wa,arch/x86/kernel/macros.s > +ASM_MACRO_FLAGS = -Wa,$(ASM_MACRO_FILE) > export ASM_MACRO_FLAGS > KBUILD_CFLAGS += $(ASM_MACRO_FLAGS) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index 6a6be9f440cf..b8d26bdf48b0 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -155,8 +155,34 @@ $(obj)/%.ll: $(src)/%.c FORCE > > quiet_cmd_cc_o_c = CC $(quiet_modtag) $@ > > +# If distcc (or icecc) are used, and when assembly macro files are needed, the > +# compilation stage and the assembly stage needs to be separated. Providing the > +# "IGNORE_DISTCC=y" option disables separate compilation and assembly. > + > +cmd_cc_o_c_direct = $(CC) $(c_flags) -c -o $(1) $< > + > +ifneq ($(if $(IGNORE_DISTCC),,$(shell $(CC) --version 2>&1 | head -n 1 | grep -E 'distcc|ICECC')),) > +a_flags_no_debug = $(filter-out $(AFLAGS_DEBUG_INFO), $(a_flags)) > +c_flags_no_macros = $(filter-out $(ASM_MACRO_FLAGS), $(c_flags)) > + > +cmd_cc_o_c_two_steps = \ > + $(CC) $(c_flags_no_macros) $(DISABLE_LTO) -fverbose-asm -S \ > + -o $(@D)/.$(@F:.o=.s) $<; \ > + cat $(ASM_MACRO_FILE) $(@D)/.$(@F:.o=.s) > \ > + $(@D)/.tmp_$(@F:.o=.s); \ > + $(CC) $(a_flags_no_debug) -c -o $(1) $(@D)/.tmp_$(@F:.o=.s); \ > + rm -f $(@D)/.$(@F:.o=.s) $(@D)/.tmp_$(@F:.o=.s) \ > + > +cmd_cc_o_c_helper = \ > + $(if $(findstring $(ASM_MACRO_FLAGS),$(c_flags)), \ > + $(call cmd_cc_o_c_two_steps, $(1)), \ > + $(call cmd_cc_o_c_direct, $(1))) > +else > +cmd_cc_o_c_helper = $(call cmd_cc_o_c_direct, $(1)) > +endif > + > ifndef CONFIG_MODVERSIONS > -cmd_cc_o_c = $(CC) $(c_flags) -c -o $@ $< > +cmd_cc_o_c = $(call cmd_cc_o_c_helper,$@) > > else > # When module versioning is enabled the following steps are executed: > @@ -171,7 +197,7 @@ else > # replace the unresolved symbols __crc_exported_symbol with > # the actual value of the checksum generated by genksyms > > -cmd_cc_o_c = $(CC) $(c_flags) -c -o $(@D)/.tmp_$(@F) $< > +cmd_cc_o_c = $(call cmd_cc_o_c_helper,$(@D)/.tmp_$(@F)) > > cmd_modversions_c = \ > if $(OBJDUMP) -h $(@D)/.tmp_$(@F) | grep -q __ksymtab; then \ > -- > 2.17.1 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros 2018-11-16 7:45 ` Masahiro Yamada @ 2018-11-16 21:01 ` Nadav Amit 2018-11-18 4:20 ` Masahiro Yamada 0 siblings, 1 reply; 10+ messages in thread From: Nadav Amit @ 2018-11-16 21:01 UTC (permalink / raw) To: Masahiro Yamada Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List From: Masahiro Yamada Sent: November 16, 2018 at 7:45:45 AM GMT > To: Nadav Amit <namit@vmware.com> > Cc: Ingo Molnar <mingo@redhat.com>, Michal Marek <michal.lkml@markovi.net>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, H. Peter Anvin <hpa@zytor.com>, X86 ML <x86@kernel.org>, Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros > > > On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: >> Introducing the use of asm macros in c-code broke distcc, since it only >> sends the preprocessed source file. The solution is to break the >> compilation into two separate phases of compilation and assembly, and >> between the two concatenate the assembly macros and the compiled (yet >> not assembled) source file. Since this is less efficient, this >> compilation mode is only used when distcc or icecc are used. >> >> Note that the assembly stage should also be distributed, if distcc is >> configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE". >> >> Reported-by: Logan Gunthorpe <logang@deltatee.com> >> Signed-off-by: Nadav Amit <namit@vmware.com> > > > Wow, this is so ugly. Indeed, it is not pretty from the Makefile system point of view. But it does have merits when it comes to the actual use (by developers). If you look on x86, there are a lot of duplicated implementation for C and Asm and crazy C macros, for example for PV function call or the alternative mechanism. The code is also less readable in its current form. > I realized how much I hated this by now. > > My question is, how long do we need to carry this? If it is purely about performance, I am not sure it would make sense to put it in, as it will indeed be (eventually) solved by the compiler, and the penalty is not too great. There is also an advantage for having assembly macros that can override the generated assembly that was generated by the compiler. I think HPA (cc’d) looks in this direction (and so do I). Regards, Nadav ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros 2018-11-16 21:01 ` Nadav Amit @ 2018-11-18 4:20 ` Masahiro Yamada 2018-11-19 17:18 ` Nadav Amit 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2018-11-18 4:20 UTC (permalink / raw) To: Nadav Amit Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List On Sat, Nov 17, 2018 at 6:02 AM Nadav Amit <namit@vmware.com> wrote: > > From: Masahiro Yamada > Sent: November 16, 2018 at 7:45:45 AM GMT > > To: Nadav Amit <namit@vmware.com> > > Cc: Ingo Molnar <mingo@redhat.com>, Michal Marek <michal.lkml@markovi.net>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, H. Peter Anvin <hpa@zytor.com>, X86 ML <x86@kernel.org>, Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros > > > > > > On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: > >> Introducing the use of asm macros in c-code broke distcc, since it only > >> sends the preprocessed source file. The solution is to break the > >> compilation into two separate phases of compilation and assembly, and > >> between the two concatenate the assembly macros and the compiled (yet > >> not assembled) source file. Since this is less efficient, this > >> compilation mode is only used when distcc or icecc are used. > >> > >> Note that the assembly stage should also be distributed, if distcc is > >> configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE". > >> > >> Reported-by: Logan Gunthorpe <logang@deltatee.com> > >> Signed-off-by: Nadav Amit <namit@vmware.com> > > > > > > Wow, this is so ugly. > > Indeed, it is not pretty from the Makefile system point of view. > > But it does have merits when it comes to the actual use (by developers). If > you look on x86, there are a lot of duplicated implementation for C and Asm > and crazy C macros, for example for PV function call or the alternative > mechanism. The code is also less readable in its current form. > > > I realized how much I hated this by now. > > > > My question is, how long do we need to carry this? > > If it is purely about performance, I am not sure it would make sense to > put it in, as it will indeed be (eventually) solved by the compiler, and > the penalty is not too great. It is unfortunate to mess up the code by having two different ways to achieve the same goal. When GCC with asm inline support is shipped, would you revert 77b0bf55 ? > There is also an advantage for having assembly macros that can override the > generated assembly that was generated by the compiler. I think HPA (cc’d) > looks in this direction (and so do I). > > Regards, > Nadav -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros 2018-11-18 4:20 ` Masahiro Yamada @ 2018-11-19 17:18 ` Nadav Amit 2018-11-21 2:47 ` Masahiro Yamada 0 siblings, 1 reply; 10+ messages in thread From: Nadav Amit @ 2018-11-19 17:18 UTC (permalink / raw) To: Masahiro Yamada Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List at 8:20 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > On Sat, Nov 17, 2018 at 6:02 AM Nadav Amit <namit@vmware.com> wrote: >> From: Masahiro Yamada >> Sent: November 16, 2018 at 7:45:45 AM GMT >>> To: Nadav Amit <namit@vmware.com> >>> Cc: Ingo Molnar <mingo@redhat.com>, Michal Marek <michal.lkml@markovi.net>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, H. Peter Anvin <hpa@zytor.com>, X86 ML <x86@kernel.org>, Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> >>> Subject: Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros >>> >>> >>> On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: >>>> Introducing the use of asm macros in c-code broke distcc, since it only >>>> sends the preprocessed source file. The solution is to break the >>>> compilation into two separate phases of compilation and assembly, and >>>> between the two concatenate the assembly macros and the compiled (yet >>>> not assembled) source file. Since this is less efficient, this >>>> compilation mode is only used when distcc or icecc are used. >>>> >>>> Note that the assembly stage should also be distributed, if distcc is >>>> configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE". >>>> >>>> Reported-by: Logan Gunthorpe <logang@deltatee.com> >>>> Signed-off-by: Nadav Amit <namit@vmware.com> >>> >>> >>> Wow, this is so ugly. >> >> Indeed, it is not pretty from the Makefile system point of view. >> >> But it does have merits when it comes to the actual use (by developers). If >> you look on x86, there are a lot of duplicated implementation for C and Asm >> and crazy C macros, for example for PV function call or the alternative >> mechanism. The code is also less readable in its current form. >> >>> I realized how much I hated this by now. >>> >>> My question is, how long do we need to carry this? >> >> If it is purely about performance, I am not sure it would make sense to >> put it in, as it will indeed be (eventually) solved by the compiler, and >> the penalty is not too great. > > > It is unfortunate to mess up the code > by having two different ways to achieve the same goal. Indeed, but I fail to see how this statement fits with the next sentence. > When GCC with asm inline support is shipped, > would you revert 77b0bf55 ? This patch and the following ones were not written to be reverted, i.e., reverting them later may not be easy since they remove redundant C inline assembly chunks. Since gcc will solve the inline assembly issues, the value of these patches is in unifying the assembly that is used in .c and .S files. Due to the lack of m4-like preprocessor in the Linux make-system, the solution is a bit “hacky” (in other words, I don’t see a reasonable alternative solution). So there is a downside in the form of performance degradation of distcc, and hacky (not too hacky?) Makefile changes. On the upside, except addressing the gcc statement cost computation (inline) issue, the patch removes redundant code, and improves assembly code readability. In addition, it provides the option to make assembly manipulations after compilation, which HPA and others (me) look into. \x10Anyhow, if after all, the downside is considered greater than the upside, it is best to remove the patches sooner than later, as a revert later will be more painful. Regards, Nadav ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros 2018-11-19 17:18 ` Nadav Amit @ 2018-11-21 2:47 ` Masahiro Yamada 0 siblings, 0 replies; 10+ messages in thread From: Masahiro Yamada @ 2018-11-21 2:47 UTC (permalink / raw) To: Nadav Amit Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List On Tue, Nov 20, 2018 at 2:21 AM Nadav Amit <namit@vmware.com> wrote: > > at 8:20 PM, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > On Sat, Nov 17, 2018 at 6:02 AM Nadav Amit <namit@vmware.com> wrote: > >> From: Masahiro Yamada > >> Sent: November 16, 2018 at 7:45:45 AM GMT > >>> To: Nadav Amit <namit@vmware.com> > >>> Cc: Ingo Molnar <mingo@redhat.com>, Michal Marek <michal.lkml@markovi.net>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, H. Peter Anvin <hpa@zytor.com>, X86 ML <x86@kernel.org>, Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > >>> Subject: Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros > >>> > >>> > >>> On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: > >>>> Introducing the use of asm macros in c-code broke distcc, since it only > >>>> sends the preprocessed source file. The solution is to break the > >>>> compilation into two separate phases of compilation and assembly, and > >>>> between the two concatenate the assembly macros and the compiled (yet > >>>> not assembled) source file. Since this is less efficient, this > >>>> compilation mode is only used when distcc or icecc are used. > >>>> > >>>> Note that the assembly stage should also be distributed, if distcc is > >>>> configured using "CFLAGS=-DENABLE_REMOTE_ASSEMBLE". > >>>> > >>>> Reported-by: Logan Gunthorpe <logang@deltatee.com> > >>>> Signed-off-by: Nadav Amit <namit@vmware.com> > >>> > >>> > >>> Wow, this is so ugly. > >> > >> Indeed, it is not pretty from the Makefile system point of view. > >> > >> But it does have merits when it comes to the actual use (by developers). If > >> you look on x86, there are a lot of duplicated implementation for C and Asm > >> and crazy C macros, for example for PV function call or the alternative > >> mechanism. The code is also less readable in its current form. > >> > >>> I realized how much I hated this by now. > >>> > >>> My question is, how long do we need to carry this? > >> > >> If it is purely about performance, I am not sure it would make sense to > >> put it in, as it will indeed be (eventually) solved by the compiler, and > >> the penalty is not too great. > > > > > > It is unfortunate to mess up the code > > by having two different ways to achieve the same goal. > > Indeed, but I fail to see how this statement fits with the next sentence. I am not tracking the progress on GCC side. I just did not sure if the 'asm inline' work was on the right track. > > When GCC with asm inline support is shipped, > > would you revert 77b0bf55 ? > > This patch and the following ones were not written to be reverted, i.e., > reverting them later may not be easy since they remove redundant C inline > assembly chunks. > > Since gcc will solve the inline assembly issues, the value of these patches > is in unifying the assembly that is used in .c and .S files. Due to the lack > of m4-like preprocessor in the Linux make-system, the solution is a bit > “hacky” (in other words, I don’t see a reasonable alternative solution). > > So there is a downside in the form of performance degradation of distcc, and > hacky (not too hacky?) Makefile changes. On the upside, except addressing > the gcc statement cost computation (inline) issue, the patch removes > redundant code, and improves assembly code readability. It is true that the assembly part itself is readable but the readability as a whole is worse IMHO. Each macro implementation was split into "asm volatile" (in C) + ".macro" (in ASM) parts. > In addition, it > provides the option to make assembly manipulations after compilation, which > HPA and others (me) look into. > > Anyhow, if after all, the downside is considered greater than the upside, > it is best to remove the patches sooner than later, as a revert later will > be more painful. Then, I'd be happier with the revert now. -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 2/2] x86: set a dependency on macros.S 2018-11-14 20:43 [PATCH v2 0/2] x86: Asm macros fixes Nadav Amit 2018-11-14 20:43 ` [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros Nadav Amit @ 2018-11-14 20:43 ` Nadav Amit 2018-11-16 7:37 ` Masahiro Yamada 1 sibling, 1 reply; 10+ messages in thread From: Nadav Amit @ 2018-11-14 20:43 UTC (permalink / raw) To: Masahiro Yamada, Ingo Molnar Cc: Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, x86, linux-kbuild, linux-kernel, Nadav Amit Changes in macros.S should trigger the recompilation of all C files, as the macros might need to affect their compilation. Acked-by: Ingo Molnar <mingo@kernel.org> Signed-off-by: Nadav Amit <namit@vmware.com> --- scripts/Makefile.build | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/Makefile.build b/scripts/Makefile.build index b8d26bdf48b0..efec77991c2b 100644 --- a/scripts/Makefile.build +++ b/scripts/Makefile.build @@ -313,13 +313,13 @@ cmd_undef_syms = echo endif # Built-in and composite module parts -$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE +$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) $(ASM_MACRO_FILE:.s=.S) FORCE $(call cmd,force_checksrc) $(call if_changed_rule,cc_o_c) # Single-part modules are special since we need to mark them in $(MODVERDIR) -$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE +$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) $(ASM_MACRO_FILE:.s=.S) FORCE $(call cmd,force_checksrc) $(call if_changed_rule,cc_o_c) @{ echo $(@:.o=.ko); echo $@; \ -- 2.17.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] x86: set a dependency on macros.S 2018-11-14 20:43 ` [PATCH v2 2/2] x86: set a dependency on macros.S Nadav Amit @ 2018-11-16 7:37 ` Masahiro Yamada 2018-11-16 20:18 ` Nadav Amit 0 siblings, 1 reply; 10+ messages in thread From: Masahiro Yamada @ 2018-11-16 7:37 UTC (permalink / raw) To: Nadav Amit Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: > > Changes in macros.S should trigger the recompilation of all C files, as > the macros might need to affect their compilation. > > Acked-by: Ingo Molnar <mingo@kernel.org> > Signed-off-by: Nadav Amit <namit@vmware.com> > --- When we talked about this last time, we agreed to not do this because a single line change in asm headers would cause global rebuilding. Did you change your mind? > scripts/Makefile.build | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index b8d26bdf48b0..efec77991c2b 100644 > --- a/scripts/Makefile.build > +++ b/scripts/Makefile.build > @@ -313,13 +313,13 @@ cmd_undef_syms = echo > endif > > # Built-in and composite module parts > -$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE > +$(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) $(ASM_MACRO_FILE:.s=.S) FORCE > $(call cmd,force_checksrc) > $(call if_changed_rule,cc_o_c) > > # Single-part modules are special since we need to mark them in $(MODVERDIR) > > -$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) FORCE > +$(single-used-m): $(obj)/%.o: $(src)/%.c $(recordmcount_source) $(objtool_dep) $(ASM_MACRO_FILE:.s=.S) FORCE > $(call cmd,force_checksrc) > $(call if_changed_rule,cc_o_c) > @{ echo $(@:.o=.ko); echo $@; \ > -- > 2.17.1 > -- Best Regards Masahiro Yamada ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/2] x86: set a dependency on macros.S 2018-11-16 7:37 ` Masahiro Yamada @ 2018-11-16 20:18 ` Nadav Amit 0 siblings, 0 replies; 10+ messages in thread From: Nadav Amit @ 2018-11-16 20:18 UTC (permalink / raw) To: Masahiro Yamada Cc: Ingo Molnar, Michal Marek, Thomas Gleixner, Borislav Petkov, H. Peter Anvin, X86 ML, Linux Kbuild mailing list, Linux Kernel Mailing List From: Masahiro Yamada Sent: November 16, 2018 at 7:37:46 AM GMT > To: Nadav Amit <namit@vmware.com> > Cc: Ingo Molnar <mingo@redhat.com>, Michal Marek <michal.lkml@markovi.net>, Thomas Gleixner <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>, H. Peter Anvin <hpa@zytor.com>, X86 ML <x86@kernel.org>, Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH v2 2/2] x86: set a dependency on macros.S > > > On Thu, Nov 15, 2018 at 1:01 PM Nadav Amit <namit@vmware.com> wrote: >> Changes in macros.S should trigger the recompilation of all C files, as >> the macros might need to affect their compilation. >> >> Acked-by: Ingo Molnar <mingo@kernel.org> >> Signed-off-by: Nadav Amit <namit@vmware.com> >> --- > > When we talked about this last time, > we agreed to not do this > because a single line change in asm headers > would cause global rebuilding. > > Did you change your mind? I was wrong (and you were right? I don’t quite remember). Anyhow, another patch I work on made me realize how wrong I was. If required, we can extract the macros from some of the header files into separate header files to prevent unnecessary rebuilds. Thanks, Nadav ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-11-21 2:48 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-11-14 20:43 [PATCH v2 0/2] x86: Asm macros fixes Nadav Amit 2018-11-14 20:43 ` [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros Nadav Amit 2018-11-16 7:45 ` Masahiro Yamada 2018-11-16 21:01 ` Nadav Amit 2018-11-18 4:20 ` Masahiro Yamada 2018-11-19 17:18 ` Nadav Amit 2018-11-21 2:47 ` Masahiro Yamada 2018-11-14 20:43 ` [PATCH v2 2/2] x86: set a dependency on macros.S Nadav Amit 2018-11-16 7:37 ` Masahiro Yamada 2018-11-16 20:18 ` Nadav Amit
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).