lkml.org 
[lkml]   [2022]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH v3 2/2] printk: console: Support console-specific loglevels
On Wed 2022-07-20 18:48:16, Chris Down wrote:
> In terms of technical implementation, this patch embeds a device pointer
> in the console struct, and registers each console using it so we can
> expose attributes in sysfs.
>
> For information on the precedence and application of the new controls,
> see Documentation/ABI/testing/sysfs-class-console and
> Documentation/admin-guide/per-console-loglevel.rst.

The overall logic looks good to me. I finally have good feeling that
the behavior is "easy" to understand.

The detailed documentation is very nice!

See below for comments about various implementation details.

> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -137,6 +138,22 @@ static inline int con_debug_leave(void)
> #define CON_BRL (32) /* Used for a braille device */
> #define CON_EXTENDED (64) /* Use the extended output format a la /dev/kmsg */
>
> +/*
> + * The loglevel for a console can be set in many places:
> + *
> + * 1. It can be forced to emit everything (ignore_loglevel);
> + * 2. It can be set globally (sysctls kernel.printk (deprecated),
> + * kernel.console_loglevel, magic sysrq, loglevel= on kernel command line);
> + * 3. It can be locally set for this specific console (console=...,loglevel:N on
> + * kernel command line, /sys/class/console/.../loglevel);
> + * 4. It can be set by a compile-time default
> + * (CONFIG_CONSOLE_LOGLEVEL_{DEFAULT,QUIET})
> + *
> + * If case 3 happens, even if another global value in effect, CON_LOGLEVEL will
> + * be set.

The last sentence is not clear to me.

Well, I suggest to keep it simple and remove this comment
completely. The meaning of the flag is simple. It is set when
the local (per-console) loglevel is set.

The precedence of the various loglevel setting is explained above
console_effective_loglevel() where it belongs.

> + */
> +#define CON_LOGLEVEL (128) /* Level set locally for this console */

I would write:

#define CON_LOGLEVEL (128) /* Local (per-console) loglevel is set. */

Alternatively we could avoid the flag completely. The per-console
loglevel is set when con->level > 0. A valid value must never
be below CONSOLE_MIN_LOGLEVEL which is 1. And it is perfectly fine
to say that 0 or -1 is not a valid loglevel. The same effect could
be achieved by disabling the console completely.

I do not have strong opinion. The flag has obvious meaning and might
make the code better readable. On the other hand, it adds an extra
code and complexity.

I slightly prefer to do it without the flag.

Anyway, if we add the new flag, we should also show it in
/proc/consoles, see fs/proc/consoles.c.

