Re: [PATCH v3 2/2] printk: console: Support console-specific loglevels

Chris Down
2022-09-05T14:07:52Z

Hi Petr,

Thanks a lot for getting back! :-)

Any comments not explicitly addressed are acked.

Petr Mladek writes:
>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.

That's great! Thank you for all your help improving it.

>> + */
>> +#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.

Hmm, it's true it can be done without the flag, I mostly preferred to do it
this way out of a concern for code readability.

That said, with the changes suggested below to just show -1 instead of "unset",
maybe the best solution is just to set -1.

I will think about it some more before v4 and probably just have -1 mean unset.
Thanks!

>> +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[].

No strong opinions, so I'll do this for v4. Thanks!

>>
>> @@ -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;

Oh, that's very true. Thanks!

>> + 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");

My concern with this is that this will then warn on basically any first
syslog() use, even for people who don't care about the per-console loglevel
semantics. They will now get the warning, since by default
ignore_per_console_loglevel isn't true -- however no per-console loglevel is
set either, so it's not really relevant.

That's why I implemented it as warn_on_local_loglevel() checking for
CON_LOGLEVEL, because otherwise it seems noisy for those that are not using the
feature.

Maybe you have thoughts on that? :-)

>> +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.

That's fair. In which case we probably end up with one error for all cases: do
you prefer we should return -EINVAL or -ERANGE?

>> +
>> 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.

I think this one is a little complicated: on the one hand, yes, it does seem
more ergonomic to keep everything together in /sys/class/console. On the other
hand, this means that users can no longer use the sysctl infrastructure, which
makes things more unwieldy than with kernel.printk.

Not really a problem with sysfs as much as a problem with userspace ergonomics:
sysctls have a really easy way to set them at boot, but sysfs stuff, not so.
You can hack it with systemd-tmpfiles, a boot unit, or similar, but the
infrastructure is a lot less specialised and requires more work. I am worried
that people may complain that that's unhelpful, especially since we're
deprecating kernel.printk.

Maybe I shouldn't worry about that so much? I'm curious to hear your further
thoughts.

Thanks a lot for the detailed feedback!

Chris