[PATCH v7 8/8] kbuild: modinst: do modules_install step by step

Shreenidhi Shedi posted 8 patches 2 years, 7 months ago
There is a newer version of this series
[PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Shreenidhi Shedi 2 years, 7 months ago
Currently Makefile.modinst does three tasks on each module built:
- Install modules
- Sign modules
- Compress modules

All the above tasks happen from a single place.

This patch divides this task further and uses a different makefile for
each task.
Signing module logic is completely refactored and everything happens
from a shell script now.

Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
---
 scripts/Makefile.compress |  53 ++++++++++++++++++
 scripts/Makefile.install  |  66 +++++++++++++++++++++++
 scripts/Makefile.modinst  | 111 +++-----------------------------------
 scripts/Makefile.sign     |  37 +++++++++++++
 scripts/signfile.sh       |  24 +++++++++
 5 files changed, 186 insertions(+), 105 deletions(-)
 create mode 100644 scripts/Makefile.compress
 create mode 100644 scripts/Makefile.install
 create mode 100644 scripts/Makefile.sign
 create mode 100755 scripts/signfile.sh

diff --git a/scripts/Makefile.compress b/scripts/Makefile.compress
new file mode 100644
index 000000000000..35d337ac9b6c
--- /dev/null
+++ b/scripts/Makefile.compress
@@ -0,0 +1,53 @@
+# SPDX-License-Identifier: GPL-2.0
+# ==========================================================================
+# Compressing modules
+# ==========================================================================
+
+PHONY := __modcompress
+__modcompress:
+
+include include/config/auto.conf
+include $(srctree)/scripts/Kbuild.include
+
+modules := $(call read-file, $(MODORDER))
+
+ifeq ($(KBUILD_EXTMOD),)
+dst := $(MODLIB)/kernel
+else
+INSTALL_MOD_DIR ?= updates
+dst := $(MODLIB)/$(INSTALL_MOD_DIR)
+endif
+
+suffix-y				:=
+suffix-$(CONFIG_MODULE_COMPRESS_GZIP)	:= .gz
+suffix-$(CONFIG_MODULE_COMPRESS_XZ)	:= .xz
+suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)	:= .zst
+
+modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
+
+__modcompress: $(modules)
+	@:
+
+#
+# Compression
+#
+quiet_cmd_gzip = GZIP    $@
+      cmd_gzip = $(KGZIP) -n -f $<
+quiet_cmd_xz = XZ      $@
+      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
+quiet_cmd_zstd = ZSTD    $@
+      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
+
+$(dst)/%.ko.gz: $(dst)/%.ko FORCE
+	$(call cmd,gzip)
+
+$(dst)/%.ko.xz: $(dst)/%.ko FORCE
+	$(call cmd,xz)
+
+$(dst)/%.ko.zst: $(dst)/%.ko FORCE
+	$(call cmd,zstd)
+
+PHONY += FORCE
+FORCE:
+
+.PHONY: $(PHONY)
diff --git a/scripts/Makefile.install b/scripts/Makefile.install
new file mode 100644
index 000000000000..40c496cb99dc
--- /dev/null
+++ b/scripts/Makefile.install
@@ -0,0 +1,66 @@
+# SPDX-License-Identifier: GPL-2.0
+# ==========================================================================
+# Installing modules
+# ==========================================================================
+
+PHONY := __modinstall
+__modinstall:
+
+include include/config/auto.conf
+include $(srctree)/scripts/Kbuild.include
+
+modules := $(call read-file, $(MODORDER))
+
+ifeq ($(KBUILD_EXTMOD),)
+dst := $(MODLIB)/kernel
+else
+INSTALL_MOD_DIR ?= updates
+dst := $(MODLIB)/$(INSTALL_MOD_DIR)
+endif
+
+$(foreach x, % :, $(if $(findstring $x, $(dst)), \
+	$(error module installation path cannot contain '$x')))
+
+modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
+
+__modinstall: $(modules)
+	@:
+
+#
+# Installation
+#
+quiet_cmd_install = INSTALL $@
+      cmd_install = mkdir -p $(dir $@); cp $< $@
+
+# Strip
+#
+# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
+# are installed. If INSTALL_MOD_STRIP is '1', then the default option
+# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
+# as the options to the strip command.
+ifdef INSTALL_MOD_STRIP
+
+ifeq ($(INSTALL_MOD_STRIP),1)
+strip-option := --strip-debug
+else
+strip-option := $(INSTALL_MOD_STRIP)
+endif
+
+quiet_cmd_strip = STRIP   $@
+      cmd_strip = $(STRIP) $(strip-option) $@
+
+else
+
+quiet_cmd_strip =
+      cmd_strip = :
+
+endif
+
+$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
+	$(call cmd,install)
+	$(call cmd,strip)
+
+PHONY += FORCE
+FORCE:
+
+.PHONY: $(PHONY)
diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
index ab0c5bd1a60f..d87e09e57963 100644
--- a/scripts/Makefile.modinst
+++ b/scripts/Makefile.modinst
@@ -1,116 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0
 # ==========================================================================
-# Installing modules
+# Install, Sign & Compress modules
 # ==========================================================================
 
-PHONY := __modinst
-__modinst:
-
 include include/config/auto.conf
 include $(srctree)/scripts/Kbuild.include
 
