| Message ID | 20230621161959.1061178-2-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | build: reduce number of $(shell) execution on make 4.4 | expand |
On 21/06/2023 5:19 pm, Anthony PERARD wrote: > Defining ARCH and SRCARCH later in xen/Makefile allows to switch to > immediate evaluation variable type. > > ARCH and SRCARCH depends on value defined in Config.mk and aren't used > TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a > sub-make or a rule. > > This will help reduce the number of times the shell rune is been > run. > > With GNU make 4.4, the number of execution of the command present in > these $(shell ) increased greatly. This is probably because as of make > 4.4, exported variable are also added to the environment of $(shell ) > construct. > > Also, `make -d` shows a lot of these: > Makefile:39: not recursively expanding SRCARCH to export to shell function > Makefile:38: not recursively expanding ARCH to export to shell function > > Reported-by: Jason Andryuk <jandryuk@gmail.com> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > --- > xen/Makefile | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/xen/Makefile b/xen/Makefile > index e89fc461fc4b..9631e45cfb9b 100644 > --- a/xen/Makefile > +++ b/xen/Makefile > @@ -35,12 +35,6 @@ MAKEFLAGS += -rR > > EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi > > -ARCH=$(XEN_TARGET_ARCH) > -SRCARCH=$(shell echo $(ARCH) | \ > - sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ > - -e s'/riscv.*/riscv/g') > -export ARCH SRCARCH > - > # Allow someone to change their config file > export KCONFIG_CONFIG ?= .config > > @@ -241,6 +235,13 @@ include scripts/Kbuild.include > include $(XEN_ROOT)/Config.mk > > # Set ARCH/SUBARCH appropriately. > + > +ARCH := $(XEN_TARGET_ARCH) > +SRCARCH := $(shell echo $(ARCH) | \ > + sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ > + -e s'/riscv.*/riscv/g') > +export ARCH SRCARCH > + > export TARGET_SUBARCH := $(XEN_TARGET_ARCH) > export TARGET_ARCH := $(shell echo $(XEN_TARGET_ARCH) | \ > sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ The change looks plausible to fix this issue, but could we take the opportunity to dedup the sed expression into a $(call src-arch ...) or so ? Except, given that ARCH := $(XEN_TARGET_ARCH) now, doesn't that mean SRCARCH is always TARGET_ARCH ? Can't we simplify this to just be plain aliases? ~Andrew
On Wed, Jun 21, 2023 at 12:20 PM Anthony PERARD <anthony.perard@citrix.com> wrote: > > Defining ARCH and SRCARCH later in xen/Makefile allows to switch to > immediate evaluation variable type. > > ARCH and SRCARCH depends on value defined in Config.mk and aren't used > TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a > sub-make or a rule. > > This will help reduce the number of times the shell rune is been > run. > > With GNU make 4.4, the number of execution of the command present in > these $(shell ) increased greatly. This is probably because as of make > 4.4, exported variable are also added to the environment of $(shell ) > construct. > > Also, `make -d` shows a lot of these: > Makefile:39: not recursively expanding SRCARCH to export to shell function > Makefile:38: not recursively expanding ARCH to export to shell function > > Reported-by: Jason Andryuk <jandryuk@gmail.com> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> Tested-by: Jason Andryuk <jandryuk@gmail.com> Things are back to normal speed - Thanks a lot! -Jason
On Wed, Jun 21, 2023 at 12:27 PM Jason Andryuk <jandryuk@gmail.com> wrote: > > On Wed, Jun 21, 2023 at 12:20 PM Anthony PERARD > <anthony.perard@citrix.com> wrote: > > > > Defining ARCH and SRCARCH later in xen/Makefile allows to switch to > > immediate evaluation variable type. > > > > ARCH and SRCARCH depends on value defined in Config.mk and aren't used > > TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a > > sub-make or a rule. > > > > This will help reduce the number of times the shell rune is been > > run. > > > > With GNU make 4.4, the number of execution of the command present in > > these $(shell ) increased greatly. This is probably because as of make > > 4.4, exported variable are also added to the environment of $(shell ) > > construct. > > > > Also, `make -d` shows a lot of these: > > Makefile:39: not recursively expanding SRCARCH to export to shell function > > Makefile:38: not recursively expanding ARCH to export to shell function > > > > Reported-by: Jason Andryuk <jandryuk@gmail.com> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> > > Tested-by: Jason Andryuk <jandryuk@gmail.com> Tested-by: for the whole series, FYI. Thanks, Jason
On 21.06.2023 18:25, Andrew Cooper wrote: > On 21/06/2023 5:19 pm, Anthony PERARD wrote: >> Defining ARCH and SRCARCH later in xen/Makefile allows to switch to >> immediate evaluation variable type. >> >> ARCH and SRCARCH depends on value defined in Config.mk and aren't used >> TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a >> sub-make or a rule. >> >> This will help reduce the number of times the shell rune is been >> run. >> >> With GNU make 4.4, the number of execution of the command present in >> these $(shell ) increased greatly. This is probably because as of make >> 4.4, exported variable are also added to the environment of $(shell ) >> construct. >> >> Also, `make -d` shows a lot of these: >> Makefile:39: not recursively expanding SRCARCH to export to shell function >> Makefile:38: not recursively expanding ARCH to export to shell function >> >> Reported-by: Jason Andryuk <jandryuk@gmail.com> >> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> >> --- >> xen/Makefile | 13 +++++++------ >> 1 file changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/xen/Makefile b/xen/Makefile >> index e89fc461fc4b..9631e45cfb9b 100644 >> --- a/xen/Makefile >> +++ b/xen/Makefile >> @@ -35,12 +35,6 @@ MAKEFLAGS += -rR >> >> EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi >> >> -ARCH=$(XEN_TARGET_ARCH) >> -SRCARCH=$(shell echo $(ARCH) | \ >> - sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ >> - -e s'/riscv.*/riscv/g') >> -export ARCH SRCARCH >> - >> # Allow someone to change their config file >> export KCONFIG_CONFIG ?= .config >> >> @@ -241,6 +235,13 @@ include scripts/Kbuild.include >> include $(XEN_ROOT)/Config.mk >> >> # Set ARCH/SUBARCH appropriately. >> + >> +ARCH := $(XEN_TARGET_ARCH) >> +SRCARCH := $(shell echo $(ARCH) | \ >> + sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ >> + -e s'/riscv.*/riscv/g') >> +export ARCH SRCARCH >> + >> export TARGET_SUBARCH := $(XEN_TARGET_ARCH) >> export TARGET_ARCH := $(shell echo $(XEN_TARGET_ARCH) | \ >> sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ > > The change looks plausible to fix this issue, but could we take the > opportunity to dedup the sed expression into a $(call src-arch ...) or so ? > > Except, given that ARCH := $(XEN_TARGET_ARCH) now, doesn't that mean > SRCARCH is always TARGET_ARCH ? > > Can't we simplify this to just be plain aliases? Or, putting it differently, do we actually need both TARGET_* values when they match other (exported) variables anyway? Jan
On 21.06.2023 18:19, Anthony PERARD wrote: > @@ -241,6 +235,13 @@ include scripts/Kbuild.include > include $(XEN_ROOT)/Config.mk > > # Set ARCH/SUBARCH appropriately. > + > +ARCH := $(XEN_TARGET_ARCH) > +SRCARCH := $(shell echo $(ARCH) | \ > + sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ > + -e s'/riscv.*/riscv/g') Could you take the opportunity and get quoting consistent in this construct, while you move it? Jan
On Thu, Jun 22, 2023 at 10:15:42AM +0200, Jan Beulich wrote: > On 21.06.2023 18:25, Andrew Cooper wrote: > > On 21/06/2023 5:19 pm, Anthony PERARD wrote: > >> + > >> +ARCH := $(XEN_TARGET_ARCH) > >> +SRCARCH := $(shell echo $(ARCH) | \ > >> + sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ > >> + -e s'/riscv.*/riscv/g') > >> +export ARCH SRCARCH > >> + > >> export TARGET_SUBARCH := $(XEN_TARGET_ARCH) > >> export TARGET_ARCH := $(shell echo $(XEN_TARGET_ARCH) | \ > >> sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ > > > > The change looks plausible to fix this issue, but could we take the > > opportunity to dedup the sed expression into a $(call src-arch ...) or so ? > > > > Except, given that ARCH := $(XEN_TARGET_ARCH) now, doesn't that mean > > SRCARCH is always TARGET_ARCH ? > > > > Can't we simplify this to just be plain aliases? > > Or, putting it differently, do we actually need both TARGET_* values > when they match other (exported) variables anyway? Sounds good to me, I can remove both TARGET_* variables. Thanks,
diff --git a/xen/Makefile b/xen/Makefile index e89fc461fc4b..9631e45cfb9b 100644 --- a/xen/Makefile +++ b/xen/Makefile @@ -35,12 +35,6 @@ MAKEFLAGS += -rR EFI_MOUNTPOINT ?= $(BOOT_DIR)/efi -ARCH=$(XEN_TARGET_ARCH) -SRCARCH=$(shell echo $(ARCH) | \ - sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ - -e s'/riscv.*/riscv/g') -export ARCH SRCARCH - # Allow someone to change their config file export KCONFIG_CONFIG ?= .config @@ -241,6 +235,13 @@ include scripts/Kbuild.include include $(XEN_ROOT)/Config.mk # Set ARCH/SUBARCH appropriately. + +ARCH := $(XEN_TARGET_ARCH) +SRCARCH := $(shell echo $(ARCH) | \ + sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \ + -e s'/riscv.*/riscv/g') +export ARCH SRCARCH + export TARGET_SUBARCH := $(XEN_TARGET_ARCH) export TARGET_ARCH := $(shell echo $(XEN_TARGET_ARCH) | \ sed -e 's/x86.*/x86/' -e s'/arm\(32\|64\)/arm/g' \
Defining ARCH and SRCARCH later in xen/Makefile allows to switch to immediate evaluation variable type. ARCH and SRCARCH depends on value defined in Config.mk and aren't used TARGET_SUBARCH or TARGET_ARCH, and not before it's needed in a sub-make or a rule. This will help reduce the number of times the shell rune is been run. With GNU make 4.4, the number of execution of the command present in these $(shell ) increased greatly. This is probably because as of make 4.4, exported variable are also added to the environment of $(shell ) construct. Also, `make -d` shows a lot of these: Makefile:39: not recursively expanding SRCARCH to export to shell function Makefile:38: not recursively expanding ARCH to export to shell function Reported-by: Jason Andryuk <jandryuk@gmail.com> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- xen/Makefile | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)