Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi Denis, On Mon, 29 Jul 2019 15:52:15 +0300 Denis Efremov wrote: > > Is it worth to include your measurements instead of mine in the commit > description? Maybe the note about performance downgrade could be omitted > at all in this case? Just leave your measurements (they are yours after all), but maybe say "less than a minute" rather than "approx. 1 min". -- Cheers, Stephen Rothwell pgpxzDZx_q0UG.pgp Description: OpenPGP digital signature
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
On 7/29/19 3:40 PM, Stephen Rothwell wrote: > Hi Denis, > > On Mon, 29 Jul 2019 12:16:29 +0300 Denis Efremov wrote: >> >>> Just a reminder that some of us (just me?) do well over 100+ builds per >>> day ... if this can be optimised some what that would be good. >> >> These measurements for the worst case (allmodconfig). Is it possible to >> measure the slowdown in your case? How it will perform on your typical >> workflow? > > I did 3 x86_64 allmodconfig builds without and with the patch (I do > -j 80 powerpc64 le hosted cross builds) and it doesn't look like the > patch has much impact at all. > > Without the patch: > > real 8m41.390s user 587m25.249s sys 22m0.411s > real 8m40.100s user 587m32.148s sys 21m58.419s > real 8m40.084s user 587m25.311s sys 22m2.794s > > With the patch: > > real 8m40.351s user 587m21.819s sys 21m57.389s > real 8m40.868s user 587m23.730s sys 21m58.737s > real 8m40.970s user 587m22.525s sys 22m2.467s > > I do other builds as well, but that is the biggest, so actually looks > ok. > Is it worth to include your measurements instead of mine in the commit description? Maybe the note about performance downgrade could be omitted at all in this case? Denis
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi Denis, On Mon, 29 Jul 2019 12:16:29 +0300 Denis Efremov wrote: > > > Just a reminder that some of us (just me?) do well over 100+ builds per > > day ... if this can be optimised some what that would be good. > > These measurements for the worst case (allmodconfig). Is it possible to > measure the slowdown in your case? How it will perform on your typical > workflow? I did 3 x86_64 allmodconfig builds without and with the patch (I do -j 80 powerpc64 le hosted cross builds) and it doesn't look like the patch has much impact at all. Without the patch: real8m41.390s user 587m25.249s sys 22m0.411s real8m40.100s user 587m32.148s sys 21m58.419s real8m40.084s user 587m25.311s sys 22m2.794s With the patch: real8m40.351s user 587m21.819s sys 21m57.389s real8m40.868s user 587m23.730s sys 21m58.737s real8m40.970s user 587m22.525s sys 22m2.467s I do other builds as well, but that is the biggest, so actually looks ok. -- Cheers, Stephen Rothwell pgpCfnjlQqEbO.pgp Description: OpenPGP digital signature
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
On 29.07.2019 06:29, Masahiro Yamada wrote: may I ask you to use 'int' instead of 'size_t' here? Fixed in v2. Regards, Denis
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
On Mon, Jul 29, 2019 at 6:16 PM Denis Efremov wrote: > > > Just a reminder that some of us (just me?) do well over 100+ builds per > > day ... if this can be optimised some what that would be good. > > These measurements for the worst case (allmodconfig). Is it possible to > measure the slowdown in your case? How it will perform on your typical > workflow? > > Looks like it is possible to optimize it, but I need some hints from > Masahiro on how to do it properly. Because I don't know how to match > __ksymtab_ with the without an additional loop. Right. This is not feasible without an additional loop since we put only exported symbols into the hash table. Perhaps, we could put every symbol into the hash table so that we can quickly look-up from __ksymtab_, but it would consume lots of memory. So, I think the implementation is this patch is good enough. > Introduce another hash table? > > The first loop from this patch could traverse only the exported symbols > instead of all symbols. But in this case, I don't know how to break > early from the loop because there can be many symbols with the same name > but with the different scope (static/non-static). > > For example, ring_buffer_size: > kernel/trace/ring_buffer.c > 4334:unsigned long ring_buffer_size(struct ring_buffer *buffer, int cpu) > 4347:EXPORT_SYMBOL_GPL(ring_buffer_size); > And > drivers/usb/misc/ldusb.c > 125:static int ring_buffer_size = 128; > > Or for, nfs4_disable_idmapping: > fs/nfs/super.c > 2920:bool nfs4_disable_idmapping = true; > 2930:EXPORT_SYMBOL_GPL(nfs4_disable_idmapping); > fs/nfsd/nfs4idmap.c > 48:static bool nfs4_disable_idmapping = true; -- Best Regards Masahiro Yamada
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good. These measurements for the worst case (allmodconfig). Is it possible to measure the slowdown in your case? How it will perform on your typical workflow? Looks like it is possible to optimize it, but I need some hints from Masahiro on how to do it properly. Because I don't know how to match __ksymtab_ with the without an additional loop. Introduce another hash table? The first loop from this patch could traverse only the exported symbols instead of all symbols. But in this case, I don't know how to break early from the loop because there can be many symbols with the same name but with the different scope (static/non-static). For example, ring_buffer_size: kernel/trace/ring_buffer.c 4334:unsigned long ring_buffer_size(struct ring_buffer *buffer, int cpu) 4347:EXPORT_SYMBOL_GPL(ring_buffer_size); And drivers/usb/misc/ldusb.c 125:static int ring_buffer_size = 128; Or for, nfs4_disable_idmapping: fs/nfs/super.c 2920:bool nfs4_disable_idmapping = true; 2930:EXPORT_SYMBOL_GPL(nfs4_disable_idmapping); fs/nfsd/nfs4idmap.c 48:static bool nfs4_disable_idmapping = true; Regards, Denis
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi Denis, On Sun, 28 Jul 2019 13:09:06 +0300 Denis Efremov wrote: > > Thus, the current implementation adds approx. 1 min for allmodconfig. Just a reminder that some of us (just me?) do well over 100+ builds per day ... if this can be optimised some what that would be good. -- Cheers, Stephen Rothwell pgpGYpN0Yq7wP.pgp Description: OpenPGP digital signature
Re: [PATCH] modpost: check for static EXPORT_SYMBOL* functions
On Sun, Jul 28, 2019 at 7:09 PM 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 % 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 % cpu 28:28,17 total
>
> Thus, the current implementation adds approx. 1 min for allmodconfig.
>
> Acked-by: Emil Velikov
> Signed-off-by: Denis Efremov
> ---
> scripts/mod/modpost.c | 32
> 1 file changed, 32 insertions(+)
>
>
> @@ -2425,6 +2443,7 @@ int main(int argc, char **argv)
> char *dump_write = NULL, *files_source = NULL;
> int opt;
> int err;
> + size_t n;
Sorry, I missed to ask this in the previous version.
If there is not a particular reason,
may I ask you to use 'int' instead of 'size_t' here?
SYMBOL_HASH_SIZE (= 1024) is small enough, and
it will keep consistency with
the write_dump() function in this file.
If it is tedious to send a new version,
may I fix-up 'size_t' -> 'int' ?
Thanks.
> struct ext_sym_list *extsym_iter;
> struct ext_sym_list *extsym_start = NULL;
>
> @@ -2520,6 +2539,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 the static %s\n",
> +s->name, s->module->name,
> +export_str(s->export));
> +
> + s = s->next;
> + }
> + }
> +
> free(buf.p);
>
> return err;
> --
> 2.21.0
>
--
Best Regards
Masahiro Yamada
[PATCH] modpost: check for static EXPORT_SYMBOL* functions
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 % 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 % cpu 28:28,17 total
Thus, the current implementation adds approx. 1 min for allmodconfig.
Acked-by: Emil Velikov
Signed-off-by: Denis Efremov
---
scripts/mod/modpost.c | 32
1 file changed, 32 insertions(+)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..85e885235c96 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,22 @@ static void read_symbols(const char *modname)
handle_modversions(mod, , sym, symname);
handle_moddevtable(mod, , 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);
+ unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+ if (type == STT_OBJECT || type == STT_FUNC) {
+ struct symbol *s =
+ find_symbol(remove_dot(info.strtab +
+ sym->st_name));
+
+ if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+ s->is_static = 0;
+ }
+ }
+
if (!is_vmlinux(modname) || vmlinux_section_warnings)
check_sec_ref(mod, modname, );
@@ -2425,6 +2443,7 @@ int main(int argc, char **argv)
char *dump_write = NULL, *files_source = NULL;
int opt;
int err;
+ size_t n;
struct ext_sym_list *extsym_iter;
struct ext_sym_list *extsym_start = NULL;
@@ -2520,6 +2539,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 the static %s\n",
+s->name, s->module->name,
+export_str(s->export));
+
+ s = s->next;
+ }
+ }
+
free(buf.p);
return err;
--
2.21.0
Re: [RFC PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi. On Sun, Jul 28, 2019 at 4:14 AM Denis Efremov wrote: > > Hi. > > > Could you drop the solved ones from the list? > > Yes, of course. Do you want me to remove all symbols fixed with patches > or only those are in-tree now? > > Should it be like this: > 1. "torture_onoff_cleanup" [kernel/torture] > "torture_shuffle_cleanup" [kernel/torture] > Patch: https://lkml.org/lkml/2019/7/4/411 (accepted) > 2. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress] > Patch: https://lkml.org/lkml/2019/7/8/842 > 3. "drm_client_close" [drivers/gpu/drm/drm] > Patch: https://lkml.org/lkml/2019/7/3/758 (accepted) > 4. "ahci_em_messages" [drivers/ata/libahci] > Patch: https://lkml.org/lkml/2019/7/10/550 (reviewed) > 5. "ftrace_set_clr_event" [vmlinux] > Patch: https://lkml.org/lkml/2019/7/4/609 (reviewed) > 6. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core] > Patch: https://lkml.org/lkml/2019/7/8/999 (accepted) > 10. "empty_zero_page" [vmlinux] > 11. "phys_base" [vmlinux] > 12. "hypercall_page" [vmlinux] > > Or like this: > 1. "empty_zero_page" [vmlinux] > 2. "phys_base" [vmlinux] > 3. "hypercall_page" [vmlinux] > > Well, I could remove this list at all. It seems like the following list > with existing commits is enough to show the problem of static exported > functions. Yeah, I agree. Showing some existing commits is enough. > I will resend the patch shortly after. > > Regards, > Denis -- Best Regards Masahiro Yamada
Re: [RFC PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi. Could you drop the solved ones from the list? Yes, of course. Do you want me to remove all symbols fixed with patches or only those are in-tree now? Should it be like this: 1. "torture_onoff_cleanup" [kernel/torture] "torture_shuffle_cleanup" [kernel/torture] Patch: https://lkml.org/lkml/2019/7/4/411 (accepted) 2. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress] Patch: https://lkml.org/lkml/2019/7/8/842 3. "drm_client_close" [drivers/gpu/drm/drm] Patch: https://lkml.org/lkml/2019/7/3/758 (accepted) 4. "ahci_em_messages" [drivers/ata/libahci] Patch: https://lkml.org/lkml/2019/7/10/550 (reviewed) 5. "ftrace_set_clr_event" [vmlinux] Patch: https://lkml.org/lkml/2019/7/4/609 (reviewed) 6. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core] Patch: https://lkml.org/lkml/2019/7/8/999 (accepted) 10. "empty_zero_page" [vmlinux] 11. "phys_base" [vmlinux] 12. "hypercall_page" [vmlinux] Or like this: 1. "empty_zero_page" [vmlinux] 2. "phys_base" [vmlinux] 3. "hypercall_page" [vmlinux] Well, I could remove this list at all. It seems like the following list with existing commits is enough to show the problem of static exported functions. I will resend the patch shortly after. Regards, Denis
Re: [RFC PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi.
Thanks, this patch is very nice.
On Mon, Jul 15, 2019 at 12:28 AM 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.
>
> If this check will be considered useful, I will resend the patch with
> review fixes.
>
> Currently, this check emits the warnings on the following symbols, most
> of which are accepted to be fixed:
> 1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
>Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
> 2. "torture_onoff_cleanup" [kernel/torture]
>"torture_shuffle_cleanup" [kernel/torture]
>Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
> 3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
>Patch: https://lkml.org/lkml/2019/7/8/842
> 4. "drm_client_close" [drivers/gpu/drm/drm]
>Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
> 5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
>Patch: https://lkml.org/lkml/2019/7/14/65
> 6. "i2c_new_client_device" [vmlinux]
>"i2c_new_dummy_device" [vmlinux]
>Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
> 7. "ahci_em_messages" [drivers/ata/libahci]
>Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
> 8. "ftrace_set_clr_event" [vmlinux]
>Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
> 9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
>Patch: https://lkml.org/lkml/2019/7/8/999
> 10. "empty_zero_page" [vmlinux]
> 11. "phys_base" [vmlinux]
> 12. "hypercall_page" [vmlinux]
Could you drop the solved ones from the list?
I still see the following at least, but I will apply this patch anyway.
WARNING: "phys_base" [vmlinux] is a static EXPORT_SYMBOL symbol
WARNING: "drm_client_close" [vmlinux] is a static EXPORT_SYMBOL symbol
WARNING: "ahci_em_messages" [vmlinux] is a static EXPORT_SYMBOL_GPL symbol
WARNING: "ftrace_set_clr_event" [vmlinux] is a static EXPORT_SYMBOL_GPL symbol
WARNING: "empty_zero_page" [vmlinux] is a static EXPORT_SYMBOL symbol
The code looks good to me.
I commented on some minor style issues.
> Similar commits:
> 1. 54638c6eaf44 ("net: phy: make exported variables non-static")
> 2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
> 3. 73df167c819e ("s390/zcrypt: remove the exporting of
> ap_query_configuration")
> 4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
> 5. e4e4730698c9 ("crypto: skcipher - remove the exporting of
> skcipher_walk_next")
>
> 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 % 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 % cpu 28:28,17 total
>
> Thus, the current implementation adds approx. 1 min for allmodconfig.
> However, it's possible to do the check in a more optimal way if it will
> be considered useful.
>
> Also, this kind of check could be implemented as a separate script instead.
> Here is the implementation:
> https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2
>
> Signed-off-by: Denis Efremov
> @@ -199,8 +200,9 @@ static struct symbol *alloc_symbol(const char *name,
> unsigned int weak,
>
> memset(s, 0, sizeof(*s));
> strcpy(s->name, name);
> - s->weak = weak;
> - s->next = next;
> + s->weak = weak;
> + s->next = next;
Could you drop this change?
I do not think we need to align the '=' operator.
> + s->is_static = 1;
> return s;
> }
>
> @@ -1980,6 +1982,21 @@ static void read_symbols(const char *modname)
> handle_modversions(mod, , sym, symname);
> handle_moddevtable(mod, , 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);
> + unsigned char type = ELF_ST_TYPE(sym->st_info);
> +
> + if (type == STT_OBJECT || type == STT_FUNC) {
> + struct symbol *s =
> + find_symbol(remove_dot(info.strtab +
> sym->st_name));
> +
> + if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
> + s->is_static = 0;
> +
Re: [RFC PATCH] modpost: check for static EXPORT_SYMBOL* functions
Hi Denis,
On 2019/07/14, 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.
>
> If this check will be considered useful, I will resend the patch with
> review fixes.
>
> Currently, this check emits the warnings on the following symbols, most
> of which are accepted to be fixed:
> 1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
>Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
> 2. "torture_onoff_cleanup" [kernel/torture]
>"torture_shuffle_cleanup" [kernel/torture]
>Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
> 3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
>Patch: https://lkml.org/lkml/2019/7/8/842
> 4. "drm_client_close" [drivers/gpu/drm/drm]
>Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
> 5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
>Patch: https://lkml.org/lkml/2019/7/14/65
> 6. "i2c_new_client_device" [vmlinux]
>"i2c_new_dummy_device" [vmlinux]
>Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
> 7. "ahci_em_messages" [drivers/ata/libahci]
>Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
> 8. "ftrace_set_clr_event" [vmlinux]
>Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
> 9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
>Patch: https://lkml.org/lkml/2019/7/8/999
> 10. "empty_zero_page" [vmlinux]
> 11. "phys_base" [vmlinux]
> 12. "hypercall_page" [vmlinux]
>
> Similar commits:
> 1. 54638c6eaf44 ("net: phy: make exported variables non-static")
> 2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
> 3. 73df167c819e ("s390/zcrypt: remove the exporting of
> ap_query_configuration")
> 4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
> 5. e4e4730698c9 ("crypto: skcipher - remove the exporting of
> skcipher_walk_next")
>
> 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 % 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 % cpu 28:28,17 total
>
> Thus, the current implementation adds approx. 1 min for allmodconfig.
> However, it's possible to do the check in a more optimal way if it will
> be considered useful.
>
> Also, this kind of check could be implemented as a separate script instead.
> Here is the implementation:
> https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2
>
Personally I think this is a pretty good feature.
If I did my numbers correctly, the above numbers show ~2% increase.
Although one should be able to reduce that if people feel too strongly.
That said, the patch is:
Acked-by: Emil Velikov
Can we make sure that patches for all issues are out (on the respective
mailing lists, or merged) before this lands.
HTH
Emil
[RFC PATCH] modpost: check for static EXPORT_SYMBOL* functions
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.
If this check will be considered useful, I will resend the patch with
review fixes.
Currently, this check emits the warnings on the following symbols, most
of which are accepted to be fixed:
1. "sas_wait_eh" [drivers/scsi/libsas/libsas]
Patch: https://lkml.org/lkml/2019/7/8/970 (accepted)
2. "torture_onoff_cleanup" [kernel/torture]
"torture_shuffle_cleanup" [kernel/torture]
Patch: https://lkml.org/lkml/2019/7/4/411 (accepted)
3. "LZ4HC_setExternalDict" [lib/lz4/lz4hc_compress]
Patch: https://lkml.org/lkml/2019/7/8/842
4. "drm_client_close" [drivers/gpu/drm/drm]
Patch: https://lkml.org/lkml/2019/7/3/758 (accepted)
5. "gve_probe" [drivers/net/ethernet/google/gve/gve]
Patch: https://lkml.org/lkml/2019/7/14/65
6. "i2c_new_client_device" [vmlinux]
"i2c_new_dummy_device" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/7/226 (fixed in a different patch)
7. "ahci_em_messages" [drivers/ata/libahci]
Patch: https://lkml.org/lkml/2019/7/10/550 (reviwed)
8. "ftrace_set_clr_event" [vmlinux]
Patch: https://lkml.org/lkml/2019/7/4/609 (reviwed)
9. "rmi_2d_sensor_set_input_params" [drivers/input/rmi4/rmi_core]
Patch: https://lkml.org/lkml/2019/7/8/999
10. "empty_zero_page" [vmlinux]
11. "phys_base" [vmlinux]
12. "hypercall_page" [vmlinux]
Similar commits:
1. 54638c6eaf44 ("net: phy: make exported variables non-static")
2. 98ef2046f28b ("mm: remove the exporting of totalram_pages")
3. 73df167c819e ("s390/zcrypt: remove the exporting of ap_query_configuration")
4. a57caf8c527f ("sunrpc/cache: remove the exporting of cache_seq_next")
5. e4e4730698c9 ("crypto: skcipher - remove the exporting of
skcipher_walk_next")
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 % 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 % cpu 28:28,17 total
Thus, the current implementation adds approx. 1 min for allmodconfig.
However, it's possible to do the check in a more optimal way if it will
be considered useful.
Also, this kind of check could be implemented as a separate script instead.
Here is the implementation:
https://gist.github.com/evdenis/bf2322d094f0c02c0f60fe0a225848b2
Signed-off-by: Denis Efremov
---
scripts/mod/modpost.c | 35 +--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
index f277e116e0eb..c51eef357721 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];
};
@@ -199,8 +200,9 @@ static struct symbol *alloc_symbol(const char *name,
unsigned int weak,
memset(s, 0, sizeof(*s));
strcpy(s->name, name);
- s->weak = weak;
- s->next = next;
+ 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, , sym, symname);
handle_moddevtable(mod, , 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);
+ unsigned char type = ELF_ST_TYPE(sym->st_info);
+
+ if (type == STT_OBJECT || type == STT_FUNC) {
+ struct symbol *s =
+ find_symbol(remove_dot(info.strtab + sym->st_name));
+
+ if (s && (bind == STB_GLOBAL || bind == STB_WEAK))
+ s->is_static = 0;
+ }
+ }
+
if (!is_vmlinux(modname) || vmlinux_section_warnings)
check_sec_ref(mod, modname, );
@@ -2425,6 +2442,7 @@ int main(int argc, char **argv)
char *dump_write = NULL, *files_source = NULL;