-modules := $(call read-file, $(MODORDER))
-
-ifeq ($(KBUILD_EXTMOD),)
-dst := $(MODLIB)/kernel
-else
-INSTALL_MOD_DIR ?= updates
-dst := $(MODLIB)/$(INSTALL_MOD_DIR)
-endif
-
-$(foreach x, % :, $(if $(findstring $x, $(dst)), \
-	$(error module installation path cannot contain '$x')))
-
-suffix-y				:=
-suffix-$(CONFIG_MODULE_COMPRESS_GZIP)	:= .gz
-suffix-$(CONFIG_MODULE_COMPRESS_XZ)	:= .xz
-suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)	:= .zst
-
-modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
-
-__modinst: $(modules)
-	@:
-
-#
-# Installation
-#
-quiet_cmd_install = INSTALL $@
-      cmd_install = mkdir -p $(dir $@); cp $< $@
-
-# Strip
-#
-# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
-# are installed. If INSTALL_MOD_STRIP is '1', then the default option
-# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
-# as the options to the strip command.
-ifdef INSTALL_MOD_STRIP
-
-ifeq ($(INSTALL_MOD_STRIP),1)
-strip-option := --strip-debug
-else
-strip-option := $(INSTALL_MOD_STRIP)
-endif
-
-quiet_cmd_strip = STRIP   $@
-      cmd_strip = $(STRIP) $(strip-option) $@
-
-else
-
-quiet_cmd_strip =
-      cmd_strip = :
-
-endif
-
-#
-# Signing
-# Don't stop modules_install even if we can't sign external modules.
-#
-ifeq ($(CONFIG_MODULE_SIG_ALL),y)
-ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
-sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
-else
-sig-key := $(CONFIG_MODULE_SIG_KEY)
-endif
-quiet_cmd_sign = SIGN    $@
-      cmd_sign = scripts/sign-file $(CONFIG_MODULE_SIG_HASH) "$(sig-key)" certs/signing_key.x509 $@ \
-                 $(if $(KBUILD_EXTMOD),|| true)
-else
-quiet_cmd_sign :=
-      cmd_sign := :
-endif
-
-ifeq ($(modules_sign_only),)
-
-$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
-	$(call cmd,install)
-	$(call cmd,strip)
-	$(call cmd,sign)
-
-else
-
-$(dst)/%.ko: FORCE
-	$(call cmd,sign)
-
-endif
-
-#
-# Compression
-#
-quiet_cmd_gzip = GZIP    $@
-      cmd_gzip = $(KGZIP) -n -f $<
-quiet_cmd_xz = XZ      $@
-      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
-quiet_cmd_zstd = ZSTD    $@
-      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
-
-$(dst)/%.ko.gz: $(dst)/%.ko FORCE
-	$(call cmd,gzip)
-
-$(dst)/%.ko.xz: $(dst)/%.ko FORCE
-	$(call cmd,xz)
+PHONY := __modinst
 
-$(dst)/%.ko.zst: $(dst)/%.ko FORCE
-	$(call cmd,zstd)
+__modinst: FORCE
+	$(MAKE) -f scripts/Makefile.install
+	$(MAKE) -f scripts/Makefile.sign
+	$(MAKE) -f scripts/Makefile.compress
 
 PHONY += FORCE
 FORCE:
diff --git a/scripts/Makefile.sign b/scripts/Makefile.sign
new file mode 100644
index 000000000000..d6b242b16657
--- /dev/null
+++ b/scripts/Makefile.sign
@@ -0,0 +1,37 @@
+# SPDX-License-Identifier: GPL-2.0
+# ==========================================================================
+# Signing modules
+# ==========================================================================
+
+PHONY := __modsign
+__modsign:
+
+include include/config/auto.conf
+include $(srctree)/scripts/Kbuild.include
+
+#
+# Signing
+# Don't stop modules_install even if we can't sign external modules.
+#
+ifeq ($(CONFIG_MODULE_SIG_ALL),y)
+ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
+sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
+else
+sig-key := $(CONFIG_MODULE_SIG_KEY)
+endif
+quiet_cmd_sign = SIGNING ALL MODULES ...
+      cmd_sign = $(CONFIG_SHELL) $(srctree)/scripts/signfile.sh \
+					 "$(CONFIG_MODULE_SIG_HASH)" \
+					 "$(sig-key)"
+else
+quiet_cmd_sign :=
+      cmd_sign := :
+endif
+
+__modsign: FORCE
+	$(call cmd,sign)
+
+PHONY += FORCE
+FORCE:
+
+.PHONY: $(PHONY)
diff --git a/scripts/signfile.sh b/scripts/signfile.sh
new file mode 100755
index 000000000000..b2b58bfbd5ba
--- /dev/null
+++ b/scripts/signfile.sh
@@ -0,0 +1,24 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+#
+# A sign-file wrapper used by scripts/Makefile.sign
+
+#set -x
+
+if test $# -ne 2; then
+	echo "Usage: $0 <hash-algo> <sign-key>" >&2
+	exit 1
+fi
+
+SIG_HASH="$1"
+SIG_KEY="$2"
+
+MODULES_PATH="${INSTALL_MOD_PATH}/lib/modules/${KERNELRELEASE}"
+
+find "${MODULES_PATH}" -name *.ko -type f -print0 | \
+	xargs -r -0 -P$(nproc) -x -n32 sh -c "\
+${srctree}/scripts/sign-file \
+-a \"${SIG_HASH}\" \
+-i \"${SIG_KEY}\" \
+-x ${srctree}/certs/signing_key.x509 \
+-b \$@ \$0"
-- 
2.41.0
Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Masahiro Yamada 2 years, 6 months ago
On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>
> Currently Makefile.modinst does three tasks on each module built:
> - Install modules
> - Sign modules
> - Compress modules
>
> All the above tasks happen from a single place.
>
> This patch divides this task further and uses a different makefile for
> each task.
> Signing module logic is completely refactored and everything happens
> from a shell script now.
>
> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>


This patch is bad in multiple ways.

1. Break "make modules_sign"
2.   Serialize the installation steps, that is, works less efficiently
3.   Increase code without adding any benefits.


There is no good reason to do these changes.

NACK.





