diff mbox series

[2/4] modpost: dump missing namespaces into a single modules.nsdeps file

Message ID 20191029123809.29301-3-yamada.masahiro@socionext.com
State Accepted
Commit bbc55bded4aaf47d6f2bd9389fc8d3a3821d18c0
Headers show
Series More nsdeps improvements | expand

Commit Message

Masahiro Yamada Oct. 29, 2019, 12:38 p.m. UTC 
The modpost, with the -d option given, generates per-module .ns_deps
files.

Kbuild generates per-module .mod files to carry module information.
This is convenient because Make handles multiple jobs when the -j
option is given.

On the other hand, the modpost always runs as a single thread.
I do not see a strong reason to produce separate .ns_deps files.

This commit changes the modpost to generate just one file,
modules.nsdeps, each line of which has the following format:

  <module_name>: <list of missing namespaces>

Please note it contains *missing* namespaces instead of required ones.
So, modules.nsdeps is empty if the namespace dependency is all good.

This will work more efficiently because spatch will no longer process
already imported namespaces. I removed the '(if needed)' from the
nsdeps log since spatch is invoked only when needed.

This also solved the stale .ns_deps files problem reported by
Jessica Yu:

  https://lkml.org/lkml/2019/10/28/467

While I was here, I improved the modpost code a little more;
I freed ns_deps_bus.p because buf_write() allocates memory.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

---

 .gitignore               |  2 +-
 Documentation/dontdiff   |  1 +
 Makefile                 |  4 ++--
 scripts/Makefile.modpost |  2 +-
 scripts/mod/modpost.c    | 44 +++++++++++++++++-----------------------
 scripts/mod/modpost.h    |  4 ++--
 scripts/nsdeps           | 21 +++++++++----------
 7 files changed, 36 insertions(+), 42 deletions(-)

-- 
2.17.1

Comments

Jessica Yu Oct. 30, 2019, 8:11 p.m. UTC | #1 
+++ Masahiro Yamada [29/10/19 21:38 +0900]:
>The modpost, with the -d option given, generates per-module .ns_deps

>files.

>

>Kbuild generates per-module .mod files to carry module information.

>This is convenient because Make handles multiple jobs when the -j

>option is given.

>

>On the other hand, the modpost always runs as a single thread.

>I do not see a strong reason to produce separate .ns_deps files.

>

>This commit changes the modpost to generate just one file,

>modules.nsdeps, each line of which has the following format:

>

>  <module_name>: <list of missing namespaces>

>

>Please note it contains *missing* namespaces instead of required ones.

>So, modules.nsdeps is empty if the namespace dependency is all good.

>

>This will work more efficiently because spatch will no longer process

>already imported namespaces. I removed the '(if needed)' from the

>nsdeps log since spatch is invoked only when needed.


This is a nice optimization! :-)

>This also solved the stale .ns_deps files problem reported by

>Jessica Yu:

>

>  https://lkml.org/lkml/2019/10/28/467


Tested-by: Jessica Yu <jeyu@kernel.org>

Acked-by: Jessica Yu <jeyu@kernel.org>


Thanks for the fix!

>While I was here, I improved the modpost code a little more;

>I freed ns_deps_bus.p because buf_write() allocates memory.

>

>Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>---

>

> .gitignore               |  2 +-

> Documentation/dontdiff   |  1 +

> Makefile                 |  4 ++--

> scripts/Makefile.modpost |  2 +-

> scripts/mod/modpost.c    | 44 +++++++++++++++++-----------------------

> scripts/mod/modpost.h    |  4 ++--

> scripts/nsdeps           | 21 +++++++++----------

> 7 files changed, 36 insertions(+), 42 deletions(-)

>

>diff --git a/.gitignore b/.gitignore

>index 70580bdd352c..72ef86a5570d 100644

>--- a/.gitignore

>+++ b/.gitignore

>@@ -32,7 +32,6 @@

> *.lzo

> *.mod

> *.mod.c

>-*.ns_deps

> *.o

> *.o.*

> *.patch

>@@ -61,6 +60,7 @@ modules.order

> /System.map

> /Module.markers

> /modules.builtin.modinfo

>+/modules.nsdeps

>

> #

> # RPM spec file (make rpm-pkg)

>diff --git a/Documentation/dontdiff b/Documentation/dontdiff

>index 9f4392876099..72fc2e9e2b63 100644

>--- a/Documentation/dontdiff

>+++ b/Documentation/dontdiff

>@@ -179,6 +179,7 @@ mkutf8data

> modpost

> modules.builtin

> modules.builtin.modinfo

>+modules.nsdeps

> modules.order

> modversions.h*

> nconf

>diff --git a/Makefile b/Makefile

>index 0ef897fd9cfd..1e3f307bd49b 100644

>--- a/Makefile

>+++ b/Makefile

>@@ -1356,7 +1356,7 @@ endif # CONFIG_MODULES

>

> # Directories & files removed with 'make clean'

> CLEAN_DIRS  += include/ksym

>-CLEAN_FILES += modules.builtin.modinfo

>+CLEAN_FILES += modules.builtin.modinfo modules.nsdeps

