| Message ID | 20211215172349.388497-1-willmcvicker@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] kbuild: install the modules.order for external modules | expand |
(Cc: Lucas De Marchi) On Thu, Dec 16, 2021 at 2:23 AM Will McVicker <willmcvicker@google.com> wrote: > > Add support to install the modules.order file for external modules > during module_install in order to retain the Makefile ordering > of external modules. This helps reduce the extra steps necessary to > properly order loading of external modules when there are multiple > kernel modules compiled within a given KBUILD_EXTMOD directory. > > To handle compiling multiple external modules within the same > INSTALL_MOD_DIR, kbuild will append a suffix to the installed > modules.order file defined like so: > > echo ${KBUILD_EXTMOD} | sed 's:[./_]:_:g' > > Ex: > KBUILD_EXTMOD=/mnt/a.b/c-d/my_driver results in: > modules.order._mnt_a_b_c_d_my_driver > > The installed module.order.$(extmod_suffix) files can then be cat'd > together to create a single modules.order file which would define the > order to load all of the modules during boot. So, the user must do this manually? cat extra/modules.order._mnt_a_b_c_d_my_driver \ extra/modules.order._mnt_e_f_g_h_my_driver \ >> modules.order This is so ugly, and incomplete. I cc'ed the kmod maintainer, who may have comments or better approach. > Signed-off-by: Will McVicker <willmcvicker@google.com> > --- > scripts/Makefile.modinst | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst > index ff9b09e4cfca..2e2e31696fd6 100644 > --- a/scripts/Makefile.modinst > +++ b/scripts/Makefile.modinst > @@ -24,6 +24,10 @@ suffix-$(CONFIG_MODULE_COMPRESS_XZ) := .xz > suffix-$(CONFIG_MODULE_COMPRESS_ZSTD) := .zst > > modules := $(patsubst $(extmod_prefix)%, $(dst)/%$(suffix-y), $(modules)) > +ifneq ($(KBUILD_EXTMOD),) > +extmod_suffix := $(subst /,_,$(subst .,_,$(subst -,_,$(KBUILD_EXTMOD)))) > +modules += $(dst)/modules.order.$(extmod_suffix) > +endif > > __modinst: $(modules) > @: > @@ -82,6 +86,12 @@ $(dst)/%.ko: $(extmod_prefix)%.ko FORCE > $(call cmd,strip) > $(call cmd,sign) > > +ifneq ($(KBUILD_EXTMOD),) > +$(dst)/modules.order.$(extmod_suffix): $(MODORDER) FORCE > + $(call cmd,install) > + @sed -i "s:^$(KBUILD_EXTMOD):$(INSTALL_MOD_DIR):g" $@ > +endif > + > else > > $(dst)/%.ko: FORCE > -- > 2.34.1.173.g76aa8bc2d0-goog >
On Fri, Dec 17, 2021 at 5:01 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > (Cc: Lucas De Marchi) > > On Thu, Dec 16, 2021 at 2:23 AM Will McVicker <willmcvicker@google.com> wrote: > > > > Add support to install the modules.order file for external modules > > during module_install in order to retain the Makefile ordering > > of external modules. This helps reduce the extra steps necessary to > > properly order loading of external modules when there are multiple > > kernel modules compiled within a given KBUILD_EXTMOD directory. > > > > To handle compiling multiple external modules within the same > > INSTALL_MOD_DIR, kbuild will append a suffix to the installed > > modules.order file defined like so: > > > > echo ${KBUILD_EXTMOD} | sed 's:[./_]:_:g' > > > > Ex: > > KBUILD_EXTMOD=/mnt/a.b/c-d/my_driver results in: > > modules.order._mnt_a_b_c_d_my_driver > > > > The installed module.order.$(extmod_suffix) files can then be cat'd > > together to create a single modules.order file which would define the > > order to load all of the modules during boot. > > > So, the user must do this manually? > > cat extra/modules.order._mnt_a_b_c_d_my_driver \ > extra/modules.order._mnt_e_f_g_h_my_driver \ > >> modules.order > > This is so ugly, and incomplete. > > I cc'ed the kmod maintainer, who may have > comments or better approach. > > > > > > > > Signed-off-by: Will McVicker <willmcvicker@google.com> > > --- > > scripts/Makefile.modinst | 10 ++++++++++ > > 1 file changed, 10 insertions(+) > > > > diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst > > index ff9b09e4cfca..2e2e31696fd6 100644 > > --- a/scripts/Makefile.modinst > > +++ b/scripts/Makefile.modinst > > @@ -24,6 +24,10 @@ suffix-$(CONFIG_MODULE_COMPRESS_XZ) := .xz > > suffix-$(CONFIG_MODULE_COMPRESS_ZSTD) := .zst > > > > modules := $(patsubst $(extmod_prefix)%, $(dst)/%$(suffix-y), $(modules)) > > +ifneq ($(KBUILD_EXTMOD),) > > +extmod_suffix := $(subst /,_,$(subst .,_,$(subst -,_,$(KBUILD_EXTMOD)))) > > +modules += $(dst)/modules.order.$(extmod_suffix) > > +endif > > > > __modinst: $(modules) > > @: > > @@ -82,6 +86,12 @@ $(dst)/%.ko: $(extmod_prefix)%.ko FORCE > > $(call cmd,strip) > > $(call cmd,sign) > > > > +ifneq ($(KBUILD_EXTMOD),) > > +$(dst)/modules.order.$(extmod_suffix): $(MODORDER) FORCE > > + $(call cmd,install) > > + @sed -i "s:^$(KBUILD_EXTMOD):$(INSTALL_MOD_DIR):g" $@ > > +endif > > + > > else > > > > $(dst)/%.ko: FORCE > > -- > > 2.34.1.173.g76aa8bc2d0-goog > > > > > -- > Best Regards > Masahiro Yamada > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. > Hi Masahiro, Thanks for your response! I agree with you that this is ugly and would love feedback on the direction to take this. With regards to incompleteness, the existing kbuild implementation for external modules is already incomplete in the sense that it doesn't even attempt to install the already generated modules.order files to INSTALL_MOD_DIR. I believe this patch gets us a little closer to closing the gap, but agree it's nowhere complete. I would be happy to fully close the gap if I knew that it would be accepted or welcomed. Let me give you some insight on how we currently do this on Android and how this patch changes that. Currently, the Android build.sh script (which is used to compile the Android Common Kernel) supports the build variable EXT_MODULES which is a list of paths to external modules to be compiled. The build script loops through this list and calls "make modules" and "make modules_install" to compile and install the external modules. Then, the build script creates the modules.order file like this: cd ${MODLIB} find extra -type f -name "*.ko" | sort >> modules.order Sorting is used so that the modules.order file is deterministic. This proposed patch allows us to use the Makefile defined ordering instead of this sorted ordering. In Android the paths to external modules must be relative to the kernel repository. For example, the kernel and all external modules are all cloned within a root directory and the paths in EXT_MODULES are all relative to the root directory. So our build.sh script can use the relative path from the root directory as part of INSTALL_MOD_DIR to get rid of the suffix ugliness. For example, we can do this: for EXT_MOD in ${EXT_MODULES}; do # make modules make -C ${EXT_MOD} ... # make modules_install make -C ${EXT_MOD} ... INSTALL_MOD_DIR="extra/${EXT_MOD}" modules_install done for EXT_MOD in ${EXT_MODULES}; do modules_order_file=$(ls ${MODLIB}/extra/${EXT_MOD}/modules.order.*) cat ${modules_order_file} >> ${MODLIB}/modules.order done Since kbuild doesn't know about how many external modules there are nor does it retain any information about each individual call to "make modules" or "make modules_install", we can't do the concatenation within the Makefile.modinst script. To close the gap, we could add an additional make target that one could call after all of the external modules have been installed to do this concatenation, but I wasn't sure how open everyone would be to accepting this. Let me know your thoughts on that. Thanks, Will
On Mon, Dec 20, 2021 at 10:13 AM Will McVicker <willmcvicker@google.com> wrote: > > On Fri, Dec 17, 2021 at 5:01 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > (Cc: Lucas De Marchi) > > > > On Thu, Dec 16, 2021 at 2:23 AM Will McVicker <willmcvicker@google.com> wrote: > > > > > > Add support to install the modules.order file for external modules > > > during module_install in order to retain the Makefile ordering > > > of external modules. This helps reduce the extra steps necessary to > > > properly order loading of external modules when there are multiple > > > kernel modules compiled within a given KBUILD_EXTMOD directory. > > > > > > To handle compiling multiple external modules within the same > > > INSTALL_MOD_DIR, kbuild will append a suffix to the installed > > > modules.order file defined like so: > > > > > > echo ${KBUILD_EXTMOD} | sed 's:[./_]:_:g' > > > > > > Ex: > > > KBUILD_EXTMOD=/mnt/a.b/c-d/my_driver results in: > > > modules.order._mnt_a_b_c_d_my_driver > > > > > > The installed module.order.$(extmod_suffix) files can then be cat'd > > > together to create a single modules.order file which would define the > > > order to load all of the modules during boot. > > > > > > So, the user must do this manually? > > > > cat extra/modules.order._mnt_a_b_c_d_my_driver \ > > extra/modules.order._mnt_e_f_g_h_my_driver \ > > >> modules.order > > > > This is so ugly, and incomplete. > > > > I cc'ed the kmod maintainer, who may have > > comments or better approach. > > > > > > > > > > > > > > > Signed-off-by: Will McVicker <willmcvicker@google.com> > > > --- > > > scripts/Makefile.modinst | 10 ++++++++++ > > > 1 file changed, 10 insertions(+) > > > > > > diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst > > > index ff9b09e4cfca..2e2e31696fd6 100644 > > > --- a/scripts/Makefile.modinst > > > +++ b/scripts/Makefile.modinst > > > @@ -24,6 +24,10 @@ suffix-$(CONFIG_MODULE_COMPRESS_XZ) := .xz > > > suffix-$(CONFIG_MODULE_COMPRESS_ZSTD) := .zst > > > > > > modules := $(patsubst $(extmod_prefix)%, $(dst)/%$(suffix-y), $(modules)) > > > +ifneq ($(KBUILD_EXTMOD),) > > > +extmod_suffix := $(subst /,_,$(subst .,_,$(subst -,_,$(KBUILD_EXTMOD)))) > > > +modules += $(dst)/modules.order.$(extmod_suffix) > > > +endif > > > > > > __modinst: $(modules) > > > @: > > > @@ -82,6 +86,12 @@ $(dst)/%.ko: $(extmod_prefix)%.ko FORCE > > > $(call cmd,strip) > > > $(call cmd,sign) > > > > > > +ifneq ($(KBUILD_EXTMOD),) > > > +$(dst)/modules.order.$(extmod_suffix): $(MODORDER) FORCE > > > + $(call cmd,install) > > > + @sed -i "s:^$(KBUILD_EXTMOD):$(INSTALL_MOD_DIR):g" $@ > > > +endif > > > + > > > else > > > > > > $(dst)/%.ko: FORCE > > > -- > > > 2.34.1.173.g76aa8bc2d0-goog > > > > > > > > > -- > > Best Regards > > Masahiro Yamada > > > > -- > > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. > > > > Hi Masahiro, > > Thanks for your response! I agree with you that this is ugly and would > love feedback on the direction to take this. With regards to > incompleteness, the existing kbuild implementation for external > modules is already incomplete in the sense that it doesn't even > attempt to install the already generated modules.order files to > INSTALL_MOD_DIR. I believe this patch gets us a little closer to > closing the gap, but agree it's nowhere complete. I would be happy to > fully close the gap if I knew that it would be accepted or welcomed. > Let me give you some insight on how we currently do this on Android > and how this patch changes that. > > Currently, the Android build.sh script (which is used to compile the > Android Common Kernel) supports the build variable EXT_MODULES which > is a list of paths to external modules to be compiled. The build > script loops through this list and calls "make modules" and "make > modules_install" to compile and install the external modules. Then, > the build script creates the modules.order file like this: > > cd ${MODLIB} > find extra -type f -name "*.ko" | sort >> modules.order > > Sorting is used so that the modules.order file is deterministic. This > proposed patch allows us to use the Makefile defined ordering instead > of this sorted ordering. In Android the paths to external modules must > be relative to the kernel repository. For example, the kernel and all > external modules are all cloned within a root directory and the paths > in EXT_MODULES are all relative to the root directory. So our build.sh > script can use the relative path from the root directory as part of > INSTALL_MOD_DIR to get rid of the suffix ugliness. For example, we can > do this: > > for EXT_MOD in ${EXT_MODULES}; do > # make modules > make -C ${EXT_MOD} ... > # make modules_install > make -C ${EXT_MOD} ... INSTALL_MOD_DIR="extra/${EXT_MOD}" modules_install > done > > for EXT_MOD in ${EXT_MODULES}; do > modules_order_file=$(ls ${MODLIB}/extra/${EXT_MOD}/modules.order.*) > cat ${modules_order_file} >> ${MODLIB}/modules.order > done > > Since kbuild doesn't know about how many external modules there are > nor does it retain any information about each individual call to "make > modules" or "make modules_install", we can't do the concatenation > within the Makefile.modinst script. To close the gap, we could add an > additional make target that one could call after all of the external > modules have been installed to do this concatenation, but I wasn't > sure how open everyone would be to accepting this. Let me know your > thoughts on that. > > Thanks, > Will Friendly reminder on this patch now that most are back from the holiday break. Lucas, could you comment please? Thanks, Will
diff --git a/scripts/Makefile.modinst b/scripts/Makefile.modinst index ff9b09e4cfca..2e2e31696fd6 100644 --- a/scripts/Makefile.modinst +++ b/scripts/Makefile.modinst @@ -24,6 +24,10 @@ suffix-$(CONFIG_MODULE_COMPRESS_XZ) := .xz suffix-$(CONFIG_MODULE_COMPRESS_ZSTD) := .zst modules := $(patsubst $(extmod_prefix)%, $(dst)/%$(suffix-y), $(modules)) +ifneq ($(KBUILD_EXTMOD),) +extmod_suffix := $(subst /,_,$(subst .,_,$(subst -,_,$(KBUILD_EXTMOD)))) +modules += $(dst)/modules.order.$(extmod_suffix) +endif __modinst: $(modules) @: @@ -82,6 +86,12 @@ $(dst)/%.ko: $(extmod_prefix)%.ko FORCE $(call cmd,strip) $(call cmd,sign) +ifneq ($(KBUILD_EXTMOD),) +$(dst)/modules.order.$(extmod_suffix): $(MODORDER) FORCE + $(call cmd,install) + @sed -i "s:^$(KBUILD_EXTMOD):$(INSTALL_MOD_DIR):g" $@ +endif + else $(dst)/%.ko: FORCE
Add support to install the modules.order file for external modules during module_install in order to retain the Makefile ordering of external modules. This helps reduce the extra steps necessary to properly order loading of external modules when there are multiple kernel modules compiled within a given KBUILD_EXTMOD directory. To handle compiling multiple external modules within the same INSTALL_MOD_DIR, kbuild will append a suffix to the installed modules.order file defined like so: echo ${KBUILD_EXTMOD} | sed 's:[./_]:_:g' Ex: KBUILD_EXTMOD=/mnt/a.b/c-d/my_driver results in: modules.order._mnt_a_b_c_d_my_driver The installed module.order.$(extmod_suffix) files can then be cat'd together to create a single modules.order file which would define the order to load all of the modules during boot. Signed-off-by: Will McVicker <willmcvicker@google.com> --- scripts/Makefile.modinst | 10 ++++++++++ 1 file changed, 10 insertions(+)