> ---
>  scripts/Makefile.compress |  53 ++++++++++++++++++
>  scripts/Makefile.install  |  66 +++++++++++++++++++++++
>  scripts/Makefile.modinst  | 111 +++-----------------------------------
>  scripts/Makefile.sign     |  37 +++++++++++++
>  scripts/signfile.sh       |  24 +++++++++
>  5 files changed, 186 insertions(+), 105 deletions(-)
>  create mode 100644 scripts/Makefile.compress
>  create mode 100644 scripts/Makefile.install
>  create mode 100644 scripts/Makefile.sign
>  create mode 100755 scripts/signfile.sh
>
> diff --git a/scripts/Makefile.compress b/scripts/Makefile.compress
> new file mode 100644
> index 000000000000..35d337ac9b6c
> --- /dev/null
> +++ b/scripts/Makefile.compress
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Compressing modules
> +# ==========================================================================
> +
> +PHONY := __modcompress
> +__modcompress:
> +
> +include include/config/auto.conf
> +include $(srctree)/scripts/Kbuild.include
> +
> +modules := $(call read-file, $(MODORDER))
> +
> +ifeq ($(KBUILD_EXTMOD),)
> +dst := $(MODLIB)/kernel
> +else
> +INSTALL_MOD_DIR ?= updates
> +dst := $(MODLIB)/$(INSTALL_MOD_DIR)
> +endif
> +
> +suffix-y                               :=
> +suffix-$(CONFIG_MODULE_COMPRESS_GZIP)  := .gz
> +suffix-$(CONFIG_MODULE_COMPRESS_XZ)    := .xz
> +suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
> +
> +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> +
> +__modcompress: $(modules)
> +       @:
> +
> +#
> +# Compression
> +#
> +quiet_cmd_gzip = GZIP    $@
> +      cmd_gzip = $(KGZIP) -n -f $<
> +quiet_cmd_xz = XZ      $@
> +      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
> +quiet_cmd_zstd = ZSTD    $@
> +      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
> +
> +$(dst)/%.ko.gz: $(dst)/%.ko FORCE
> +       $(call cmd,gzip)
> +
> +$(dst)/%.ko.xz: $(dst)/%.ko FORCE
> +       $(call cmd,xz)
> +
> +$(dst)/%.ko.zst: $(dst)/%.ko FORCE
> +       $(call cmd,zstd)
> +
> +PHONY += FORCE
> +FORCE:
> +
> +.PHONY: $(PHONY)
> diff --git a/scripts/Makefile.install b/scripts/Makefile.install
> new file mode 100644
> index 000000000000..40c496cb99dc
> --- /dev/null
> +++ b/scripts/Makefile.install
> @@ -0,0 +1,66 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Installing modules
> +# ==========================================================================
> +
> +PHONY := __modinstall
> +__modinstall:
> +
> +include include/config/auto.conf
> +include $(srctree)/scripts/Kbuild.include
> +
> +modules := $(call read-file, $(MODORDER))
> +
> +ifeq ($(KBUILD_EXTMOD),)
> +dst := $(MODLIB)/kernel
> +else
> +INSTALL_MOD_DIR ?= updates
> +dst := $(MODLIB)/$(INSTALL_MOD_DIR)
> +endif
> +
> +$(foreach x, % :, $(if $(findstring $x, $(dst)), \
> +       $(error module installation path cannot contain '$x')))
> +
> +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> +
> +__modinstall: $(modules)
> +       @:
> +
> +#
> +# Installation
> +#
> +quiet_cmd_install = INSTALL $@
> +      cmd_install = mkdir -p $(dir $@); cp $< $@
> +
> +# Strip
> +#
> +# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
> +# are installed. If INSTALL_MOD_STRIP is '1', then the default option
> +# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
> +# as the options to the strip command.
> +ifdef INSTALL_MOD_STRIP
> +
> +ifeq ($(INSTALL_MOD_STRIP),1)
> +strip-option := --strip-debug
> +else
> +strip-option := $(INSTALL_MOD_STRIP)
> +endif
> +
> +quiet_cmd_strip = STRIP   $@
> +      cmd_strip = $(STRIP) $(strip-option) $@
> +
> +else
> +
> +quiet_cmd_strip =
> +      cmd_strip = :
> +
> +endif
> +
> +$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
> +       $(call cmd,install)
> +       $(call cmd,strip)
> +
> +PHONY += FORCE
> +FORCE:
> +
> +.PHONY: $(PHONY)
> diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
> index ab0c5bd1a60f..d87e09e57963 100644
> --- a/scripts/Makefile.modinst
> +++ b/scripts/Makefile.modinst
> @@ -1,116 +1,17 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # ==========================================================================
> -# Installing modules
> +# Install, Sign & Compress modules
>  # ==========================================================================
>
> -PHONY := __modinst
> -__modinst:
> -
>  include include/config/auto.conf
>  include $(srctree)/scripts/Kbuild.include
>
> -modules := $(call read-file, $(MODORDER))
> -
> -ifeq ($(KBUILD_EXTMOD),)
> -dst := $(MODLIB)/kernel
> -else
> -INSTALL_MOD_DIR ?= updates
> -dst := $(MODLIB)/$(INSTALL_MOD_DIR)
> -endif
> -
> -$(foreach x, % :, $(if $(findstring $x, $(dst)), \
> -       $(error module installation path cannot contain '$x')))
> -
> -suffix-y                               :=
> -suffix-$(CONFIG_MODULE_COMPRESS_GZIP)  := .gz
> -suffix-$(CONFIG_MODULE_COMPRESS_XZ)    := .xz
> -suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
> -
> -modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
> -
> -__modinst: $(modules)
> -       @:
> -
> -#
> -# Installation
> -#
> -quiet_cmd_install = INSTALL $@
> -      cmd_install = mkdir -p $(dir $@); cp $< $@
> -
> -# Strip
> -#
> -# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
> -# are installed. If INSTALL_MOD_STRIP is '1', then the default option
> -# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
> -# as the options to the strip command.
> -ifdef INSTALL_MOD_STRIP
> -
> -ifeq ($(INSTALL_MOD_STRIP),1)
> -strip-option := --strip-debug
> -else
> -strip-option := $(INSTALL_MOD_STRIP)
> -endif
> -
> -quiet_cmd_strip = STRIP   $@
> -      cmd_strip = $(STRIP) $(strip-option) $@
> -
> -else
> -
> -quiet_cmd_strip =
> -      cmd_strip = :
> -
> -endif
> -
> -#
> -# Signing
> -# Don't stop modules_install even if we can't sign external modules.
> -#
> -ifeq ($(CONFIG_MODULE_SIG_ALL),y)
> -ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
> -sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
> -else
> -sig-key := $(CONFIG_MODULE_SIG_KEY)
> -endif
> -quiet_cmd_sign = SIGN    $@
> -      cmd_sign = scripts/sign-file $(CONFIG_MODULE_SIG_HASH) "$(sig-key)" certs/signing_key.x509 $@ \
> -                 $(if $(KBUILD_EXTMOD),|| true)
> -else
> -quiet_cmd_sign :=
> -      cmd_sign := :
> -endif
> -
> -ifeq ($(modules_sign_only),)
> -
> -$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
> -       $(call cmd,install)
> -       $(call cmd,strip)
> -       $(call cmd,sign)
> -
> -else
> -
> -$(dst)/%.ko: FORCE
> -       $(call cmd,sign)
> -
> -endif
> -
> -#
> -# Compression
> -#
> -quiet_cmd_gzip = GZIP    $@
> -      cmd_gzip = $(KGZIP) -n -f $<
> -quiet_cmd_xz = XZ      $@
> -      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
> -quiet_cmd_zstd = ZSTD    $@
> -      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
> -
> -$(dst)/%.ko.gz: $(dst)/%.ko FORCE
> -       $(call cmd,gzip)
> -
> -$(dst)/%.ko.xz: $(dst)/%.ko FORCE
> -       $(call cmd,xz)
> +PHONY := __modinst
>
> -$(dst)/%.ko.zst: $(dst)/%.ko FORCE
> -       $(call cmd,zstd)
> +__modinst: FORCE
> +       $(MAKE) -f scripts/Makefile.install
> +       $(MAKE) -f scripts/Makefile.sign
> +       $(MAKE) -f scripts/Makefile.compress
>
>  PHONY += FORCE
>  FORCE:
> diff --git a/scripts/Makefile.sign b/scripts/Makefile.sign
> new file mode 100644
> index 000000000000..d6b242b16657
> --- /dev/null
> +++ b/scripts/Makefile.sign
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# ==========================================================================
> +# Signing modules
> +# ==========================================================================
> +
> +PHONY := __modsign
> +__modsign:
> +
> +include include/config/auto.conf
> +include $(srctree)/scripts/Kbuild.include
> +
> +#
> +# Signing
> +# Don't stop modules_install even if we can't sign external modules.
> +#
> +ifeq ($(CONFIG_MODULE_SIG_ALL),y)
> +ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
> +sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
> +else
> +sig-key := $(CONFIG_MODULE_SIG_KEY)
> +endif
> +quiet_cmd_sign = SIGNING ALL MODULES ...
> +      cmd_sign = $(CONFIG_SHELL) $(srctree)/scripts/signfile.sh \
> +                                        "$(CONFIG_MODULE_SIG_HASH)" \
> +                                        "$(sig-key)"
> +else
> +quiet_cmd_sign :=
> +      cmd_sign := :
> +endif
> +
> +__modsign: FORCE
> +       $(call cmd,sign)
> +
> +PHONY += FORCE
> +FORCE:
> +
> +.PHONY: $(PHONY)
> diff --git a/scripts/signfile.sh b/scripts/signfile.sh
> new file mode 100755
> index 000000000000..b2b58bfbd5ba
> --- /dev/null
> +++ b/scripts/signfile.sh
> @@ -0,0 +1,24 @@
> +#!/bin/sh
> +# SPDX-License-Identifier: GPL-2.0
> +#
> +# A sign-file wrapper used by scripts/Makefile.sign
> +
> +#set -x
> +
> +if test $# -ne 2; then
> +       echo "Usage: $0 <hash-algo> <sign-key>" >&2
> +       exit 1
> +fi
> +
> +SIG_HASH="$1"
> +SIG_KEY="$2"
> +
> +MODULES_PATH="${INSTALL_MOD_PATH}/lib/modules/${KERNELRELEASE}"
> +
> +find "${MODULES_PATH}" -name *.ko -type f -print0 | \
> +       xargs -r -0 -P$(nproc) -x -n32 sh -c "\
> +${srctree}/scripts/sign-file \
> +-a \"${SIG_HASH}\" \
> +-i \"${SIG_KEY}\" \
> +-x ${srctree}/certs/signing_key.x509 \
> +-b \$@ \$0"
> --
> 2.41.0
>