>

> # Directories & files removed with 'make mrproper'

> MRPROPER_DIRS  += include/config include/generated          \

>@@ -1660,7 +1660,7 @@ clean: $(clean-dirs)

> 		-o -name '*.ko.*' \

> 		-o -name '*.dtb' -o -name '*.dtb.S' -o -name '*.dt.yaml' \

> 		-o -name '*.dwo' -o -name '*.lst' \

>-		-o -name '*.su' -o -name '*.mod' -o -name '*.ns_deps' \

>+		-o -name '*.su' -o -name '*.mod' \

> 		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \

> 		-o -name '*.lex.c' -o -name '*.tab.[ch]' \

> 		-o -name '*.asn1.[ch]' \

>diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost

>index c9757b20b048..da37128c3f9f 100644

>--- a/scripts/Makefile.modpost

>+++ b/scripts/Makefile.modpost

>@@ -66,7 +66,7 @@ __modpost:

> else

>

> MODPOST += $(subst -i,-n,$(filter -i,$(MAKEFLAGS))) -s -T - \

>-	$(if $(KBUILD_NSDEPS),-d)

>+	$(if $(KBUILD_NSDEPS),-d modules.nsdeps)

>

> ifeq ($(KBUILD_EXTMOD),)

> MODPOST += $(wildcard vmlinux)

>diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c

>index dcd90d563ce8..f7425f5d4ab0 100644

>--- a/scripts/mod/modpost.c

>+++ b/scripts/mod/modpost.c

>@@ -38,8 +38,6 @@ static int sec_mismatch_count = 0;

> static int sec_mismatch_fatal = 0;

> /* ignore missing files */

> static int ignore_missing_files;

>-/* write namespace dependencies */

>-static int write_namespace_deps;

>

