| Message ID | 20210202070218.856847-1-masahiroy@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] printk: use CONFIG_CONSOLE_LOGLEVEL_* directly | expand |
On 2021-02-02, Masahiro Yamada <masahiroy@kernel.org> wrote: > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > used in <linux/printk.h>, which is included from most of source files. > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > arch/x86/platform/uv/uv_nmi.c > drivers/firmware/efi/libstub/efi-stub-helper.c > drivers/tty/sysrq.c > kernel/printk/printk.c > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > kernel, it is enough to recompile those 4 files. > > Remove the CONSOLE_LOGLEVEL_DEFAULT definition from <linux/printk.h>, > and use CONFIG_CONSOLE_LOGLEVEL_DEFAULT directly. With commit a8fe19ebfbfd ("kernel/printk: use symbolic defines for console loglevels") it can be seen that various drivers used to hard-code their own values. The introduction of the macros in an intuitive location (include/linux/printk.h) made it easier for authors to find/use the various available printk settings and thresholds. Technically there is no problem using Kconfig macros directly. But will authors bother to hunt down available Kconfig settings? Or will they only look in printk.h to see what is available? IMHO if code wants to use settings from a foreign subsystem, it should be taking those from headers of that subsystem, rather than using some Kconfig settings from that subsystem. Headers exist to make information available to external code. Kconfig (particularly for a subsystem) exist to configure that subsystem. But my feeling on this may be misguided. Is it generally accepted in the kernel that any code can use Kconfig settings of any other code? John Ogness
On (21/02/02 16:02), Masahiro Yamada wrote: > > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > used in <linux/printk.h>, which is included from most of source files. > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > arch/x86/platform/uv/uv_nmi.c > drivers/firmware/efi/libstub/efi-stub-helper.c > drivers/tty/sysrq.c > kernel/printk/printk.c > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > kernel, it is enough to recompile those 4 files. Do you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT so often that it becomes a problem? -ss
On Tue, Feb 2, 2021 at 7:09 PM Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote: > > On (21/02/02 16:02), Masahiro Yamada wrote: > > > > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > > used in <linux/printk.h>, which is included from most of source files. > > > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > > > arch/x86/platform/uv/uv_nmi.c > > drivers/firmware/efi/libstub/efi-stub-helper.c > > drivers/tty/sysrq.c > > kernel/printk/printk.c > > > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > > kernel, it is enough to recompile those 4 files. > > Do you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT so often that it becomes a > problem? > > -ss <linux/printk.h> is one of most included headers, so it is worth downsizing. CONSOLE_LOGLEVEL_DEFAULT is not such a parameter that printk() users need to know. Changing CONFIG_CONSOLE_LOGLEVEL_DEFAULT results in the rebuilds of the entire tree, which is a flag of bad code structure. So, this is not only CONSOLE_LOGLEVEL_DEFAULT. <linux/printk.h> contains parameters and func declarations that printk() users do not need to know. Examples: CONSOLE_LOGLEVEL_DEFAULT log_buf_addr_get() log_buf_len_get() oops_in_progress ... They are only needed for those who want to more closely get access to the printk internals. Ideally, such parameters and func declarations can go to the subsystems' local header (kernel/printk/internal.h) but when it is not possible, they can be separated out to a different header. I can see a similar idea in the consumer/provider model in several subsystems. Consumers and providers are often orthogonal, and de-coupling them clarifies who needs what. See other subsystems, for example, <linux/clk.h> - clk consumer <linux/clk-provider.h> - clk provider -- Best Regards Masahiro Yamada
On Tue 2021-02-02 09:44:22, John Ogness wrote: > On 2021-02-02, Masahiro Yamada <masahiroy@kernel.org> wrote: > > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > > used in <linux/printk.h>, which is included from most of source files. > > > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > > > arch/x86/platform/uv/uv_nmi.c > > drivers/firmware/efi/libstub/efi-stub-helper.c > > drivers/tty/sysrq.c > > kernel/printk/printk.c > > > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > > kernel, it is enough to recompile those 4 files. > > > > Remove the CONSOLE_LOGLEVEL_DEFAULT definition from <linux/printk.h>, > > and use CONFIG_CONSOLE_LOGLEVEL_DEFAULT directly. > > With commit a8fe19ebfbfd ("kernel/printk: use symbolic defines for > console loglevels") it can be seen that various drivers used to > hard-code their own values. The introduction of the macros in an > intuitive location (include/linux/printk.h) made it easier for authors > to find/use the various available printk settings and thresholds. > > Technically there is no problem using Kconfig macros directly. But will > authors bother to hunt down available Kconfig settings? Or will they > only look in printk.h to see what is available? > > IMHO if code wants to use settings from a foreign subsystem, it should > be taking those from headers of that subsystem, rather than using some > Kconfig settings from that subsystem. Headers exist to make information > available to external code. Kconfig (particularly for a subsystem) exist > to configure that subsystem. I agree with this this view. What about using default_console_loglevel() in the external code? It reads the value from an array. This value is initialized to CONSOLE_LOGLEVEL_DEFAULT and never modified later. Best Regards, Petr
On Thu, Feb 4, 2021 at 12:23 AM Petr Mladek <pmladek@suse.com> wrote: > > On Tue 2021-02-02 09:44:22, John Ogness wrote: > > On 2021-02-02, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > > > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > > > > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > > > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > > > used in <linux/printk.h>, which is included from most of source files. > > > > > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > > > > > arch/x86/platform/uv/uv_nmi.c > > > drivers/firmware/efi/libstub/efi-stub-helper.c > > > drivers/tty/sysrq.c > > > kernel/printk/printk.c > > > > > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > > > kernel, it is enough to recompile those 4 files. > > > > > > Remove the CONSOLE_LOGLEVEL_DEFAULT definition from <linux/printk.h>, > > > and use CONFIG_CONSOLE_LOGLEVEL_DEFAULT directly. > > > > With commit a8fe19ebfbfd ("kernel/printk: use symbolic defines for > > console loglevels") it can be seen that various drivers used to > > hard-code their own values. The introduction of the macros in an > > intuitive location (include/linux/printk.h) made it easier for authors > > to find/use the various available printk settings and thresholds. > > > > Technically there is no problem using Kconfig macros directly. But will > > authors bother to hunt down available Kconfig settings? Or will they > > only look in printk.h to see what is available? > > > > IMHO if code wants to use settings from a foreign subsystem, it should > > be taking those from headers of that subsystem, rather than using some > > Kconfig settings from that subsystem. Headers exist to make information > > available to external code. Kconfig (particularly for a subsystem) exist > > to configure that subsystem. > > I agree with this this view. I have never seen a policy to restrict the use of CONFIG options in relevant subsystem headers. > What about using default_console_loglevel() in the external code? > It reads the value from an array. This value is initialized to > CONSOLE_LOGLEVEL_DEFAULT and never modified later. I do not think default_console_loglevel() is a perfect constant because it can be modified via /proc/sys/kernel/printk I am not sure if it works either. Some code may not be linked to vmlinux. drivers/firmware/efi/libstub/efi-stub-helper.c > Best Regards, > Petr -- Best Regards Masahiro Yamada
On Thu 2021-02-04 06:51:09, Masahiro Yamada wrote: > On Thu, Feb 4, 2021 at 12:23 AM Petr Mladek <pmladek@suse.com> wrote: > > > > On Tue 2021-02-02 09:44:22, John Ogness wrote: > > > On 2021-02-02, Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of > > > > CONFIG_CONSOLE_LOGLEVEL_DEFAULT. > > > > > > > > When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost > > > > all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is > > > > used in <linux/printk.h>, which is included from most of source files. > > > > > > > > In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: > > > > > > > > arch/x86/platform/uv/uv_nmi.c > > > > drivers/firmware/efi/libstub/efi-stub-helper.c > > > > drivers/tty/sysrq.c > > > > kernel/printk/printk.c > > > > > > > > So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the > > > > kernel, it is enough to recompile those 4 files. > > > > > > > > Remove the CONSOLE_LOGLEVEL_DEFAULT definition from <linux/printk.h>, > > > > and use CONFIG_CONSOLE_LOGLEVEL_DEFAULT directly. > > > > > > With commit a8fe19ebfbfd ("kernel/printk: use symbolic defines for > > > console loglevels") it can be seen that various drivers used to > > > hard-code their own values. The introduction of the macros in an > > > intuitive location (include/linux/printk.h) made it easier for authors > > > to find/use the various available printk settings and thresholds. > > > > > > Technically there is no problem using Kconfig macros directly. But will > > > authors bother to hunt down available Kconfig settings? Or will they > > > only look in printk.h to see what is available? > > > > > > IMHO if code wants to use settings from a foreign subsystem, it should > > > be taking those from headers of that subsystem, rather than using some > > > Kconfig settings from that subsystem. Headers exist to make information > > > available to external code. Kconfig (particularly for a subsystem) exist > > > to configure that subsystem. > > > > I agree with this this view. > > > I have never seen a policy to restrict > the use of CONFIG options in relevant > subsystem headers. I would say that it is a common sense. But I admit that I did not look at the code in detail. See below. > > What about using default_console_loglevel() in the external code? > > It reads the value from an array. This value is initialized to > > CONSOLE_LOGLEVEL_DEFAULT and never modified later. > > I do not think default_console_loglevel() > is a perfect constant > because it can be modified via > /proc/sys/kernel/printk And that is the problem. I somehow expected that the external code wanted to have the currently valid value and not the prebuilt one. When I look closely: + arch/x86/platform/uv/uv_nmi.c + drivers/firmware/efi/libstub/efi-stub-helper.c These use the value to statically initialize global variables that might later be modified by subsystem-specific kernel parameters. CONFIG_CONSOLE_LOGLEVEL_DEFAULT is acceptable here from my POV. The build dependency sucks. And it is not worth any too complicated solution. + drivers/tty/sysrq.c The intention here is to use the highest console loglevel so that people really see them. It used to be hardcoded "7". sysrq is typically the last chance to get some information from the system. We actually want to use the hardcoded "7" here. But we should define it via a macro in printk.h, e.g. #define CONSOLE_LOGLEVEL_ALL_NORMAL 7 /* all non-debugging messages */ or #define CONSOLE_LOGLEVEL_NO_DEBUG 7 /* all non-debugging messages */ Best Regards, Petr
diff --git a/arch/x86/platform/uv/uv_nmi.c b/arch/x86/platform/uv/uv_nmi.c index eafc530c8767..4751299c7416 100644 --- a/arch/x86/platform/uv/uv_nmi.c +++ b/arch/x86/platform/uv/uv_nmi.c @@ -100,7 +100,7 @@ static cpumask_var_t uv_nmi_cpu_mask; * Default is all stack dumps go to the console and buffer. * Lower level to send to log buffer only. */ -static int uv_nmi_loglevel = CONSOLE_LOGLEVEL_DEFAULT; +static int uv_nmi_loglevel = CONFIG_CONSOLE_LOGLEVEL_DEFAULT; module_param_named(dump_loglevel, uv_nmi_loglevel, int, 0644); /* diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c index aa8da0a49829..3e8d8f706589 100644 --- a/drivers/firmware/efi/libstub/efi-stub-helper.c +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c @@ -12,7 +12,7 @@ #include <linux/ctype.h> #include <linux/efi.h> #include <linux/kernel.h> -#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_* */ +#include <linux/printk.h> /* For CONSOLE_LOGLEVEL_DEBUG */ #include <asm/efi.h> #include <asm/setup.h> @@ -21,7 +21,7 @@ bool efi_nochunk; bool efi_nokaslr = !IS_ENABLED(CONFIG_RANDOMIZE_BASE); bool efi_noinitrd; -int efi_loglevel = CONSOLE_LOGLEVEL_DEFAULT; +int efi_loglevel = CONFIG_CONSOLE_LOGLEVEL_DEFAULT; bool efi_novamap; static bool efi_nosoftreserve; @@ -213,7 +213,7 @@ efi_status_t efi_parse_options(char const *cmdline) if (!strcmp(param, "nokaslr")) { efi_nokaslr = true; } else if (!strcmp(param, "quiet")) { - efi_loglevel = CONSOLE_LOGLEVEL_QUIET; + efi_loglevel = CONFIG_CONSOLE_LOGLEVEL_QUIET; } else if (!strcmp(param, "noinitrd")) { efi_noinitrd = true; } else if (!strcmp(param, "efi") && val) { diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index 959f9e121cc6..e0ae7793155e 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -103,7 +103,7 @@ static void sysrq_handle_loglevel(int key) int i; i = key - '0'; - console_loglevel = CONSOLE_LOGLEVEL_DEFAULT; + console_loglevel = CONFIG_CONSOLE_LOGLEVEL_DEFAULT; pr_info("Loglevel set to %d\n", i); console_loglevel = i; } @@ -584,7 +584,7 @@ void __handle_sysrq(int key, bool check_mask) * routing in the consumers of /proc/kmsg. */ orig_log_level = console_loglevel; - console_loglevel = CONSOLE_LOGLEVEL_DEFAULT; + console_loglevel = CONFIG_CONSOLE_LOGLEVEL_DEFAULT; op_p = __sysrq_get_key_op(key); if (op_p) { diff --git a/drivers/video/fbdev/core/fbcon.c b/drivers/video/fbdev/core/fbcon.c index bf61598bf1c3..75b97268663f 100644 --- a/drivers/video/fbdev/core/fbcon.c +++ b/drivers/video/fbdev/core/fbcon.c @@ -1043,7 +1043,7 @@ static void fbcon_init(struct vc_data *vc, int init) info = registered_fb[con2fb_map[vc->vc_num]]; - if (logo_shown < 0 && console_loglevel <= CONSOLE_LOGLEVEL_QUIET) + if (logo_shown < 0 && console_loglevel <= CONFIG_CONSOLE_LOGLEVEL_QUIET) logo_shown = FBCON_LOGO_DONTSHOW; if (vc != svc || logo_shown == FBCON_LOGO_DONTSHOW || diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c index e57c00824965..dfb234a0a59d 100644 --- a/drivers/video/fbdev/efifb.c +++ b/drivers/video/fbdev/efifb.c @@ -160,7 +160,7 @@ static void efifb_show_boot_graphics(struct fb_info *info) } /* Avoid flashing the logo if we're going to print std probe messages */ - if (console_loglevel > CONSOLE_LOGLEVEL_QUIET) + if (console_loglevel > CONFIG_CONSOLE_LOGLEVEL_QUIET) return; /* bgrt_tab.status is unreliable, so we don't check it */ diff --git a/include/linux/printk.h b/include/linux/printk.h index fe7eb2351610..fd34b3aa2f90 100644 --- a/include/linux/printk.h +++ b/include/linux/printk.h @@ -46,22 +46,12 @@ static inline const char *printk_skip_headers(const char *buffer) #define CONSOLE_EXT_LOG_MAX 8192 -/* printk's without a loglevel use this.. */ -#define MESSAGE_LOGLEVEL_DEFAULT CONFIG_MESSAGE_LOGLEVEL_DEFAULT - /* We show everything that is MORE important than this.. */ #define CONSOLE_LOGLEVEL_SILENT 0 /* Mum's the word */ #define CONSOLE_LOGLEVEL_MIN 1 /* Minimum loglevel we let people use */ #define CONSOLE_LOGLEVEL_DEBUG 10 /* issue debug messages */ #define CONSOLE_LOGLEVEL_MOTORMOUTH 15 /* You can't shut this one up */ -/* - * Default used to be hard-coded at 7, quiet used to be hardcoded at 4, - * we're now allowing both to be set from kernel config. - */ -#define CONSOLE_LOGLEVEL_DEFAULT CONFIG_CONSOLE_LOGLEVEL_DEFAULT -#define CONSOLE_LOGLEVEL_QUIET CONFIG_CONSOLE_LOGLEVEL_QUIET - extern int console_printk[]; #define console_loglevel (console_printk[0]) diff --git a/init/main.c b/init/main.c index c68d784376ca..4dfcbf7f24c6 100644 --- a/init/main.c +++ b/init/main.c @@ -236,7 +236,7 @@ static int __init debug_kernel(char *str) static int __init quiet_kernel(char *str) { - console_loglevel = CONSOLE_LOGLEVEL_QUIET; + console_loglevel = CONFIG_CONSOLE_LOGLEVEL_QUIET; return 0; } diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c index 5a95c688621f..92b93340905c 100644 --- a/kernel/printk/printk.c +++ b/kernel/printk/printk.c @@ -61,10 +61,10 @@ #include "internal.h" int console_printk[4] = { - CONSOLE_LOGLEVEL_DEFAULT, /* console_loglevel */ - MESSAGE_LOGLEVEL_DEFAULT, /* default_message_loglevel */ + CONFIG_CONSOLE_LOGLEVEL_DEFAULT, /* console_loglevel */ + CONFIG_MESSAGE_LOGLEVEL_DEFAULT, /* default_message_loglevel */ CONSOLE_LOGLEVEL_MIN, /* minimum_console_loglevel */ - CONSOLE_LOGLEVEL_DEFAULT, /* default_console_loglevel */ + CONFIG_CONSOLE_LOGLEVEL_DEFAULT, /* default_console_loglevel */ }; EXPORT_SYMBOL_GPL(console_printk);
CONSOLE_LOGLEVEL_DEFAULT is nothing more than a shorthand of CONFIG_CONSOLE_LOGLEVEL_DEFAULT. When you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from Kconfig, almost all objects are rebuilt because CONFIG_CONSOLE_LOGLEVEL_DEFAULT is used in <linux/printk.h>, which is included from most of source files. In fact, there are only 4 users of CONSOLE_LOGLEVEL_DEFAULT: arch/x86/platform/uv/uv_nmi.c drivers/firmware/efi/libstub/efi-stub-helper.c drivers/tty/sysrq.c kernel/printk/printk.c So, when you change CONFIG_CONSOLE_LOGLEVEL_DEFAULT and rebuild the kernel, it is enough to recompile those 4 files. Remove the CONSOLE_LOGLEVEL_DEFAULT definition from <linux/printk.h>, and use CONFIG_CONSOLE_LOGLEVEL_DEFAULT directly. With this, the build system will rebuild the minimal number of objects. Steps to confirm it: [1] Do the full build [2] Change CONFIG_CONSOLE_LOGLEVEL_DEFAULT from 'make menuconfig' etc. [3] Rebuild $ make SYNC include/config/auto.conf CALL scripts/checksyscalls.sh CALL scripts/atomic/check-atomics.sh DESCEND objtool CHK include/generated/compile.h CC kernel/printk/printk.o AR kernel/printk/built-in.a AR kernel/built-in.a CC drivers/tty/sysrq.o AR drivers/tty/built-in.a CC drivers/firmware/efi/libstub/efi-stub-helper.o STUBCPY drivers/firmware/efi/libstub/efi-stub-helper.stub.o AR drivers/firmware/efi/libstub/lib.a AR drivers/built-in.a GEN .version CHK include/generated/compile.h UPD include/generated/compile.h CC init/version.o AR init/built-in.a LD vmlinux.o ... For the same reason, do likewise for CONSOLE_LOGLEVEL_QUIET and MESSAGE_LOGLEVEL_DEFAULT. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- arch/x86/platform/uv/uv_nmi.c | 2 +- drivers/firmware/efi/libstub/efi-stub-helper.c | 6 +++--- drivers/tty/sysrq.c | 4 ++-- drivers/video/fbdev/core/fbcon.c | 2 +- drivers/video/fbdev/efifb.c | 2 +- include/linux/printk.h | 10 ---------- init/main.c | 2 +- kernel/printk/printk.c | 6 +++--- 8 files changed, 12 insertions(+), 22 deletions(-)