-- 
Best Regards
Masahiro Yamada
Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Shreenidhi Shedi 2 years, 6 months ago
On 07/08/23 01:02, Masahiro Yamada wrote:
> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>>
>> Currently Makefile.modinst does three tasks on each module built:
>> - Install modules
>> - Sign modules
>> - Compress modules
>>
>> All the above tasks happen from a single place.
>>
>> This patch divides this task further and uses a different makefile for
>> each task.
>> Signing module logic is completely refactored and everything happens
>> from a shell script now.
>>
>> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
> 
> 
> This patch is bad in multiple ways.
> 
> 1. Break "make modules_sign"

Correct, somehow I missed it. I will fix it.
I'm using below command to test sign only option. Please let me know if 
I should use something else.

make modules_sign modules_sign_only=1 INSTALL_MOD_PATH=$PWD/tmp -j8

> 2.   Serialize the installation steps, that is, works less efficiently

Even in the existing system it happens in serially. And the existing 
method takes more time than the proposed version.

root@ph5dev:~/linux-6.3.5 # ./test.sh orig

real	0m14.699s
user	0m55.519s
sys	0m9.036s

root@ph5dev:~/linux-6.3.5 # ./test.sh new

real	0m13.327s
user	0m46.885s
sys	0m6.770s

Here is my test script.
```
#!/bin/bash

set -e

if [ "$1" != "new" ] && [ "$1" != "orig" ]; then
   echo "invalid arg, ($0 [orig|new])" >&2
   exit 1
fi

rm -rf $PWD/tmp

s="scripts/sign-file.c"
m="scripts/Makefile.modinst"
fns=($s $m)

for f in ${fns[@]}; do
     cp $f.$1 $f
done

cd scripts
gcc -o sign-file sign-file.c -lcrypto
cd -

time make modules_install INSTALL_MOD_PATH=$PWD/tmp -s -j$(nproc)
```

> 3.   Increase code without adding any benefits.
>Agree with increased code but this change is one step closer to Unix 
philosophy, do one thing well wrt modules_install.

> 
> There is no good reason to do these chang >I hope the data I provided above to your 2nd point provides evidence 
that this fix is improving existing system. Please take a look again.

> NACK.

Hi Masahiro Yamada,

Replies inline above.

Please correct me if my understanding is wrong. Thanks a lot for your 
time and patience. Have a nice time ahead.

