| Message ID | 20190730181146.6507-1-efremov@linux.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] modpost: check for static EXPORT_SYMBOL* functions | expand |
Changes in v1:
1. Fixed indentations.
2. Removed lkml links from the description of the commit.
Changes in v2:
1. Changed the 'n' variable type in the main function from size_t to
int.
Changes in v3:
1. Changed warning message from "%s is the static EXPORT_*" to
"%s a static ..."
2. Improved the commit description from "approx. 1 min" to
"less than a minute". Thanks Stephen Rothwell for additional
measurements.
Changes in v4:
1. Dropped ELF_ST_TYPE check. This fixes false-positives detected by
Stephen Rothwell.
2. Moved ELF_ST_BIND check before the call to find_symbol. Thanks
Masahiro Yamada for suggesting it.
Denis
On 30.07.2019 21:11, Denis Efremov wrote:
> This patch adds a check to warn about static EXPORT_SYMBOL* functions
> during the modpost. In most of the cases, a static symbol marked for
> exporting is an odd combination that should be fixed either by deleting
> the exporting mark or by removing the static attribute and adding the
> appropriate declaration to headers.
>
> This check could help to detect the following problems:
> 1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too")
> 2. 54638c6eaf44 ("net: phy: make exported variables non-static")
> 3. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
> 4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
> 5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
> 6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next")
> 7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe")
> 8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh")
> 9. ...
>
> Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x):
> $ make mrproper; make allmodconfig; time make -j12; \
> git checkout HEAD~1; \
> make mrproper; make allmodconfig; time make -j12
> 1.
> (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total
> (w/o patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total
> 2.
> (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total
> (w/o patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total
> 3.
> (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total
> (w/o patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total
>
> The check adds less than a minute to a usual build.
>
> Acked-by: Emil Velikov <emil.l.velikov@gmail.com>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
> scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
>
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index f277e116e0eb..3e6d36ddfcdf 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -169,6 +169,7 @@ struct symbol {
> unsigned int kernel:1; /* 1 if symbol is from kernel
> * (only for external modules) **/
> unsigned int preloaded:1; /* 1 if symbol from Module.symvers, or crc */
> + unsigned int is_static:1; /* 1 if symbol is not global */
> enum export export; /* Type of export */
> char name[0];
> };
> @@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak,
> strcpy(s->name, name);
> s->weak = weak;
> s->next = next;
> + s->is_static = 1;
> return s;
> }
>
> @@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
> handle_modversions(mod, &info, sym, symname);
> handle_moddevtable(mod, &info, sym, symname);
> }
> +
> + // check for static EXPORT_SYMBOL_* functions && global vars
> + for (sym = info.symtab_start; sym < info.symtab_stop; sym++) {
> + unsigned char bind = ELF_ST_BIND(sym->st_info);
> +
> + if (bind == STB_GLOBAL || bind == STB_WEAK) {
> + struct symbol *s =
> + find_symbol(remove_dot(info.strtab +
> + sym->st_name));
> +
> + if (s)
> + s->is_static = 0;
> + }
> + }
> +
> if (!is_vmlinux(modname) || vmlinux_section_warnings)
> check_sec_ref(mod, modname, &info);
>
> @@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
> char *dump_write = NULL, *files_source = NULL;
> int opt;
> int err;
> + int n;
> struct ext_sym_list *extsym_iter;
> struct ext_sym_list *extsym_start = NULL;
>
> @@ -2520,6 +2538,19 @@ int main(int argc, char **argv)
> if (sec_mismatch_count && sec_mismatch_fatal)
> fatal("modpost: Section mismatches detected.\n"
> "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n");
> + for (n = 0; n < SYMBOL_HASH_SIZE; n++) {
> + struct symbol *s = symbolhash[n];
> +
> + while (s) {
> + if (s->is_static)
> + warn("\"%s\" [%s] is a static %s\n",
> + s->name, s->module->name,
> + export_str(s->export));
> +
> + s = s->next;
> + }
> + }
> +
> free(buf.p);
>
> return err;
>
Hi. On Wed, Jul 31, 2019 at 3:12 AM Denis Efremov <efremov@linux.com> wrote: > > This patch adds a check to warn about static EXPORT_SYMBOL* functions > during the modpost. In most of the cases, a static symbol marked for > exporting is an odd combination that should be fixed either by deleting > the exporting mark or by removing the static attribute and adding the > appropriate declaration to headers. > > This check could help to detect the following problems: > 1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too") > 2. 54638c6eaf44 ("net: phy: make exported variables non-static") > 3. 98ef2046f28b ("mm: remove the exporting of totalram_pages") > 4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration") > 5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next") > 6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next") > 7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe") > 8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh") > 9. ... > > Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x): > $ make mrproper; make allmodconfig; time make -j12; \ > git checkout HEAD~1; \ > make mrproper; make allmodconfig; time make -j12 > 1. > (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total > (w/o patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total > 2. > (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total > (w/o patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total > 3. > (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total > (w/o patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total > > The check adds less than a minute to a usual build. I am not convinced with this analysis. Since commit b7dca6dd1e591ad19, scripts/mod/modpost is only invoked from scripts/Makefile.modpost. So, it is much easier to to measure the elapsed time of modpost directly. I applied your patch on top next-20190731, and the following: diff --git a/scripts/Makefile.modpost b/scripts/Makefile.modpost index 92ed02d7cd5e..e0db1a626e50 100644 --- a/scripts/Makefile.modpost +++ b/scripts/Makefile.modpost @@ -98,7 +98,7 @@ MODPOST += $(subst -i,-n,$(filter -i,$(MAKEFLAGS))) -s -T - $(wildcard vmlinux) # We can go over command line length here, so be careful. quiet_cmd_modpost = MODPOST $(words $(modules)) modules - cmd_modpost = sed 's/ko$$/o/' $(modorder) | $(MODPOST) + cmd_modpost = sed 's/ko$$/o/' $(modorder) | time -o modpost-time.txt $(MODPOST) PHONY += modules-modpost modules-modpost: The difference is less than 1 sec (not 1 min!) even for allmodconfig. So, the performance regression is almost unnoticeable level.
On Wed, Jul 31, 2019 at 5:54 PM Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Hi. > > > > On Wed, Jul 31, 2019 at 3:12 AM Denis Efremov <efremov@linux.com> wrote: > > > > This patch adds a check to warn about static EXPORT_SYMBOL* functions > > during the modpost. In most of the cases, a static symbol marked for > > exporting is an odd combination that should be fixed either by deleting > > the exporting mark or by removing the static attribute and adding the > > appropriate declaration to headers. > > > > This check could help to detect the following problems: > > 1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too") > > 2. 54638c6eaf44 ("net: phy: make exported variables non-static") > > 3. 98ef2046f28b ("mm: remove the exporting of totalram_pages") > > 4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration") > > 5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next") > > 6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next") > > 7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe") > > 8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh") > > 9. ... > > > > Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x): > > $ make mrproper; make allmodconfig; time make -j12; \ > > git checkout HEAD~1; \ > > make mrproper; make allmodconfig; time make -j12 > > 1. > > (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total > > (w/o patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total > > 2. > > (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total > > (w/o patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total > > 3. > > (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total > > (w/o patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total > > > > The check adds less than a minute to a usual build. The build time impact is very limited. I guess this measurement is unnecessary in the commit log.
On 01.08.2019 05:20, Masahiro Yamada wrote: > > The build time impact is very limited. > I guess this measurement is unnecessary in the commit log. > Thank you for your observations! It was not easy for me to guess to do it this way because of my limited knowledge about kbuild && modpost work. In v5 I've only updated commit log: measurements removed, "The build time impact is very limited and is almost at the unnoticeable level (< 1 sec)" added instead. Fell free to edit it if you want. Denis
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c index f277e116e0eb..3e6d36ddfcdf 100644 --- a/scripts/mod/modpost.c +++ b/scripts/mod/modpost.c @@ -169,6 +169,7 @@ struct symbol { unsigned int kernel:1; /* 1 if symbol is from kernel * (only for external modules) **/ unsigned int preloaded:1; /* 1 if symbol from Module.symvers, or crc */ + unsigned int is_static:1; /* 1 if symbol is not global */ enum export export; /* Type of export */ char name[0]; }; @@ -201,6 +202,7 @@ static struct symbol *alloc_symbol(const char *name, unsigned int weak, strcpy(s->name, name); s->weak = weak; s->next = next; + s->is_static = 1; return s; } @@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname) handle_modversions(mod, &info, sym, symname); handle_moddevtable(mod, &info, sym, symname); } + + // check for static EXPORT_SYMBOL_* functions && global vars + for (sym = info.symtab_start; sym < info.symtab_stop; sym++) { + unsigned char bind = ELF_ST_BIND(sym->st_info); + + if (bind == STB_GLOBAL || bind == STB_WEAK) { + struct symbol *s = + find_symbol(remove_dot(info.strtab + + sym->st_name)); + + if (s) + s->is_static = 0; + } + } + if (!is_vmlinux(modname) || vmlinux_section_warnings) check_sec_ref(mod, modname, &info); @@ -2425,6 +2442,7 @@ int main(int argc, char **argv) char *dump_write = NULL, *files_source = NULL; int opt; int err; + int n; struct ext_sym_list *extsym_iter; struct ext_sym_list *extsym_start = NULL; @@ -2520,6 +2538,19 @@ int main(int argc, char **argv) if (sec_mismatch_count && sec_mismatch_fatal) fatal("modpost: Section mismatches detected.\n" "Set CONFIG_SECTION_MISMATCH_WARN_ONLY=y to allow them.\n"); + for (n = 0; n < SYMBOL_HASH_SIZE; n++) { + struct symbol *s = symbolhash[n]; + + while (s) { + if (s->is_static) + warn("\"%s\" [%s] is a static %s\n", + s->name, s->module->name, + export_str(s->export)); + + s = s->next; + } + } + free(buf.p); return err;
This patch adds a check to warn about static EXPORT_SYMBOL* functions during the modpost. In most of the cases, a static symbol marked for exporting is an odd combination that should be fixed either by deleting the exporting mark or by removing the static attribute and adding the appropriate declaration to headers. This check could help to detect the following problems: 1. 550113d4e9f5 ("i2c: add newly exported functions to the header, too") 2. 54638c6eaf44 ("net: phy: make exported variables non-static") 3. 98ef2046f28b ("mm: remove the exporting of totalram_pages") 4. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration") 5. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next") 6. e4e4730698c9 ("crypto: skcipher - remove the exporting of skcipher_walk_next") 7. 14b4c48bb1ce ("gve: Remove the exporting of gve_probe") 8. 9b79ee9773a8 ("scsi: libsas: remove the exporting of sas_wait_eh") 9. ... Build time impact, allmodconfig, Dell XPS 15 9570 (measurements 3x): $ make mrproper; make allmodconfig; time make -j12; \ git checkout HEAD~1; \ make mrproper; make allmodconfig; time make -j12 1. (with patch) 17635,94s user 1895,54s system 1085% cpu 29:59,22 total (w/o patch) 17275,42s user 1803,87s system 1112% cpu 28:35,66 total 2. (with patch) 17369,51s user 1763,28s system 1111% cpu 28:41,47 total (w/o patch) 16880,50s user 1670,93s system 1113% cpu 27:46,56 total 3. (with patch) 17937,88s user 1842,53s system 1109% cpu 29:42,26 total (w/o patch) 17267,55s user 1725,09s system 1111% cpu 28:28,17 total The check adds less than a minute to a usual build. Acked-by: Emil Velikov <emil.l.velikov@gmail.com> Signed-off-by: Denis Efremov <efremov@linux.com> --- scripts/mod/modpost.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)