> enum export {

> 	export_plain,      export_unused,     export_gpl,

>@@ -2217,14 +2215,11 @@ static int check_exports(struct module *mod)

> 		else

> 			basename = mod->name;

>

>-		if (exp->namespace) {

>-			add_namespace(&mod->required_namespaces,

>-				      exp->namespace);

>-

>-			if (!module_imports_namespace(mod, exp->namespace)) {

>-				warn("module %s uses symbol %s from namespace %s, but does not import it.\n",

>-				     basename, exp->name, exp->namespace);

>-			}

>+		if (exp->namespace &&

>+		    !module_imports_namespace(mod, exp->namespace)) {

>+			warn("module %s uses symbol %s from namespace %s, but does not import it.\n",

>+			     basename, exp->name, exp->namespace);

>+			add_namespace(&mod->missing_namespaces, exp->namespace);

> 		}

>

> 		if (!mod->gpl_compatible)

>@@ -2525,29 +2520,27 @@ static void write_dump(const char *fname)

> 	free(buf.p);

> }

>

>-static void write_namespace_deps_files(void)

>+static void write_namespace_deps_files(const char *fname)

> {

> 	struct module *mod;

> 	struct namespace_list *ns;

> 	struct buffer ns_deps_buf = {};

>

> 	for (mod = modules; mod; mod = mod->next) {

>-		char fname[PATH_MAX];

>

>-		if (mod->skip)

>+		if (mod->skip || !mod->missing_namespaces)

> 			continue;

>

>-		ns_deps_buf.pos = 0;

>+		buf_printf(&ns_deps_buf, "%s.ko:", mod->name);

>

>-		for (ns = mod->required_namespaces; ns; ns = ns->next)

>-			buf_printf(&ns_deps_buf, "%s\n", ns->namespace);

>+		for (ns = mod->missing_namespaces; ns; ns = ns->next)

>+			buf_printf(&ns_deps_buf, " %s", ns->namespace);

>

>-		if (ns_deps_buf.pos == 0)

>-			continue;

>-

>-		sprintf(fname, "%s.ns_deps", mod->name);

>-		write_if_changed(&ns_deps_buf, fname);

>+		buf_printf(&ns_deps_buf, "\n");

> 	}

>+

>+	write_if_changed(&ns_deps_buf, fname);

>+	free(ns_deps_buf.p);

> }

>

> struct ext_sym_list {

>@@ -2560,6 +2553,7 @@ int main(int argc, char **argv)

> 	struct module *mod;

> 	struct buffer buf = { };

> 	char *kernel_read = NULL;

>+	char *missing_namespace_deps = NULL;

> 	char *dump_write = NULL, *files_source = NULL;

> 	int opt;

> 	int err;

>@@ -2567,7 +2561,7 @@ int main(int argc, char **argv)

> 	struct ext_sym_list *extsym_iter;

> 	struct ext_sym_list *extsym_start = NULL;

>

>-	while ((opt = getopt(argc, argv, "i:e:mnsT:o:awEd")) != -1) {

>+	while ((opt = getopt(argc, argv, "i:e:mnsT:o:awEd:")) != -1) {

> 		switch (opt) {

> 		case 'i':

> 			kernel_read = optarg;

>@@ -2606,7 +2600,7 @@ int main(int argc, char **argv)

> 			sec_mismatch_fatal = 1;

> 			break;

> 		case 'd':

>-			write_namespace_deps = 1;

>+			missing_namespace_deps = optarg;

> 			break;

> 		default:

> 			exit(1);

>@@ -2654,8 +2648,8 @@ int main(int argc, char **argv)

> 		write_if_changed(&buf, fname);

> 	}

>

>-	if (write_namespace_deps)

>-		write_namespace_deps_files();

>+	if (missing_namespace_deps)

>+		write_namespace_deps_files(missing_namespace_deps);

>

> 	if (dump_write)

> 		write_dump(dump_write);

>diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h

>index ad271bc6c313..fe6652535e4b 100644

>--- a/scripts/mod/modpost.h

>+++ b/scripts/mod/modpost.h

>@@ -126,8 +126,8 @@ struct module {

> 	struct buffer dev_table_buf;

> 	char	     srcversion[25];

> 	int is_dot_o;

>-	// Required namespace dependencies

>-	struct namespace_list *required_namespaces;

>+	// Missing namespace dependencies

>+	struct namespace_list *missing_namespaces;

> 	// Actual imported namespaces

> 	struct namespace_list *imported_namespaces;

> };

>diff --git a/scripts/nsdeps b/scripts/nsdeps

>index dda6fbac016e..08db427a7fe5 100644

>--- a/scripts/nsdeps

>+++ b/scripts/nsdeps

>@@ -27,15 +27,14 @@ generate_deps_for_ns() {

> }

>

> generate_deps() {

>-	local mod_name=`basename $@ .ko`

>-	local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'`

>-	local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'`

>-	if [ ! -f "$ns_deps_file" ]; then return; fi

>-	local mod_source_files=`cat $mod_file | sed -n 1p                      \

>+	local mod=${1%.ko:}

>+	shift

>+	local namespaces="$*"

>+	local mod_source_files=`cat $mod.mod | sed -n 1p                      \

> 					      | sed -e 's/\.o/\.c/g'           \

> 					      | sed "s|[^ ]* *|${srctree}/&|g"`

>-	for ns in `cat $ns_deps_file`; do

>-		echo "Adding namespace $ns to module $mod_name (if needed)."

>+	for ns in $namespaces; do

>+		echo "Adding namespace $ns to module $mod.ko."

> 		generate_deps_for_ns $ns $mod_source_files

> 		# sort the imports

> 		for source_file in $mod_source_files; do

>@@ -52,7 +51,7 @@ generate_deps() {

> 	done

> }

>

>-for f in `cat $objtree/modules.order`; do

>-	generate_deps $f

>-done

>-

>+while read line

>+do

>+	generate_deps $line

>+done < modules.nsdeps

>-- 

>2.17.1

>
Jessica Yu Oct. 31, 2019, 11:20 a.m. UTC | #2 
+++ Masahiro Yamada [29/10/19 21:38 +0900]:
>The modpost, with the -d option given, generates per-module .ns_deps

>files.

>

>Kbuild generates per-module .mod files to carry module information.

>This is convenient because Make handles multiple jobs when the -j

>option is given.

>

>On the other hand, the modpost always runs as a single thread.

>I do not see a strong reason to produce separate .ns_deps files.

>

>This commit changes the modpost to generate just one file,

>modules.nsdeps, each line of which has the following format:

>

>  <module_name>: <list of missing namespaces>

>

>Please note it contains *missing* namespaces instead of required ones.

>So, modules.nsdeps is empty if the namespace dependency is all good.

>

>This will work more efficiently because spatch will no longer process

>already imported namespaces. I removed the '(if needed)' from the

>nsdeps log since spatch is invoked only when needed.

>

>This also solved the stale .ns_deps files problem reported by

>Jessica Yu:

>

>  https://lkml.org/lkml/2019/10/28/467

>

>While I was here, I improved the modpost code a little more;

>I freed ns_deps_bus.p because buf_write() allocates memory.

>

>Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>---

>

> .gitignore               |  2 +-

> Documentation/dontdiff   |  1 +

> Makefile                 |  4 ++--

> scripts/Makefile.modpost |  2 +-

> scripts/mod/modpost.c    | 44 +++++++++++++++++-----------------------

> scripts/mod/modpost.h    |  4 ++--

> scripts/nsdeps           | 21 +++++++++----------

> 7 files changed, 36 insertions(+), 42 deletions(-)

>

>diff --git a/.gitignore b/.gitignore

>index 70580bdd352c..72ef86a5570d 100644

>--- a/.gitignore

>+++ b/.gitignore

>@@ -32,7 +32,6 @@

> *.lzo

> *.mod

> *.mod.c

>-*.ns_deps

> *.o

> *.o.*

> *.patch

>@@ -61,6 +60,7 @@ modules.order

> /System.map

> /Module.markers

> /modules.builtin.modinfo

>+/modules.nsdeps

>

> #

> # RPM spec file (make rpm-pkg)

>diff --git a/Documentation/dontdiff b/Documentation/dontdiff

>index 9f4392876099..72fc2e9e2b63 100644

>--- a/Documentation/dontdiff

>+++ b/Documentation/dontdiff

>@@ -179,6 +179,7 @@ mkutf8data

> modpost

> modules.builtin

> modules.builtin.modinfo

>+modules.nsdeps

> modules.order

> modversions.h*

> nconf

>diff --git a/Makefile b/Makefile

>index 0ef897fd9cfd..1e3f307bd49b 100644

>--- a/Makefile

>+++ b/Makefile

>@@ -1356,7 +1356,7 @@ endif # CONFIG_MODULES

>

> # Directories & files removed with 'make clean'

> CLEAN_DIRS  += include/ksym

>-CLEAN_FILES += modules.builtin.modinfo

>+CLEAN_FILES += modules.builtin.modinfo modules.nsdeps


Hmm, I tried to run `make -C path/to/kernel/src M=$(PWD) clean` for a test
external module, but it didn't remove modules.nsdeps for me. I declared a
MODULE namespace for testing purposes.

$ make 
make -C /dev/shm/linux M=/tmp/ppyu/test-module 
make[1]: Entering directory '/dev/shm/linux'
  AR      /tmp/ppyu/test-module/built-in.a
  CC [M]  /tmp/ppyu/test-module/test1.o
  CC [M]  /tmp/ppyu/test-module/test2.o
  LD [M]  /tmp/ppyu/test-module/test.o
  Building modules, stage 2.
  MODPOST 1 modules
WARNING: module test uses symbol try_module_get from namespace MODULE, but does not import it.
  CC [M]  /tmp/ppyu/test-module/test.mod.o
  LD [M]  /tmp/ppyu/test-module/test.ko
make[1]: Leaving directory '/dev/shm/linux'

Then I make nsdeps:

make -C /dev/shm/linux M=/tmp/ppyu/test-module nsdeps
make[1]: Entering directory '/dev/shm/linux'
  Building modules, stage 2.
  MODPOST 1 modules
WARNING: module test uses symbol try_module_get from namespace MODULE, but does not import it.
Adding namespace MODULE to module /tmp/ppyu/test-module/test.ko.
--- /tmp/ppyu/test-module/test1.c
+++ /tmp/cocci-output-3696-c1f8b3-test1.c
@@ -38,3 +38,4 @@ static void __exit hello_cleanup(void)
 module_init(hello_init);
 module_exit(hello_cleanup);
 MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(MODULE);
make[1]: Leaving directory '/dev/shm/linux'
$ cat modules.nsdeps
/tmp/ppyu/test-module/test.ko: MODULE

Looks good so far, then I try make clean:

$ make clean
make -C /dev/shm/linux M=/tmp/ppyu/test-module clean
make[1]: Entering directory '/dev/shm/linux'
  CLEAN   /tmp/ppyu/test-module/Module.symvers
make[1]: Leaving directory '/dev/shm/linux'
$ ls
Makefile  modules.nsdeps  test1.c  test2.c

But modules.nsdeps is still there.
Masahiro Yamada Oct. 31, 2019, 11:53 a.m. UTC | #3 
On Thu, Oct 31, 2019 at 8:20 PM Jessica Yu <jeyu@kernel.org> wrote:
>

> +++ Masahiro Yamada [29/10/19 21:38 +0900]:

> >The modpost, with the -d option given, generates per-module .ns_deps

> >files.

> >

> >Kbuild generates per-module .mod files to carry module information.

> >This is convenient because Make handles multiple jobs when the -j

> >option is given.

> >

> >On the other hand, the modpost always runs as a single thread.

> >I do not see a strong reason to produce separate .ns_deps files.

> >

> >This commit changes the modpost to generate just one file,

> >modules.nsdeps, each line of which has the following format:

> >

> >  <module_name>: <list of missing namespaces>

> >

> >Please note it contains *missing* namespaces instead of required ones.

> >So, modules.nsdeps is empty if the namespace dependency is all good.

> >

> >This will work more efficiently because spatch will no longer process

> >already imported namespaces. I removed the '(if needed)' from the

> >nsdeps log since spatch is invoked only when needed.

> >

> >This also solved the stale .ns_deps files problem reported by

> >Jessica Yu:

> >

> >  https://lkml.org/lkml/2019/10/28/467

> >

> >While I was here, I improved the modpost code a little more;

> >I freed ns_deps_bus.p because buf_write() allocates memory.

> >

> >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

> >---

> >

> > .gitignore               |  2 +-

> > Documentation/dontdiff   |  1 +

> > Makefile                 |  4 ++--

> > scripts/Makefile.modpost |  2 +-

> > scripts/mod/modpost.c    | 44 +++++++++++++++++-----------------------

> > scripts/mod/modpost.h    |  4 ++--

> > scripts/nsdeps           | 21 +++++++++----------

> > 7 files changed, 36 insertions(+), 42 deletions(-)

> >

> >diff --git a/.gitignore b/.gitignore

> >index 70580bdd352c..72ef86a5570d 100644

> >--- a/.gitignore

> >+++ b/.gitignore

> >@@ -32,7 +32,6 @@

> > *.lzo

> > *.mod

> > *.mod.c

> >-*.ns_deps

> > *.o

> > *.o.*

> > *.patch

> >@@ -61,6 +60,7 @@ modules.order

> > /System.map

> > /Module.markers

> > /modules.builtin.modinfo

> >+/modules.nsdeps

> >

> > #

> > # RPM spec file (make rpm-pkg)

> >diff --git a/Documentation/dontdiff b/Documentation/dontdiff

> >index 9f4392876099..72fc2e9e2b63 100644

> >--- a/Documentation/dontdiff

> >+++ b/Documentation/dontdiff

> >@@ -179,6 +179,7 @@ mkutf8data

> > modpost

> > modules.builtin

> > modules.builtin.modinfo

> >+modules.nsdeps

> > modules.order

> > modversions.h*

> > nconf

> >diff --git a/Makefile b/Makefile

> >index 0ef897fd9cfd..1e3f307bd49b 100644

> >--- a/Makefile

> >+++ b/Makefile

> >@@ -1356,7 +1356,7 @@ endif # CONFIG_MODULES

> >

> > # Directories & files removed with 'make clean'

> > CLEAN_DIRS  += include/ksym

> >-CLEAN_FILES += modules.builtin.modinfo

> >+CLEAN_FILES += modules.builtin.modinfo modules.nsdeps

>

> Hmm, I tried to run `make -C path/to/kernel/src M=$(PWD) clean` for a test

> external module, but it didn't remove modules.nsdeps for me. I declared a

> MODULE namespace for testing purposes.

>

> $ make

> make -C /dev/shm/linux M=/tmp/ppyu/test-module

> make[1]: Entering directory '/dev/shm/linux'

>   AR      /tmp/ppyu/test-module/built-in.a

>   CC [M]  /tmp/ppyu/test-module/test1.o

>   CC [M]  /tmp/ppyu/test-module/test2.o

>   LD [M]  /tmp/ppyu/test-module/test.o

>   Building modules, stage 2.

>   MODPOST 1 modules

> WARNING: module test uses symbol try_module_get from namespace MODULE, but does not import it.

>   CC [M]  /tmp/ppyu/test-module/test.mod.o

>   LD [M]  /tmp/ppyu/test-module/test.ko

> make[1]: Leaving directory '/dev/shm/linux'

>

> Then I make nsdeps:

>

> make -C /dev/shm/linux M=/tmp/ppyu/test-module nsdeps

> make[1]: Entering directory '/dev/shm/linux'

>   Building modules, stage 2.

>   MODPOST 1 modules

> WARNING: module test uses symbol try_module_get from namespace MODULE, but does not import it.

> Adding namespace MODULE to module /tmp/ppyu/test-module/test.ko.

> --- /tmp/ppyu/test-module/test1.c

> +++ /tmp/cocci-output-3696-c1f8b3-test1.c

> @@ -38,3 +38,4 @@ static void __exit hello_cleanup(void)

>  module_init(hello_init);

>  module_exit(hello_cleanup);

>  MODULE_LICENSE("GPL");

> +MODULE_IMPORT_NS(MODULE);

> make[1]: Leaving directory '/dev/shm/linux'

> $ cat modules.nsdeps

> /tmp/ppyu/test-module/test.ko: MODULE

>

> Looks good so far, then I try make clean:

>

> $ make clean

> make -C /dev/shm/linux M=/tmp/ppyu/test-module clean

> make[1]: Entering directory '/dev/shm/linux'

>   CLEAN   /tmp/ppyu/test-module/Module.symvers

> make[1]: Leaving directory '/dev/shm/linux'

> $ ls

> Makefile  modules.nsdeps  test1.c  test2.c

>

> But modules.nsdeps is still there.

>


Good catch!

I forgot to take care of it for external module builds.

The following should work. I will fold it in 3/4.




diff --git a/Makefile b/Makefile
index 79be70bf2899..6035702803eb 100644
--- a/Makefile
+++ b/Makefile
@@ -1619,7 +1619,7 @@ _emodinst_post: _emodinst_
        $(call cmd,depmod)

 clean-dirs := $(KBUILD_EXTMOD)
-clean: rm-files := $(KBUILD_EXTMOD)/Module.symvers
+clean: rm-files := $(KBUILD_EXTMOD)/Module.symvers
$(KBUILD_EXTMOD)/modules.nsdeps

 PHONY += /
 /:



-- 
Best Regards
Masahiro Yamada
Matthias Maennich Nov. 6, 2019, 3:24 p.m. UTC | #4 
On Thu, Oct 31, 2019 at 08:53:25PM +0900, Masahiro Yamada wrote:
>On Thu, Oct 31, 2019 at 8:20 PM Jessica Yu <jeyu@kernel.org> wrote:

>>

>> +++ Masahiro Yamada [29/10/19 21:38 +0900]:

>> >The modpost, with the -d option given, generates per-module .ns_deps

>> >files.

>> >

>> >Kbuild generates per-module .mod files to carry module information.

>> >This is convenient because Make handles multiple jobs when the -j

>> >option is given.

>> >

>> >On the other hand, the modpost always runs as a single thread.

>> >I do not see a strong reason to produce separate .ns_deps files.

>> >

>> >This commit changes the modpost to generate just one file,

>> >modules.nsdeps, each line of which has the following format:

>> >

>> >  <module_name>: <list of missing namespaces>

>> >

>> >Please note it contains *missing* namespaces instead of required ones.

>> >So, modules.nsdeps is empty if the namespace dependency is all good.

>> >

>> >This will work more efficiently because spatch will no longer process

>> >already imported namespaces. I removed the '(if needed)' from the

>> >nsdeps log since spatch is invoked only when needed.

>> >

>> >This also solved the stale .ns_deps files problem reported by

>> >Jessica Yu:

>> >

>> >  https://lkml.org/lkml/2019/10/28/467

>> >

>> >While I was here, I improved the modpost code a little more;

>> >I freed ns_deps_bus.p because buf_write() allocates memory.

>> >

>> >Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>

>> >---

>> >

>> > .gitignore               |  2 +-

>> > Documentation/dontdiff   |  1 +

>> > Makefile                 |  4 ++--

>> > scripts/Makefile.modpost |  2 +-

>> > scripts/mod/modpost.c    | 44 +++++++++++++++++-----------------------

>> > scripts/mod/modpost.h    |  4 ++--

>> > scripts/nsdeps           | 21 +++++++++----------

>> > 7 files changed, 36 insertions(+), 42 deletions(-)

>> >

>> >diff --git a/.gitignore b/.gitignore

>> >index 70580bdd352c..72ef86a5570d 100644

>> >--- a/.gitignore

>> >+++ b/.gitignore

>> >@@ -32,7 +32,6 @@

>> > *.lzo

>> > *.mod

>> > *.mod.c

>> >-*.ns_deps

>> > *.o

>> > *.o.*

>> > *.patch

>> >@@ -61,6 +60,7 @@ modules.order

>> > /System.map

>> > /Module.markers

>> > /modules.builtin.modinfo

>> >+/modules.nsdeps

>> >

>> > #

>> > # RPM spec file (make rpm-pkg)

>> >diff --git a/Documentation/dontdiff b/Documentation/dontdiff

>> >index 9f4392876099..72fc2e9e2b63 100644

>> >--- a/Documentation/dontdiff

>> >+++ b/Documentation/dontdiff

>> >@@ -179,6 +179,7 @@ mkutf8data

>> > modpost

>> > modules.builtin

>> > modules.builtin.modinfo

>> >+modules.nsdeps

>> > modules.order

>> > modversions.h*

>> > nconf

>> >diff --git a/Makefile b/Makefile

>> >index 0ef897fd9cfd..1e3f307bd49b 100644

>> >--- a/Makefile

>> >+++ b/Makefile

>> >@@ -1356,7 +1356,7 @@ endif # CONFIG_MODULES

>> >

>> > # Directories & files removed with 'make clean'

>> > CLEAN_DIRS  += include/ksym

>> >-CLEAN_FILES += modules.builtin.modinfo

>> >+CLEAN_FILES += modules.builtin.modinfo modules.nsdeps

>>

>> Hmm, I tried to run `make -C path/to/kernel/src M=$(PWD) clean` for a test

>> external module, but it didn't remove modules.nsdeps for me. I declared a

>> MODULE namespace for testing purposes.

>>

>> $ make

>> make -C /dev/shm/linux M=/tmp/ppyu/test-module

>> make[1]: Entering directory '/dev/shm/linux'

>>   AR      /tmp/ppyu/test-module/built-in.a

>>   CC [M]  /tmp/ppyu/test-module/test1.o

>>   CC [M]  /tmp/ppyu/test-module/test2.o

>>   LD [M]  /tmp/ppyu/test-module/test.o

>>   Building modules, stage 2.

>>   MODPOST 1 modules

>> WARNING: module test uses symbol try_module_get from namespace MODULE, but does not import it.

>>   CC [M]  /tmp/ppyu/test-module/test.mod.o

>>   LD [M]  /tmp/ppyu/test-module/test.ko

>> make[1]: Leaving directory '/dev/shm/linux'

>>

>> Then I make nsdeps:

>>

>> make -C /dev/shm/linux M=/tmp/ppyu/test-module nsdeps

>> make[1]: Entering directory '/dev/shm/linux'

>>   Building modules, stage 2.

>>   MODPOST 1 modules

>> WARNING: module test uses symbol try_module_get from namespace MODULE, but does not import it.

>> Adding namespace MODULE to module /tmp/ppyu/test-module/test.ko.

>> --- /tmp/ppyu/test-module/test1.c

>> +++ /tmp/cocci-output-3696-c1f8b3-test1.c

>> @@ -38,3 +38,4 @@ static void __exit hello_cleanup(void)

>>  module_init(hello_init);

>>  module_exit(hello_cleanup);

>>  MODULE_LICENSE("GPL");

>> +MODULE_IMPORT_NS(MODULE);

>> make[1]: Leaving directory '/dev/shm/linux'

>> $ cat modules.nsdeps

>> /tmp/ppyu/test-module/test.ko: MODULE

>>

>> Looks good so far, then I try make clean:

>>

>> $ make clean

>> make -C /dev/shm/linux M=/tmp/ppyu/test-module clean

>> make[1]: Entering directory '/dev/shm/linux'

>>   CLEAN   /tmp/ppyu/test-module/Module.symvers

>> make[1]: Leaving directory '/dev/shm/linux'

>> $ ls

>> Makefile  modules.nsdeps  test1.c  test2.c

>>

>> But modules.nsdeps is still there.

>>

>

>Good catch!

>

>I forgot to take care of it for external module builds.

>

>The following should work. I will fold it in 3/4.

>

>

>

>

>diff --git a/Makefile b/Makefile

>index 79be70bf2899..6035702803eb 100644

>--- a/Makefile

>+++ b/Makefile

>@@ -1619,7 +1619,7 @@ _emodinst_post: _emodinst_

>        $(call cmd,depmod)

>

> clean-dirs := $(KBUILD_EXTMOD)

>-clean: rm-files := $(KBUILD_EXTMOD)/Module.symvers

>+clean: rm-files := $(KBUILD_EXTMOD)/Module.symvers $(KBUILD_EXTMOD)/modules.nsdeps


Reviewed-by: Matthias Maennich <maennich@google.com>

Tested-by: Matthias Maennich <maennich@google.com>


Thanks for this improvement!

Cheers,
Matthias

>

> PHONY += /

> /:

>

>

>

>-- 

>Best Regards

>Masahiro Yamada
diff mbox series

Patch

diff --git a/.gitignore b/.gitignore
index 70580bdd352c..72ef86a5570d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -32,7 +32,6 @@ 
 *.lzo
 *.mod
 *.mod.c
-*.ns_deps
 *.o
 *.o.*
 *.patch
@@ -61,6 +60,7 @@  modules.order
 /System.map
 /Module.markers
 /modules.builtin.modinfo
+/modules.nsdeps
 
 #
 # RPM spec file (make rpm-pkg)
diff --git a/Documentation/dontdiff b/Documentation/dontdiff
index 9f4392876099..72fc2e9e2b63 100644
--- a/Documentation/dontdiff
+++ b/Documentation/dontdiff
@@ -179,6 +179,7 @@  mkutf8data
 modpost
 modules.builtin
 modules.builtin.modinfo
+modules.nsdeps
 modules.order
 modversions.h*
 nconf
diff --git a/Makefile b/Makefile
index 0ef897fd9cfd..1e3f307bd49b 100644
--- a/Makefile
+++ b/Makefile
@@ -1356,7 +1356,7 @@  endif # CONFIG_MODULES
 
 # Directories & files removed with 'make clean'
 CLEAN_DIRS  += include/ksym
-CLEAN_FILES += modules.builtin.modinfo
+CLEAN_FILES += modules.builtin.modinfo modules.nsdeps
 
 # Directories & files removed with 'make mrproper'
 MRPROPER_DIRS  += include/config include/generated          \
@@ -1660,7 +1660,7 @@  clean: $(clean-dirs)
 		-o -name '*.ko.*' \
 		-o -name '*.dtb' -o -name '*.dtb.S' -o -name '*.dt.yaml' \
 		-o -name '*.dwo' -o -name '*.lst' \
-		-o -name '*.su' -o -name '*.mod' -o -name '*.ns_deps' \
+		-o -name '*.su' -o -name '*.mod' \
 		-o -name '.*.d' -o -name '.*.tmp' -o -name '*.mod.c' \
 		-o -name '*.lex.c' -o -name '*.tab.[ch]' \
 		-o -name '*.asn1.[ch]' \
diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost
index c9757b20b048..da37128c3f9f 100644
--- a/scripts/Makefile.modpost
+++ b/scripts/Makefile.modpost
@@ -66,7 +66,7 @@  __modpost:
 else
 
 MODPOST += $(subst -i,-n,$(filter -i,$(MAKEFLAGS))) -s -T - \
-	$(if $(KBUILD_NSDEPS),-d)
+	$(if $(KBUILD_NSDEPS),-d modules.nsdeps)
 
 ifeq ($(KBUILD_EXTMOD),)
 MODPOST += $(wildcard vmlinux)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index dcd90d563ce8..f7425f5d4ab0 100644
--- a/scripts/mod/modpost.c
+++ b/scripts/mod/modpost.c
@@ -38,8 +38,6 @@  static int sec_mismatch_count = 0;
 static int sec_mismatch_fatal = 0;
 /* ignore missing files */
 static int ignore_missing_files;
-/* write namespace dependencies */
-static int write_namespace_deps;
 
 enum export {
 	export_plain,      export_unused,     export_gpl,
@@ -2217,14 +2215,11 @@  static int check_exports(struct module *mod)
 		else
 			basename = mod->name;
 
-		if (exp->namespace) {
-			add_namespace(&mod->required_namespaces,
-				      exp->namespace);
-
-			if (!module_imports_namespace(mod, exp->namespace)) {
-				warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
-				     basename, exp->name, exp->namespace);
-			}
+		if (exp->namespace &&
+		    !module_imports_namespace(mod, exp->namespace)) {
+			warn("module %s uses symbol %s from namespace %s, but does not import it.\n",
+			     basename, exp->name, exp->namespace);
+			add_namespace(&mod->missing_namespaces, exp->namespace);
 		}
 
 		if (!mod->gpl_compatible)
@@ -2525,29 +2520,27 @@  static void write_dump(const char *fname)
 	free(buf.p);
 }
 
-static void write_namespace_deps_files(void)
+static void write_namespace_deps_files(const char *fname)
 {
 	struct module *mod;
 	struct namespace_list *ns;
 	struct buffer ns_deps_buf = {};
 
 	for (mod = modules; mod; mod = mod->next) {
-		char fname[PATH_MAX];
 
-		if (mod->skip)
+		if (mod->skip || !mod->missing_namespaces)
 			continue;
 
-		ns_deps_buf.pos = 0;
+		buf_printf(&ns_deps_buf, "%s.ko:", mod->name);
 
-		for (ns = mod->required_namespaces; ns; ns = ns->next)
-			buf_printf(&ns_deps_buf, "%s\n", ns->namespace);
+		for (ns = mod->missing_namespaces; ns; ns = ns->next)
+			buf_printf(&ns_deps_buf, " %s", ns->namespace);
 
-		if (ns_deps_buf.pos == 0)
-			continue;
-
-		sprintf(fname, "%s.ns_deps", mod->name);
-		write_if_changed(&ns_deps_buf, fname);
+		buf_printf(&ns_deps_buf, "\n");
 	}
+
+	write_if_changed(&ns_deps_buf, fname);
+	free(ns_deps_buf.p);
 }
 
 struct ext_sym_list {
@@ -2560,6 +2553,7 @@  int main(int argc, char **argv)
 	struct module *mod;
 	struct buffer buf = { };
 	char *kernel_read = NULL;
+	char *missing_namespace_deps = NULL;
 	char *dump_write = NULL, *files_source = NULL;
 	int opt;
 	int err;
@@ -2567,7 +2561,7 @@  int main(int argc, char **argv)
 	struct ext_sym_list *extsym_iter;
 	struct ext_sym_list *extsym_start = NULL;
 
-	while ((opt = getopt(argc, argv, "i:e:mnsT:o:awEd")) != -1) {
+	while ((opt = getopt(argc, argv, "i:e:mnsT:o:awEd:")) != -1) {
 		switch (opt) {
 		case 'i':
 			kernel_read = optarg;
@@ -2606,7 +2600,7 @@  int main(int argc, char **argv)
 			sec_mismatch_fatal = 1;
 			break;
 		case 'd':
-			write_namespace_deps = 1;
+			missing_namespace_deps = optarg;
 			break;
 		default:
 			exit(1);
@@ -2654,8 +2648,8 @@  int main(int argc, char **argv)
 		write_if_changed(&buf, fname);
 	}
 
-	if (write_namespace_deps)
-		write_namespace_deps_files();
+	if (missing_namespace_deps)
+		write_namespace_deps_files(missing_namespace_deps);
 
 	if (dump_write)
 		write_dump(dump_write);
diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
index ad271bc6c313..fe6652535e4b 100644
--- a/scripts/mod/modpost.h
+++ b/scripts/mod/modpost.h
@@ -126,8 +126,8 @@  struct module {
 	struct buffer dev_table_buf;
 	char	     srcversion[25];
 	int is_dot_o;
-	// Required namespace dependencies
-	struct namespace_list *required_namespaces;
+	// Missing namespace dependencies
+	struct namespace_list *missing_namespaces;
 	// Actual imported namespaces
 	struct namespace_list *imported_namespaces;
 };
diff --git a/scripts/nsdeps b/scripts/nsdeps
index dda6fbac016e..08db427a7fe5 100644
--- a/scripts/nsdeps
+++ b/scripts/nsdeps
@@ -27,15 +27,14 @@  generate_deps_for_ns() {
 }
 
 generate_deps() {
-	local mod_name=`basename $@ .ko`
-	local mod_file=`echo $@ | sed -e 's/\.ko/\.mod/'`
-	local ns_deps_file=`echo $@ | sed -e 's/\.ko/\.ns_deps/'`
-	if [ ! -f "$ns_deps_file" ]; then return; fi
-	local mod_source_files=`cat $mod_file | sed -n 1p                      \
+	local mod=${1%.ko:}
+	shift
+	local namespaces="$*"
+	local mod_source_files=`cat $mod.mod | sed -n 1p                      \
 					      | sed -e 's/\.o/\.c/g'           \
 					      | sed "s|[^ ]* *|${srctree}/&|g"`
-	for ns in `cat $ns_deps_file`; do
-		echo "Adding namespace $ns to module $mod_name (if needed)."
+	for ns in $namespaces; do
+		echo "Adding namespace $ns to module $mod.ko."
 		generate_deps_for_ns $ns $mod_source_files
 		# sort the imports
 		for source_file in $mod_source_files; do
@@ -52,7 +51,7 @@  generate_deps() {
 	done
 }
 
-for f in `cat $objtree/modules.order`; do
-	generate_deps $f
-done
-
+while read line
+do
+	generate_deps $line
+done < modules.nsdeps