> 
> 
> 
> 
> 
>> ---
>>   scripts/Makefile.compress |  53 ++++++++++++++++++
>>   scripts/Makefile.install  |  66 +++++++++++++++++++++++
>>   scripts/Makefile.modinst  | 111 +++-----------------------------------
>>   scripts/Makefile.sign     |  37 +++++++++++++
>>   scripts/signfile.sh       |  24 +++++++++
>>   5 files changed, 186 insertions(+), 105 deletions(-)
>>   create mode 100644 scripts/Makefile.compress
>>   create mode 100644 scripts/Makefile.install
>>   create mode 100644 scripts/Makefile.sign
>>   create mode 100755 scripts/signfile.sh
>>
>> diff --git a/scripts/Makefile.compress b/scripts/Makefile.compress
>> new file mode 100644
>> index 000000000000..35d337ac9b6c
>> --- /dev/null
>> +++ b/scripts/Makefile.compress
>> @@ -0,0 +1,53 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# ==========================================================================
>> +# Compressing modules
>> +# ==========================================================================
>> +
>> +PHONY := __modcompress
>> +__modcompress:
>> +
>> +include include/config/auto.conf
>> +include $(srctree)/scripts/Kbuild.include
>> +
>> +modules := $(call read-file, $(MODORDER))
>> +
>> +ifeq ($(KBUILD_EXTMOD),)
>> +dst := $(MODLIB)/kernel
>> +else
>> +INSTALL_MOD_DIR ?= updates
>> +dst := $(MODLIB)/$(INSTALL_MOD_DIR)
>> +endif
>> +
>> +suffix-y                               :=
>> +suffix-$(CONFIG_MODULE_COMPRESS_GZIP)  := .gz
>> +suffix-$(CONFIG_MODULE_COMPRESS_XZ)    := .xz
>> +suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
>> +
>> +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
>> +
>> +__modcompress: $(modules)
>> +       @:
>> +
>> +#
>> +# Compression
>> +#
>> +quiet_cmd_gzip = GZIP    $@
>> +      cmd_gzip = $(KGZIP) -n -f $<
>> +quiet_cmd_xz = XZ      $@
>> +      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
>> +quiet_cmd_zstd = ZSTD    $@
>> +      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
>> +
>> +$(dst)/%.ko.gz: $(dst)/%.ko FORCE
>> +       $(call cmd,gzip)
>> +
>> +$(dst)/%.ko.xz: $(dst)/%.ko FORCE
>> +       $(call cmd,xz)
>> +
>> +$(dst)/%.ko.zst: $(dst)/%.ko FORCE
>> +       $(call cmd,zstd)
>> +
>> +PHONY += FORCE
>> +FORCE:
>> +
>> +.PHONY: $(PHONY)
>> diff --git a/scripts/Makefile.install b/scripts/Makefile.install
>> new file mode 100644
>> index 000000000000..40c496cb99dc
>> --- /dev/null
>> +++ b/scripts/Makefile.install
>> @@ -0,0 +1,66 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# ==========================================================================
>> +# Installing modules
>> +# ==========================================================================
>> +
>> +PHONY := __modinstall
>> +__modinstall:
>> +
>> +include include/config/auto.conf
>> +include $(srctree)/scripts/Kbuild.include
>> +
>> +modules := $(call read-file, $(MODORDER))
>> +
>> +ifeq ($(KBUILD_EXTMOD),)
>> +dst := $(MODLIB)/kernel
>> +else
>> +INSTALL_MOD_DIR ?= updates
>> +dst := $(MODLIB)/$(INSTALL_MOD_DIR)
>> +endif
>> +
>> +$(foreach x, % :, $(if $(findstring $x, $(dst)), \
>> +       $(error module installation path cannot contain '$x')))
>> +
>> +modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
>> +
>> +__modinstall: $(modules)
>> +       @:
>> +
>> +#
>> +# Installation
>> +#
>> +quiet_cmd_install = INSTALL $@
>> +      cmd_install = mkdir -p $(dir $@); cp $< $@
>> +
>> +# Strip
>> +#
>> +# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
>> +# are installed. If INSTALL_MOD_STRIP is '1', then the default option
>> +# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
>> +# as the options to the strip command.
>> +ifdef INSTALL_MOD_STRIP
>> +
>> +ifeq ($(INSTALL_MOD_STRIP),1)
>> +strip-option := --strip-debug
>> +else
>> +strip-option := $(INSTALL_MOD_STRIP)
>> +endif
>> +
>> +quiet_cmd_strip = STRIP   $@
>> +      cmd_strip = $(STRIP) $(strip-option) $@
>> +
>> +else
>> +
>> +quiet_cmd_strip =
>> +      cmd_strip = :
>> +
>> +endif
>> +
>> +$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
>> +       $(call cmd,install)
>> +       $(call cmd,strip)
>> +
>> +PHONY += FORCE
>> +FORCE:
>> +
>> +.PHONY: $(PHONY)
>> diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst
>> index ab0c5bd1a60f..d87e09e57963 100644
>> --- a/scripts/Makefile.modinst
>> +++ b/scripts/Makefile.modinst
>> @@ -1,116 +1,17 @@
>>   # SPDX-License-Identifier: GPL-2.0
>>   # ==========================================================================
>> -# Installing modules
>> +# Install, Sign & Compress modules
>>   # ==========================================================================
>>
>> -PHONY := __modinst
>> -__modinst:
>> -
>>   include include/config/auto.conf
>>   include $(srctree)/scripts/Kbuild.include
>>
>> -modules := $(call read-file, $(MODORDER))
>> -
>> -ifeq ($(KBUILD_EXTMOD),)
>> -dst := $(MODLIB)/kernel
>> -else
>> -INSTALL_MOD_DIR ?= updates
>> -dst := $(MODLIB)/$(INSTALL_MOD_DIR)
>> -endif
>> -
>> -$(foreach x, % :, $(if $(findstring $x, $(dst)), \
>> -       $(error module installation path cannot contain '$x')))
>> -
>> -suffix-y                               :=
>> -suffix-$(CONFIG_MODULE_COMPRESS_GZIP)  := .gz
>> -suffix-$(CONFIG_MODULE_COMPRESS_XZ)    := .xz
>> -suffix-$(CONFIG_MODULE_COMPRESS_ZSTD)  := .zst
>> -
>> -modules := $(patsubst $(extmod_prefix)%.o, $(dst)/%.ko$(suffix-y), $(modules))
>> -
>> -__modinst: $(modules)
>> -       @:
>> -
>> -#
>> -# Installation
>> -#
>> -quiet_cmd_install = INSTALL $@
>> -      cmd_install = mkdir -p $(dir $@); cp $< $@
>> -
>> -# Strip
>> -#
>> -# INSTALL_MOD_STRIP, if defined, will cause modules to be stripped after they
>> -# are installed. If INSTALL_MOD_STRIP is '1', then the default option
>> -# --strip-debug will be used. Otherwise, INSTALL_MOD_STRIP value will be used
>> -# as the options to the strip command.
>> -ifdef INSTALL_MOD_STRIP
>> -
>> -ifeq ($(INSTALL_MOD_STRIP),1)
>> -strip-option := --strip-debug
>> -else
>> -strip-option := $(INSTALL_MOD_STRIP)
>> -endif
>> -
>> -quiet_cmd_strip = STRIP   $@
>> -      cmd_strip = $(STRIP) $(strip-option) $@
>> -
>> -else
>> -
>> -quiet_cmd_strip =
>> -      cmd_strip = :
>> -
>> -endif
>> -
>> -#
>> -# Signing
>> -# Don't stop modules_install even if we can't sign external modules.
>> -#
>> -ifeq ($(CONFIG_MODULE_SIG_ALL),y)
>> -ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
>> -sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
>> -else
>> -sig-key := $(CONFIG_MODULE_SIG_KEY)
>> -endif
>> -quiet_cmd_sign = SIGN    $@
>> -      cmd_sign = scripts/sign-file $(CONFIG_MODULE_SIG_HASH) "$(sig-key)" certs/signing_key.x509 $@ \
>> -                 $(if $(KBUILD_EXTMOD),|| true)
>> -else
>> -quiet_cmd_sign :=
>> -      cmd_sign := :
>> -endif
>> -
>> -ifeq ($(modules_sign_only),)
>> -
>> -$(dst)/%.ko: $(extmod_prefix)%.ko FORCE
>> -       $(call cmd,install)
>> -       $(call cmd,strip)
>> -       $(call cmd,sign)
>> -
>> -else
>> -
>> -$(dst)/%.ko: FORCE
>> -       $(call cmd,sign)
>> -
>> -endif
>> -
>> -#
>> -# Compression
>> -#
>> -quiet_cmd_gzip = GZIP    $@
>> -      cmd_gzip = $(KGZIP) -n -f $<
>> -quiet_cmd_xz = XZ      $@
>> -      cmd_xz = $(XZ) --lzma2=dict=2MiB -f $<
>> -quiet_cmd_zstd = ZSTD    $@
>> -      cmd_zstd = $(ZSTD) -T0 --rm -f -q $<
>> -
>> -$(dst)/%.ko.gz: $(dst)/%.ko FORCE
>> -       $(call cmd,gzip)
>> -
>> -$(dst)/%.ko.xz: $(dst)/%.ko FORCE
>> -       $(call cmd,xz)
>> +PHONY := __modinst
>>
>> -$(dst)/%.ko.zst: $(dst)/%.ko FORCE
>> -       $(call cmd,zstd)
>> +__modinst: FORCE
>> +       $(MAKE) -f scripts/Makefile.install
>> +       $(MAKE) -f scripts/Makefile.sign
>> +       $(MAKE) -f scripts/Makefile.compress
>>
>>   PHONY += FORCE
>>   FORCE:
>> diff --git a/scripts/Makefile.sign b/scripts/Makefile.sign
>> new file mode 100644
>> index 000000000000..d6b242b16657
>> --- /dev/null
>> +++ b/scripts/Makefile.sign
>> @@ -0,0 +1,37 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +# ==========================================================================
>> +# Signing modules
>> +# ==========================================================================
>> +
>> +PHONY := __modsign
>> +__modsign:
>> +
>> +include include/config/auto.conf
>> +include $(srctree)/scripts/Kbuild.include
>> +
>> +#
>> +# Signing
>> +# Don't stop modules_install even if we can't sign external modules.
>> +#
>> +ifeq ($(CONFIG_MODULE_SIG_ALL),y)
>> +ifeq ($(filter pkcs11:%, $(CONFIG_MODULE_SIG_KEY)),)
>> +sig-key := $(if $(wildcard $(CONFIG_MODULE_SIG_KEY)),,$(srctree)/)$(CONFIG_MODULE_SIG_KEY)
>> +else
>> +sig-key := $(CONFIG_MODULE_SIG_KEY)
>> +endif
>> +quiet_cmd_sign = SIGNING ALL MODULES ...
>> +      cmd_sign = $(CONFIG_SHELL) $(srctree)/scripts/signfile.sh \
>> +                                        "$(CONFIG_MODULE_SIG_HASH)" \
>> +                                        "$(sig-key)"
>> +else
>> +quiet_cmd_sign :=
>> +      cmd_sign := :
>> +endif
>> +
>> +__modsign: FORCE
>> +       $(call cmd,sign)
>> +
>> +PHONY += FORCE
>> +FORCE:
>> +
>> +.PHONY: $(PHONY)
>> diff --git a/scripts/signfile.sh b/scripts/signfile.sh
>> new file mode 100755
>> index 000000000000..b2b58bfbd5ba
>> --- /dev/null
>> +++ b/scripts/signfile.sh
>> @@ -0,0 +1,24 @@
>> +#!/bin/sh
>> +# SPDX-License-Identifier: GPL-2.0
>> +#
>> +# A sign-file wrapper used by scripts/Makefile.sign
>> +
>> +#set -x
>> +
>> +if test $# -ne 2; then
>> +       echo "Usage: $0 <hash-algo> <sign-key>" >&2
>> +       exit 1
>> +fi
>> +
>> +SIG_HASH="$1"
>> +SIG_KEY="$2"
>> +
>> +MODULES_PATH="${INSTALL_MOD_PATH}/lib/modules/${KERNELRELEASE}"
>> +
>> +find "${MODULES_PATH}" -name *.ko -type f -print0 | \
>> +       xargs -r -0 -P$(nproc) -x -n32 sh -c "\
>> +${srctree}/scripts/sign-file \
>> +-a \"${SIG_HASH}\" \
>> +-i \"${SIG_KEY}\" \
>> +-x ${srctree}/certs/signing_key.x509 \
>> +-b \$@ \$0"
>> --
>> 2.41.0
>>
> 
> 

