Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 13 Jul 2022 15:49:19 +0100
From:   Chris Down <chris@chrisdown.name>
To:     Petr Mladek <pmladek@suse.com>
Cc:     linux-kernel@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Sergey Senozhatsky <senozhatsky@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        John Ogness <john.ogness@linutronix.de>,
        Geert Uytterhoeven <geert@linux-m68k.org>, kernel-team@fb.com
Subject: Re: design: was: Re: [RFC PATCH v2] printk: console: Allow each
 console to have its own loglevel
Message-ID: <Ys7bb8+/G0iMWfzb@chrisdown.name>
References: <YoeQLxhDeIk4VSmx@chrisdown.name>
 <YshL2M8VdIEONDdc@alley>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <YshL2M8VdIEONDdc@alley>
User-Agent: Mutt/2.2.6 (2022-06-05)
Precedence: bulk

Thanks for the detailed feedback! Anything not explicitly discussed is acked 
for v3. :-)

Petr Mladek writes:
>> +    * ``minimum``: The global minimum loglevel
>> +      (``kernel.minimum_console_loglevel``) is in effect. Set a higher
>> +      console-specific loglevel to override it.
>
>This is confusing. It might be because I am not a native speaker. It
>sounds like a higher console-specific loglevel might update (override)
>the minimum value.

Ah, yes. I think this higher/lower confusion also has something to do with the 
historic artefacts of using lower-numbered integers to represent 
higher-priority messages.
>
>A better explanation is that it is a limit. Neither the global nor the per-console
>loglevel could be set below this value.

Yes, that sounds better. :-)

>> +    * ``forced_minimum``: The global minimum loglevel
>> +      (``kernel.minimum_console_loglevel``) is in effect. Even if the local
>> +      console-specific loglevel is higher, it is in effect because the global
>> +      forced loglevel (``kernel.force_console_loglevel``) is present, but is
>> +      below ``kernel.minimum_console_loglevel``. Write "unset" to
>> +      ``kernel.force_console_loglevel`` to disable the forcing, and make sure
>> +      ``kernel.minimum_console_loglevel`` is below the local console loglevel
>> +      if you want the per-console loglevel to take effect.
>> +      console-specific loglevel to override it.
>
>IMHO, this makes things too complicated. A better solution is to do
>not allow to set any log level below this limit in the first place.

Hmm, how should we then handle the case that you have set the per-console 
loglevel to 3 and minimum_console_loglevel later gets changed to 5?

We had this problem when designing cgroup v2 as well, for example in cases 
where a child requests a higher memory protection than can be afforded by the 
parent, or where a child sets a higher memory limit than a parent specifies. We 
went back and forth and eventually settled on allowing these, because the 
alternatives seemed difficult to reason about or unnecessarily inflexible.

 From the per-console loglevel side, one option is to return ERANGE or EINVAL on 
values we know won't be honoured when setting the per-console loglevel. The 
problem with that is that it doesn't allow to specify a "desired" limit in case 
the external factors (in this case, the minimum loglevel) change. This is even 
more difficult to reason about in our case because the minimum loglevel may be 
changed dynamically outside of user control.

Another is to disallow setting the minimum loglevel without first resetting 
consoles which are above the value that is desired to be set, but this seems 
really cumbersome, and again it doesn't account for cases like panic() and 
elsewhere where we blindly change it anyway.

Maybe you have another idea about how it should work in the case that the 
minimum loglevel would take precedence over an existing loglevel?

>> +    * ``ignore_loglevel``: ``ignore_loglevel`` was specified on the kernel
>> +      command line. Restart without it to use other controls.
>> +
>> +* ``enabled`` (r): Whether the console is enabled.
>> +* ``loglevel`` (rw): The local loglevel for this console. This will be in
>> +  effect if no other global control overrides it. Look at
>> +  ``effective_loglevel`` and ``effective_loglevel_source`` to verify that.
>>
>> +Deprecated
>> +~~~~~~~~~~
>> +
>> +* ``syslog(SYSLOG_ACTION_CONSOLE_*)``: This sets
>
>Why does it use "_*"? It looks like the entire syslog interface is
>obsolete. But this patch affects only three actions: ON, OFF, LEVEL.

Not totally sure I know what you mean -- SYSLOG_ACTION_CONSOLE_* limits it to 
those, no?

     % git grep -ho 'SYSLOG_ACTION_CONSOLE_[A-Z0-9]\+' | sort -u
     SYSLOG_ACTION_CONSOLE_LEVEL
     SYSLOG_ACTION_CONSOLE_OFF
     SYSLOG_ACTION_CONSOLE_ON

>> +  ``kernel.force_console_loglevel``. It is unaware of per-console loglevel
>> +  semantics and is not recommended. A warning will be emitted if it is used
>> +  while local loglevels are in effect.
>
>Do we really want to obsolete it? It might be enough to say
>that it works as force_console_loglevel.

That's also fine -- my only concern with syslog() is that it's not very 
explicit about what will happen to consoles with a per-console loglevel set.

That said, once this is merged I suppose we can make it more clear in 
downstream consumers like `dmesg -n`, so not a big issue either way. :-)

>I have one idea. A big difference might be replacing
>"force_console_loglevel" value with "ignore_per_console_loglevel" parameter.
>
>[...]
>
>The main logic difference is that we do not have 4 loglevel values
>(global,per-console,minimum,forced) and complicated rules which one
>will be used in the end.
>
>Instead we have only two values: global, and optional
>console-specific. 2nd one is used when and only when defined.
>Plus one or both might ignored when the "ignore*" parameter is used.
>
>Everything else are ways how to set the values at build/boot/run time.
>
>How does it sounds, please?

This sounds pretty good! It had bothered me that we have these two 
default/forced options, and I think this does make things simpler.

Thanks for your detailed feedback! I will incorporate it into v3.