> +
> struct console {
> char name[16];
> void (*write)(struct console *, const char *, unsigned);
> @@ -155,8 +172,15 @@ struct console {
> unsigned long dropped;
> void *data;
> struct console *next;
> + int level;
> + struct device *classdev;
> };
>
> +static inline struct console *classdev_to_console(struct device *dev)
> +{
> + return dev_get_drvdata(dev);
> +}

Please, open code this in the _show()/_store() callbacks. dev_get_drvdata()
seems to be the standard way how it is done. Hiding it into a custom
function just adds an extra step when reading the code.

> +
> /*
> * for_each_console() allows you to iterate on each console
> */
> diff --git a/kernel/printk/console_cmdline.h b/kernel/printk/console_cmdline.h
> index 3ca74ad391d6..40f1a1ff0965 100644
> --- a/kernel/printk/console_cmdline.h
> +++ b/kernel/printk/console_cmdline.h
> @@ -6,6 +6,8 @@ struct console_cmdline
> {
> char name[16]; /* Name of the driver */
> int index; /* Minor dev. to use */
> + int level; /* Log level to use */
> + short flags; /* Initial flags */
> bool user_specified; /* Specified by command line vs. platform */
> char *options; /* Options for the driver */
> #ifdef CONFIG_A11Y_BRAILLE_CONSOLE
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6094f773ad4a..6f5e29b60875 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1202,9 +1217,72 @@ module_param(ignore_loglevel, bool, S_IRUGO | S_IWUSR);
> MODULE_PARM_DESC(ignore_loglevel,
> "ignore loglevel setting (prints all kernel messages to the console)");
>
> -static bool suppress_message_printing(int level)
> +static bool __read_mostly ignore_per_console_loglevel;
> +
> +static int __init ignore_per_console_loglevel_setup(char *str)
> +{
> + ignore_per_console_loglevel = true;
> + return 0;
> +}
> +
> +early_param("ignore_per_console_loglevel", ignore_per_console_loglevel_setup);
> +module_param(ignore_per_console_loglevel, bool, 0644);
> +MODULE_PARM_DESC(ignore_per_console_loglevel,
> + "ignore per-console loglevel setting (only respect global console loglevel)");
> +
> +/*
> + * Hierarchy of loglevel authority:
> + *
> + * 1. con->level. The locally set, console-specific loglevel. Optional, only
> + * valid if the CON_LOGLEVEL flag is set.
> + * 2. console_loglevel. The default global console loglevel, always present.
> + *
> + * The behaviour can be further changed by the following printk module
> + * parameters:
> + *
> + * 1. ignore_loglevel. Can be set at boot or at runtime with
> + * /sys/module/printk/parameters/ignore_loglevel. Overrides absolutely
> + * everything since it's used to debug.
> + * 2. ignore_per_console_loglevel. Existing per-console loglevel values are left
> + * intact, but are ignored in favour of console_loglevel as long as this is
> + * true.
> + *
> + * Callers typically only need the level _or_ the source, but they're both
> + * emitted from this function so that the effective loglevel logic can be
> + * kept in one place.
> + */
> +static int console_effective_loglevel(const struct console *con,
> + enum loglevel_source *source)
> +{
> + enum loglevel_source lsource;
> + int level;
> +
> + if (ignore_loglevel) {
> + lsource = LLS_IGNORE_LOGLEVEL;
> + level = CONSOLE_LOGLEVEL_MOTORMOUTH;
> + goto out;
> + }
> +
> + if (!ignore_per_console_loglevel &&
> + (con && (con->flags & CON_LOGLEVEL))) {
> + lsource = LLS_LOCAL;
> + level = con->level;
> + goto out;
> + }
> +
> + lsource = LLS_GLOBAL;
> + level = console_loglevel;
> +
> +out:
> + *source = lsource;
> + return level;
> +}

It might be a matter of taste. But I would probably do it the
following way (note that these would not be used in
boot_delay_msec()):

static int console_effective_loglevel(const struct console *con)
{
enum loglevel_source source;
int level;

if (WARN_ON_ONCE(!con))
return;

source = console_effective_loglevel_source(con);

switch (source) {
case LLS_IGNORE_LOGLEVEL:
level = CONSOLE_LOGLEVEL_MOTORMOUTH;
break;
case LSS_LOCAL:
level = con->level;
break;
case LSS_GLOBAL:
level = console_loglevel;
break;
default:
pr_warn("Unhandled console loglevel source: %d, source);
level = default_console_loglevel;
break;
}

return level;
}

static const char *console_effective_loglevel_source_str(const struct *con)
{
enum loglevel_source source;
const char *str;

if (WARN_ON_ONCE(!con))
return;

source = console_effective_loglevel_source(con);
switch (source) {
case LLS_IGNORE_LOGLEVEL:
str = "ignore_loglevel";
break;
case LSS_LOCAL:
str = "local"
break;
case LSS_GLOBAL:
str = "global";
break;
default:
pr_warn("Unhandled console loglevel source: %d, source);
str = "unknown";
break;
}
return str;
}
static enum loglevel_source
console_effective_loglevel_source(const struct console *con)
{
if (WARN_ON_ONCE(!con))
return;
if (ignore_loglevel)
return LLS_IGNORE_LOGLEVEL;
if (con->flags & CON_LOGLEVEL && !ignore_per_console_loglevel))
return LLS_LOCAL;
return LLS_GLOBAL;
}
It looks like a bit cleaner and better separated (layered) logic.

There is no need to define "enum loglevel_source" variable when
the caller is interested only into the loglevel value.

The advantage of console_effective_loglevel_source_str() is that it
always returns a valid string. It prevents a potential out-of-bound
access to loglevel_source_names[].

> +
> +static bool suppress_message_printing(int level, struct console *con)
> {
> - return (level >= console_loglevel && !ignore_loglevel);
> + enum loglevel_source source;
> +
> + return level >= console_effective_loglevel(con, &source);
> }
>
> #ifdef CONFIG_BOOT_PRINTK_DELAY
> @@ -1236,7 +1314,7 @@ static void boot_delay_msec(int level)
> unsigned long timeout;
>
> if ((boot_delay == 0 || system_state >= SYSTEM_RUNNING)
> - || suppress_message_printing(level)) {
> + || suppress_message_printing(level, NULL)) {

This does not take into account per-console loglevels.
And we could not check them in vprintk_emit() because
we could not take console_lock() there.

AFAIK, the purpose of this call is to allow reading the messages
on consoles when they do not support scrolling.

A solution would be to call boot_delay_msec() in
console_flush_all(). It would need adding parameter
into console_emit_next_record() that pass information
whether it emitted or suppressed the message.
Something like:

/*
[...]
* @emitted will be set to "true" when the message was really emitted to the
* console. It means that it was not suppressed because of console loglevel.
[...]
*/
static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
char *dropped_text, bool *emitted, bool *handover)

> return;
> }
>
> @@ -1701,12 +1806,14 @@ int do_syslog(int type, char __user *buf, int len, int source)
> break;
> /* Disable logging to console */
> case SYSLOG_ACTION_CONSOLE_OFF:
> + warn_on_local_loglevel();
> if (saved_console_loglevel == LOGLEVEL_DEFAULT)
> saved_console_loglevel = console_loglevel;
> console_loglevel = minimum_console_loglevel;
> break;

We actually could disable logging on all consoles by setting
ignore_per_console_loglevel. Something like:

case SYSLOG_ACTION_CONSOLE_OFF:
if (saved_console_loglevel == LOGLEVEL_DEFAULT) {
saved_console_loglevel = console_loglevel;
saved_ignore_per_console_loglevel = ignore_per_console_loglevel;
}
console_loglevel = minimum_console_loglevel;
ignore_per_console_loglevel = true;
break;


> /* Enable logging to console */
> case SYSLOG_ACTION_CONSOLE_ON:
> + warn_on_local_loglevel();
> if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
> console_loglevel = saved_console_loglevel;
> saved_console_loglevel = LOGLEVEL_DEFAULT;

and here:

case SYSLOG_ACTION_CONSOLE_ON:
if (saved_console_loglevel != LOGLEVEL_DEFAULT) {
console_loglevel = saved_console_loglevel;
ignore_per_console_loglevel = saved_ignore_per_console_loglevel;
saved_console_loglevel = LOGLEVEL_DEFAULT;
}

> @@ -1714,6 +1821,7 @@ int do_syslog(int type, char __user *buf, int len, int source)
> break;
> /* Set level of messages printed to console */
> case SYSLOG_ACTION_CONSOLE_LEVEL:
> + warn_on_local_loglevel();

I would keep it simple:

if (!ignore_per_console_loglevel)
pr_warn_once("SYSLOG_ACTION_CONSOLE_LEVEL is ignored by consoles with explicitely set per-console loglevel, see Documentation/admin-guide/per-console-loglevel\n");
People should know that this API has limits. The check of
ignore_per_console_loglevel allows to prevent the warning
when the users are not interested into the per-console
loglevels feature.

I think about opening discussion about obsoleting the syslog
syscall. It has many drawbacks. Everything is stored in global
variables. As a result, it supports only one reader and one
writer.


> if (len < 1 || len > 8)
> return -EINVAL;
> if (len < minimum_console_loglevel)
> @@ -3030,6 +3149,145 @@ static int __init keep_bootcon_setup(char *str)
>
> early_param("keep_bootcon", keep_bootcon_setup);
>
> +#ifdef CONFIG_PRINTK
> +static ssize_t loglevel_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct console *con = classdev_to_console(dev);
> +
> + if (con->flags & CON_LOGLEVEL)
> + return sysfs_emit(buf, "%d\n", con->level);
> + else
> + return sysfs_emit(buf, "unset\n");

I can't find any other sysfs interface using this style (number or
"unset"). Or any other interface combining number or a string values.

I feel a bit uneasy to introduce a completely new semantic. Instead I
prefer using either -1 or 0 when the per-console is not set. It might
be slightly less obvious but it is a standard way so it might be
easier for users in the end.

Both 0 and -1 values are usable as explained above. I personally
prefer -1 because the meaning is more obvious. I would even
use it instead of CON_LOGLEVEL flag.


> +}
> +
> +static ssize_t loglevel_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t size)
> +{
> + struct console *con = classdev_to_console(dev);
> + ssize_t ret;
> + int tmp;
> +
> + if (!strcmp(buf, "unset") || !strcmp(buf, "unset\n")) {
> + con->flags &= ~CON_LOGLEVEL;
> + return size;
> + }
> +
> + ret = kstrtoint(buf, 10, &tmp);
> + if (ret < 0)
> + return ret;
> +
> + if (tmp < LOGLEVEL_EMERG || tmp > LOGLEVEL_DEBUG + 1)
> + return -ERANGE;
> +
> + if (tmp < minimum_console_loglevel)
> + return -EINVAL;

This looks superfluous. Please, use minimum_console_loglevel
instead of LOGLEVEL_EMERG in the above range check.

Also, we should make a helper function for this. The same range check
is useful also for the global console_loglevel.

> +
> + con->level = tmp;
> + con->flags |= CON_LOGLEVEL;
> +
> + return size;
> +}
> +
> +static DEVICE_ATTR_RW(loglevel);
> +
[...]
> +static struct attribute *console_sysfs_attrs[] = {
> + &dev_attr_loglevel.attr,
> + &dev_attr_effective_loglevel_source.attr,
> + &dev_attr_effective_loglevel.attr,
> + &dev_attr_enabled.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(console_sysfs);
> +
> +static void console_classdev_release(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static void console_register_device(struct console *new)

Please use "con" instead of "new" like it is done in the other
API manipulating struct console. It helps when reading the code.

> +{
> + /*
> + * We might be called from register_console() before the class is
> + * registered. If that happens, we'll take care of it in
> + * printk_late_init.
> + */
> + if (IS_ERR_OR_NULL(console_class))
> + return;
> +

We should check whether new->classdev is NULL to prevent double
initialization. It should not happen but better be on the safe side.


> + new->classdev = kzalloc(sizeof(struct device), GFP_KERNEL);
> + if (!new->classdev)
> + return;
> +
> + device_initialize(new->classdev);
> + dev_set_name(new->classdev, "%s", new->name);

This should be:

dev_set_name(new->classdev, "%s%d", con->name, con->index);

It should match console names defined on the command line
and shown by /proc/consoles. See how the name is printed in
fs/proc/consoles.c.

> + dev_set_drvdata(new->classdev, new);
> + new->classdev->release = console_classdev_release;
> + new->classdev->class = console_class;
> + if (device_add(new->classdev))
> + put_device(new->classdev);
> +}
> +
> +static void console_setup_class(void)
> +{
> + struct console *con;
> +
> + /*
> + * printk exists for the lifetime of the kernel, it cannot be unloaded,
> + * so we should never end up back in here.
> + */
> + if (WARN_ON(console_class))
> + return;
> +
> + console_class = class_create(THIS_MODULE, "console");
> + if (!IS_ERR(console_class))
> + console_class->dev_groups = console_sysfs_groups;
> +
> + for_each_console(con)
> + console_register_device(con);

This should be done under console_lock so that the list of registered
console could not get manipulated.

> +}
> +#else /* CONFIG_PRINTK */
> +static void console_register_device(struct console *new) {}
> +static void console_setup_class(void) {}
> +#endif
> +
> /*
> * This is called by register_console() to try to match
> * the newly registered console with any of the ones selected
> --- a/kernel/printk/sysctl.c
> +++ b/kernel/printk/sysctl.c
> @@ -7,10 +7,14 @@
> #include <linux/printk.h>
> #include <linux/capability.h>
> #include <linux/ratelimit.h>
> +#include <linux/console.h>
> #include "internal.h"
>
> static const int ten_thousand = 10000;
>
> +static int min_loglevel = LOGLEVEL_EMERG;

We should use minimum_console_loglevel instead.

> +static int max_loglevel = LOGLEVEL_DEBUG + 1;

Please, define maximum_console_loglevel in kernel/printk/printk.c
after the console_printk[4] array.

This will allow to create a function for the range check that might
be used for both sysfs and proc interface.

Also I would set the maximal value to CONSOLE_LOGLEVEL_MOTORMOUTH.

> +
> static int proc_dointvec_minmax_sysadmin(struct ctl_table *table, int write,
> void *buffer, size_t *lenp, loff_t *ppos)
> {
> @@ -76,6 +122,22 @@ static struct ctl_table printk_sysctls[] = {
> .extra1 = SYSCTL_ZERO,
> .extra2 = SYSCTL_TWO,
> },
> + {
> + .procname = "console_loglevel",
> + .data = &console_loglevel,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = printk_console_loglevel,
> + },
> + {
> + .procname = "default_message_loglevel",
> + .data = &default_message_loglevel,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &min_loglevel,
> + .extra2 = &max_loglevel,
> + },

Is there any chance to add this into /sys/class/console instead?
I mean:

/sys/class/console/loglevel
/sys/class/console/default_message_loglevel

It would be nice to have the things are on the same place.
Especially it would be nice to have the global loglevel there.

Best Regards,
Petr

\
 
 \ /
  Last update: 2022-09-01 11:37    [W:0.193 / U:0.084 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site