-- 
Shedi

Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Masahiro Yamada 2 years, 6 months ago
On Mon, Aug 7, 2023 at 5:08 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>
> On 07/08/23 01:02, Masahiro Yamada wrote:
> > On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
> >>
> >> Currently Makefile.modinst does three tasks on each module built:
> >> - Install modules
> >> - Sign modules
> >> - Compress modules
> >>
> >> All the above tasks happen from a single place.
> >>
> >> This patch divides this task further and uses a different makefile for
> >> each task.
> >> Signing module logic is completely refactored and everything happens
> >> from a shell script now.
> >>
> >> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
> >
> >
> > This patch is bad in multiple ways.
> >
> > 1. Break "make modules_sign"
>
> Correct, somehow I missed it. I will fix it.
> I'm using below command to test sign only option. Please let me know if
> I should use something else.
>
> make modules_sign modules_sign_only=1 INSTALL_MOD_PATH=$PWD/tmp -j8
>
> > 2.   Serialize the installation steps, that is, works less efficiently
>
> Even in the existing system it happens in serially.

The existing code runs in parallel.

 1.  Copy the module "foo.ko" to the destination
 2.  Sign the module "bar.ko"
 3.  Compress the module "baz.ko"

Those three have no dependency among them, so
should be able to run in parallel.

Your code serializes 1 -> 2 -> 3



> And the existing
> method takes more time than the proposed version.
>
> root@ph5dev:~/linux-6.3.5 # ./test.sh orig
>
> real    0m14.699s
> user    0m55.519s
> sys     0m9.036s
>
> root@ph5dev:~/linux-6.3.5 # ./test.sh new
>
> real    0m13.327s
> user    0m46.885s
> sys     0m6.770s
>
> Here is my test script.
> ```
> #!/bin/bash
>
> set -e
>
> if [ "$1" != "new" ] && [ "$1" != "orig" ]; then
>    echo "invalid arg, ($0 [orig|new])" >&2
>    exit 1
> fi
>
> rm -rf $PWD/tmp
>
> s="scripts/sign-file.c"
> m="scripts/Makefile.modinst"
> fns=($s $m)
>
> for f in ${fns[@]}; do
>      cp $f.$1 $f
> done
>
> cd scripts
> gcc -o sign-file sign-file.c -lcrypto
> cd -
>
> time make modules_install INSTALL_MOD_PATH=$PWD/tmp -s -j$(nproc)
> ```
>
> > 3.   Increase code without adding any benefits.
> >Agree with increased code but this change is one step closer to Unix
> philosophy, do one thing well wrt modules_install.


