[PATCH v2 0/2] x86: Asm macros fixes

[PATCH v2 0/2] x86: Asm macros fixes

[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

[PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros

[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

[PATCH v2 2/2] x86: set a dependency on macros.S

[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

Re: [PATCH v2 2/2] x86: set a dependency on macros.S

[Masahiro Yamada]

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

Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros

[Masahiro Yamada]

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

Re: [PATCH v2 2/2] x86: set a dependency on macros.S

[Nadav Amit]

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

Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros

[Nadav Amit]

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

Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros

[Masahiro Yamada]

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

Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros

[Nadav Amit]

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

Re: [PATCH v2 1/2] Makefile: Fix distcc compilation with x86 macros

[Masahiro Yamada]

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