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

From: Matthias Maennich <maennich@google.com>
To: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Jessica Yu <jeyu@kernel.org>,
	Linux Kbuild mailing list <linux-kbuild@vger.kernel.org>,
	Matthias Kaehlcke <mka@chromium.org>,
	Jonathan Corbet <corbet@lwn.net>,
	Michal Marek <michal.lkml@markovi.net>,
	"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/4] modpost: dump missing namespaces into a single modules.nsdeps file
Date: Wed, 6 Nov 2019 15:24:54 +0000	[thread overview]
Message-ID: <20191106152454.GA1243@google.com> (raw)
In-Reply-To: <CAK7LNASCmSUuqLyJhZW+3yrGk1KTPxA-_0x86N=Y7A5psCVUSg@mail.gmail.com>

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