I do not understand why "closer to Unix philosophy"?

You are adding extra/unnecessary complexity.

Currently, the parallel job is managed by Make's job server.

You are introducing another way of parallel execution
in scripts/signfile.sh
(and you completely ignored  -j <jobs> option to Make,
and always spawned $(nproc) threads).


Leave the parallel execution GNU Make.
That is how Kbuild works _properly_.




> > There is no good reason to do these chang >I hope the data I provided above to your 2nd point provides evidence
> that this fix is improving existing system. Please take a look again.


I saw it.   I re-confirmed this is not an improvement.  Thanks for the data.

As I replied to the other thread, my measurement did not show an
attractive result.
https://lore.kernel.org/lkml/CAK7LNATNRchNoj0Y6sdb+_81xwV3kAX57-w5q2zew-q8RyzJVg@mail.gmail.com/T/#m8234fc76e631363fbe6bdfb6e45ef6727fc48e80


>
> > NACK.
>
> Hi Masahiro Yamada,
>
> Replies inline above.
>
> Please correct me if my understanding is wrong. Thanks a lot for your
> time and patience. Have a nice time ahead.


I must let you know you are misunderstanding
the meaning of NACK.


NACK means:
 "I do not like it. Please do not submit it any more".



--
Best Regards
Masahiro Yamada
Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Shreenidhi Shedi 2 years, 6 months ago
On 08/08/23 00:14, Masahiro Yamada wrote:
> On Mon, Aug 7, 2023 at 5:08 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>>
>> On 07/08/23 01:02, Masahiro Yamada wrote:
>>> On Fri, Jun 23, 2023 at 11:54 PM Shreenidhi Shedi <yesshedi@gmail.com> wrote:
>>>>
>>>> Currently Makefile.modinst does three tasks on each module built:
>>>> - Install modules
>>>> - Sign modules
>>>> - Compress modules
>>>>
>>>> All the above tasks happen from a single place.
>>>>
>>>> This patch divides this task further and uses a different makefile for
>>>> each task.
>>>> Signing module logic is completely refactored and everything happens
>>>> from a shell script now.
>>>>
>>>> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
>>>
>>>
>>> This patch is bad in multiple ways.
>>>
>>> 1. Break "make modules_sign"
>>
>> Correct, somehow I missed it. I will fix it.
>> I'm using below command to test sign only option. Please let me know if
>> I should use something else.
>>
>> make modules_sign modules_sign_only=1 INSTALL_MOD_PATH=$PWD/tmp -j8
>>
>>> 2.   Serialize the installation steps, that is, works less efficiently
>>
>> Even in the existing system it happens in serially.
> 
> The existing code runs in parallel.
> 
>   1.  Copy the module "foo.ko" to the destination
>   2.  Sign the module "bar.ko"
>   3.  Compress the module "baz.ko"
> 
> Those three have no dependency among them, so
> should be able to run in parallel.
> 
> Your code serializes 1 -> 2 -> 3
> 
> 
> 
>> And the existing
>> method takes more time than the proposed version.
>>
>> root@ph5dev:~/linux-6.3.5 # ./test.sh orig
>>
>> real    0m14.699s
>> user    0m55.519s
>> sys     0m9.036s
>>
>> root@ph5dev:~/linux-6.3.5 # ./test.sh new
>>
>> real    0m13.327s
>> user    0m46.885s
>> sys     0m6.770s
>>
>> Here is my test script.
>> ```
>> #!/bin/bash
>>
>> set -e
>>
>> if [ "$1" != "new" ] && [ "$1" != "orig" ]; then
>>     echo "invalid arg, ($0 [orig|new])" >&2
>>     exit 1
>> fi
>>
>> rm -rf $PWD/tmp
>>
>> s="scripts/sign-file.c"
>> m="scripts/Makefile.modinst"
>> fns=($s $m)
>>
>> for f in ${fns[@]}; do
>>       cp $f.$1 $f
>> done
>>
>> cd scripts
>> gcc -o sign-file sign-file.c -lcrypto
>> cd -
>>
>> time make modules_install INSTALL_MOD_PATH=$PWD/tmp -s -j$(nproc)
>> ```
>>
>>> 3.   Increase code without adding any benefits.
>>> Agree with increased code but this change is one step closer to Unix
>> philosophy, do one thing well wrt modules_install.
> 
> 
> I do not understand why "closer to Unix philosophy"?
> 
> You are adding extra/unnecessary complexity.
> 
> Currently, the parallel job is managed by Make's job server.
> 
> You are introducing another way of parallel execution
> in scripts/signfile.sh
> (and you completely ignored  -j <jobs> option to Make,
> and always spawned $(nproc) threads).
> 
> 
> Leave the parallel execution GNU Make.
> That is how Kbuild works _properly_.
> 
> 
> 
> 
>>> There is no good reason to do these chang >I hope the data I provided above to your 2nd point provides evidence
>> that this fix is improving existing system. Please take a look again.
> 
> 
> I saw it.   I re-confirmed this is not an improvement.  Thanks for the data.
> 
> As I replied to the other thread, my measurement did not show an
> attractive result.
> https://lore.kernel.org/lkml/CAK7LNATNRchNoj0Y6sdb+_81xwV3kAX57-w5q2zew-q8RyzJVg@mail.gmail.com/T/#m8234fc76e631363fbe6bdfb6e45ef6727fc48e80
> 
> 
>>
>>> NACK.
>>
>> Hi Masahiro Yamada,
>>
>> Replies inline above.
>>
>> Please correct me if my understanding is wrong. Thanks a lot for your
>> time and patience. Have a nice time ahead.
> 
> 
> I must let you know you are misunderstanding
> the meaning of NACK.
> 
> 
> NACK means:
>   "I do not like it. Please do not submit it any more".
> 
> 
> 
> --
> Best Regards
> Masahiro Yamada

Thanks for all the advice and your time, I'm getting used to this 
process. I dropped Kbuild changes from my patch series v9. PTAL.

Have a nice time ahead.

