lkml.org 
[lkml]   [2022]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
Subjectcosmetic: was: Re: [patch RFC 19/29] printk: Add basic infrastructure for non-BKL consoles
On Sun 2022-09-11 00:28:01, Thomas Gleixner wrote:
> The current console/printk subsystem is protected by a Big Kernel Lock,
> aka. console_lock which has has ill defined semantics and is more or less
> stateless. This puts severe limitations on the console subsystem and makes
> forced takeover and output in emergency and panic situations a fragile
> endavour which is based on try and pray.
>
> The goal of non-BKL consoles is to break out of the console lock jail and
> to provide a new infrastructure which avoids the pitfalls and allows
> console drivers to be gradually converted over.
>
> The proposed infrastructure aims for the following properties:
>
> - Lockless (SCRU protected) console list walk
> - Per console locking instead of global locking
> - Per console state which allows to make informed decisions
> - Stateful handover and takeover
>
> As a first step this adds state to struct console. The per console state is
> a atomic_long_t with a 32bit bit field and on 64bit a 32bit sequence for
> tracking the last printed ringbuffer sequence number. On 32bit the sequence
> is seperate from state for obvious reasons which requires to handle a few
> extra race conditions.
>
> Add the initial state with the most basic 'alive' and 'enabled' bits and
> wire it up into the console register/unregister functionality and exclude
> such consoles from being handled in the console BKL mechanisms.
>
> The decision to use a bitfield was made as using a plain u32 and mask/shift
> operations turned out to result in uncomprehensible code.
>
> --- a/include/linux/console.h
> +++ b/include/linux/console.h
> @@ -237,6 +272,9 @@ struct console {
> unsigned long dropped;
> void *data;
> struct hlist_node node;
> +
> + /* NOBKL console specific members */
> + atomic_long_t __private atomic_state[2];

Just to be sure about the meaning. "real" state means the current
state and "handover" means a requested state.

> };
>
> #ifdef CONFIG_LOCKDEP
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2339,7 +2339,9 @@ static bool suppress_message_printing(in
> static bool pr_flush(int timeout_ms, bool reset_on_progress) { return true; }
> static bool __pr_flush(struct console *con, int timeout_ms, bool reset_on_progress) { return true; }
>
> -#endif /* CONFIG_PRINTK */
> +#endif /* !CONFIG_PRINTK */
> +
> +#include "printk_nobkl.c"

Is there any chance to get rid of this?

If we need to use some of this API in printk.c then please declare
it either in "internal.h" or in a new "printk_noblk.h".

Honestly, I do not have any real arguments why it is bad. But there
are probably reasons why it is not a common pattern. IMHO, split
sources might help to:

+ speed up compilation
+ separate public and internal API
+ keep #ifdef/#else/#endif close each other in .h files
+ keep the sources somehow usable even without cscope
+ ???


> #ifdef CONFIG_EARLY_PRINTK
> struct console *early_console;
> @@ -2635,6 +2637,13 @@ static bool abandon_console_lock_in_pani
> */
> static inline bool console_is_usable(struct console *con)
> {
> + /*
> + * Exclude the NOBKL consoles. They are handled seperately
> + * as they do not require the console BKL
> + */
> + if ((con->flags & CON_NO_BKL))
> + return false;

This is confusing. Nobody would expect that a function called
"console_is_usable()" would return false just because the console
has CON_NO_BLK flag set.

Either we need a better name, for example, console_is_blk_and_usable().
Or please put the test into a separate function, e.g. console_is_blk()
and check it separately where needed.

IMHO, the original console_is_usable() would be useful even for
CON_NO_BLK consoles.


> +
> if (!(con->flags & CON_ENABLED))
> return false;
>
> --- /dev/null
> +++ b/kernel/printk/printk_nobkl.c
> @@ -0,0 +1,176 @@
> +
> +enum state_selector {
> + STATE_REAL,
> + STATE_HANDOVER,
> +};

It might be problem that I am not a native speaker. But the names
are a bit ambiguous to me. I would personally use:

enum state_selector {
CON_STATE_CURRENT,
CON_STATE_REQUESTED,
};

or if it is too long: CON_STATE_CUR and CON_STATE_REQ.

Well, I do not resist on the change. I am not sure how the proposed names
would play with the followup patches. The original names might
be good after all. They are not that bad. I primary wanted
to document my first reaction ;-)

> +/**
> + * cons_nobkl_init - Initialize the NOBKL console state
> + * @con: Console to initialize
> + */
> +static void cons_nobkl_init(struct console *con)
> +{
> + struct cons_state state = {
> + .alive = 1,
> + .enabled = !!(con->flags & CON_ENABLED),
> + };
> +
> + cons_state_set(con, STATE_REAL, &state);
> +}

IMHO. we need to update the function description, e.g.

/**
* cons_nobkl_init - Initialize the NOBKL console specific data
* @con: Console to initialize
*/


Background:

The function name does not match the rest:

+ The function name suggests that it initializes NOBLK console.

+ The function description and the implementation suggests that
it initializes struct cons_state.

I see that the followup patches update this function. It initializes
all the members needed by noblk consoles in struct console. It
allocates per-CPU data and creates the kthread. It means
that the function name is reasonable after all.


> +
> +/**
> + * cons_nobkl_cleanup - Cleanup the NOBKL console state
> + * @con: Console to cleanup
> + */
> +static void cons_nobkl_cleanup(struct console *con)
> +{
> + struct cons_state state = { };
> +
> + cons_state_set(con, STATE_REAL, &state);
> +}

Same as with cons_noblk_init(). The function does a lot
more in the later patches. The description should be

/**
* cons_nobkl_cleanup - Cleanup the NOBKL console specific data
* @con: Console to cleanup
*/

Best Regards,
Petr

\
 
 \ /
  Last update: 2022-11-07 17:12    [W:0.298 / U:0.200 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site