-- 
Shedi

Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Greg KH 2 years, 6 months ago
On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi wrote:
> Currently Makefile.modinst does three tasks on each module built:
> - Install modules
> - Sign modules
> - Compress modules
> 
> All the above tasks happen from a single place.
> 
> This patch divides this task further and uses a different makefile for
> each task.
> Signing module logic is completely refactored and everything happens
> from a shell script now.
> 
> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
> ---
>  scripts/Makefile.compress |  53 ++++++++++++++++++
>  scripts/Makefile.install  |  66 +++++++++++++++++++++++
>  scripts/Makefile.modinst  | 111 +++-----------------------------------
>  scripts/Makefile.sign     |  37 +++++++++++++
>  scripts/signfile.sh       |  24 +++++++++
>  5 files changed, 186 insertions(+), 105 deletions(-)
>  create mode 100644 scripts/Makefile.compress
>  create mode 100644 scripts/Makefile.install
>  create mode 100644 scripts/Makefile.sign
>  create mode 100755 scripts/signfile.sh

As you are touching the build process, you should always cc: the proper
mailing list, and the KBUILD maintainer.  Please do so for this series,
as that is the proper tree for this to go through.

thanks,

greg k-h
Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Shreenidhi Shedi 2 years, 6 months ago
On 04/08/23 19:36, Greg KH wrote:
> On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi wrote:
>> Currently Makefile.modinst does three tasks on each module built:
>> - Install modules
>> - Sign modules
>> - Compress modules
>>
>> All the above tasks happen from a single place.
>>
>> This patch divides this task further and uses a different makefile for
>> each task.
>> Signing module logic is completely refactored and everything happens
>> from a shell script now.
>>
>> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
>> ---
>>   scripts/Makefile.compress |  53 ++++++++++++++++++
>>   scripts/Makefile.install  |  66 +++++++++++++++++++++++
>>   scripts/Makefile.modinst  | 111 +++-----------------------------------
>>   scripts/Makefile.sign     |  37 +++++++++++++
>>   scripts/signfile.sh       |  24 +++++++++
>>   5 files changed, 186 insertions(+), 105 deletions(-)
>>   create mode 100644 scripts/Makefile.compress
>>   create mode 100644 scripts/Makefile.install
>>   create mode 100644 scripts/Makefile.sign
>>   create mode 100755 scripts/signfile.sh
> 
> As you are touching the build process, you should always cc: the proper
> mailing list, and the KBUILD maintainer.  Please do so for this series,
> as that is the proper tree for this to go through.
> 
> thanks,
> 
> greg k-h

Thanks for the inputs Greg.

CC-ing linux-kbuild@vger.kernel.org as suggested.

-- 
Shedi
Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Greg KH 2 years, 6 months ago
On Sun, Aug 06, 2023 at 12:30:22AM +0530, Shreenidhi Shedi wrote:
> On 04/08/23 19:36, Greg KH wrote:
> > On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi wrote:
> > > Currently Makefile.modinst does three tasks on each module built:
> > > - Install modules
> > > - Sign modules
> > > - Compress modules
> > > 
> > > All the above tasks happen from a single place.
> > > 
> > > This patch divides this task further and uses a different makefile for
> > > each task.
> > > Signing module logic is completely refactored and everything happens
> > > from a shell script now.
> > > 
> > > Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
> > > ---
> > >   scripts/Makefile.compress |  53 ++++++++++++++++++
> > >   scripts/Makefile.install  |  66 +++++++++++++++++++++++
> > >   scripts/Makefile.modinst  | 111 +++-----------------------------------
> > >   scripts/Makefile.sign     |  37 +++++++++++++
> > >   scripts/signfile.sh       |  24 +++++++++
> > >   5 files changed, 186 insertions(+), 105 deletions(-)
> > >   create mode 100644 scripts/Makefile.compress
> > >   create mode 100644 scripts/Makefile.install
> > >   create mode 100644 scripts/Makefile.sign
> > >   create mode 100755 scripts/signfile.sh
> > 
> > As you are touching the build process, you should always cc: the proper
> > mailing list, and the KBUILD maintainer.  Please do so for this series,
> > as that is the proper tree for this to go through.
> > 
> > thanks,
> > 
> > greg k-h
> 
> Thanks for the inputs Greg.
> 
> CC-ing linux-kbuild@vger.kernel.org as suggested.

This doesn't actually do anything, sorry.  Please resend the whole
patchset again and add the proper people and list.

thanks,

greg k-h
Re: [PATCH v7 8/8] kbuild: modinst: do modules_install step by step
Posted by Shreenidhi Shedi 2 years, 6 months ago
On 06/08/23 12:15, Greg KH wrote:
> On Sun, Aug 06, 2023 at 12:30:22AM +0530, Shreenidhi Shedi wrote:
>> On 04/08/23 19:36, Greg KH wrote:
>>> On Fri, Jun 23, 2023 at 08:23:58PM +0530, Shreenidhi Shedi wrote:
>>>> Currently Makefile.modinst does three tasks on each module built:
>>>> - Install modules
>>>> - Sign modules
>>>> - Compress modules
>>>>
>>>> All the above tasks happen from a single place.
>>>>
>>>> This patch divides this task further and uses a different makefile for
>>>> each task.
>>>> Signing module logic is completely refactored and everything happens
>>>> from a shell script now.
>>>>
>>>> Signed-off-by: Shreenidhi Shedi <yesshedi@gmail.com>
>>>> ---
>>>>    scripts/Makefile.compress |  53 ++++++++++++++++++
>>>>    scripts/Makefile.install  |  66 +++++++++++++++++++++++
>>>>    scripts/Makefile.modinst  | 111 +++-----------------------------------
>>>>    scripts/Makefile.sign     |  37 +++++++++++++
>>>>    scripts/signfile.sh       |  24 +++++++++
>>>>    5 files changed, 186 insertions(+), 105 deletions(-)
>>>>    create mode 100644 scripts/Makefile.compress
>>>>    create mode 100644 scripts/Makefile.install
>>>>    create mode 100644 scripts/Makefile.sign
>>>>    create mode 100755 scripts/signfile.sh
>>>
>>> As you are touching the build process, you should always cc: the proper
>>> mailing list, and the KBUILD maintainer.  Please do so for this series,
>>> as that is the proper tree for this to go through.
>>>
>>> thanks,
>>>
>>> greg k-h
>>
>> Thanks for the inputs Greg.
>>
>> CC-ing linux-kbuild@vger.kernel.org as suggested.
> 
> This doesn't actually do anything, sorry.  Please resend the whole
> patchset again and add the proper people and list.
> 
> thanks,
> 
> greg k-h

Done. Addressed comments from Masahiro Yamada and sent a new patch 
series, hopefully I have added everyone this time :) Thanks.

-- 
Shedi