From: Thomas Gleixner <tglx@linutronix.de>

Unprotected list walks are a brilliant idea. Especially in the context of
hotpluggable consoles.

The new list lock provides not only synchronization for console list
manipulation, but also for manipulation of console->flags:

    console_list_lock();
    console_lock();

    /* may now manipulate the console list and/or console->flags */

    console_unlock();
    console_list_unlock();

Therefore it is safe to iterate the console list and read console->flags
if holding either the console lock or the console list lock.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 include/linux/console.h | 30 +++++++++++++--
 kernel/printk/printk.c  | 84 ++++++++++++++++++++++++++++++++++-------
 2 files changed, 98 insertions(+), 16 deletions(-)

diff --git a/include/linux/console.h b/include/linux/console.h
index 8c1686e2c233..24344f9b0bc1 100644
--- a/include/linux/console.h
+++ b/include/linux/console.h
@@ -157,10 +157,34 @@ struct console {
 	struct	 console *next;
 };
 
-/*
- * for_each_console() allows you to iterate on each console
+#ifdef CONFIG_LOCKDEP
+extern void lockdep_assert_console_list_lock_held(void);
+#else
+static inline void lockdep_assert_console_list_lock_held(void) { }
+#endif
+
+extern void console_list_lock(void) __acquires(console_mutex);
+extern void console_list_unlock(void) __releases(console_mutex);
+
+/**
+ * for_each_registered_console() - Iterator over registered consoles
+ * @con:	struct console pointer used as loop cursor
+ *
+ * Requires console_list_lock to be held. Can only be invoked from
+ * preemptible context.
+ */
+#define for_each_registered_console(con)				\
+	lockdep_assert_console_list_lock_held();			\
+	for (con = console_drivers; con != NULL; con = con->next)
+
+/**
+ * for_each_console() - Iterator over registered consoles
+ * @con:	struct console pointer used as loop cursor
+ *
+ * Requires console_lock to be held which guarantees that the
+ * list is immutable.
  */
-#define for_each_console(con) \
+#define for_each_console(con)						\
 	for (con = console_drivers; con != NULL; con = con->next)
 
 extern int console_set_on_cmdline;
diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index e4f1e7478b52..8c56f6071873 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -79,10 +79,17 @@ int oops_in_progress;
 EXPORT_SYMBOL(oops_in_progress);
 
 /*
- * console_sem protects the console_drivers list, and also
- * provides serialisation for access to the entire console
- * driver system.
+ * console_sem protects the console_drivers list, and also provides
+ * serialization for access to the entire console driver system.
+ *
+ * console_mutex serializes register/unregister.
+ *
+ * console_sem must be taken inside a console_mutex locked section
+ * for any list manipulation in order to keep the console BKL
+ * machinery happy. This requirement also applies to manipulation
+ * of console->flags.
  */
+static DEFINE_MUTEX(console_mutex);
 static DEFINE_SEMAPHORE(console_sem);
 struct console *console_drivers;
 EXPORT_SYMBOL_GPL(console_drivers);
@@ -103,6 +110,12 @@ static int __read_mostly suppress_panic_printk;
 static struct lockdep_map console_lock_dep_map = {
 	.name = "console_lock"
 };
+
+void lockdep_assert_console_list_lock_held(void)
+{
+	lockdep_assert_held(&console_mutex);
+}
+
 #endif
 
 enum devkmsg_log_bits {
@@ -220,6 +233,28 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
 }
 #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
 
+/**
+ * console_list_lock - Lock the console list
+ *
+ * For non-console related list walks, e.g. procfs, sysfs...
+ */
+void console_list_lock(void)
+{
+	mutex_lock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_lock);
+
+/**
+ * console_list_unlock - Unlock the console list
+ *
+ * Counterpart to console_list_lock()
+ */
+void console_list_unlock(void)
+{
+	mutex_unlock(&console_mutex);
+}
+EXPORT_SYMBOL(console_list_unlock);
+
 /*
  * Helper macros to handle lockdep when locking/unlocking console_sem. We use
  * macros instead of functions so that _RET_IP_ contains useful information.
@@ -2978,17 +3013,21 @@ struct tty_driver *console_device(int *index)
 void console_stop(struct console *console)
 {
 	__pr_flush(console, 1000, true);
+	console_list_lock();
 	console_lock();
 	console->flags &= ~CON_ENABLED;
 	console_unlock();
+	console_list_unlock();
 }
 EXPORT_SYMBOL(console_stop);
 
 void console_start(struct console *console)
 {
+	console_list_lock();
 	console_lock();
 	console->flags |= CON_ENABLED;
 	console_unlock();
+	console_list_unlock();
 	__pr_flush(console, 1000, true);
 }
 EXPORT_SYMBOL(console_start);
@@ -3081,6 +3120,8 @@ static void try_enable_default_console(struct console *newcon)
 	       (con->flags & CON_BOOT) ? "boot" : "",	\
 	       con->name, con->index, ##__VA_ARGS__)
 
+static int console_unregister_locked(struct console *console);
+
 /*
  * The console driver calls this routine during kernel initialization
  * to register the console printing procedure with printk() and to
@@ -3107,13 +3148,14 @@ void register_console(struct console *newcon)
 	bool realcon_enabled = false;
 	int err;
 
-	for_each_console(con) {
+	console_list_lock();
+	for_each_registered_console(con) {
 		if (WARN(con == newcon, "console '%s%d' already registered\n",
 					 con->name, con->index))
-			return;
+			goto unlock;
 	}
 
-	for_each_console(con) {
+	for_each_registered_console(con) {
 		if (con->flags & CON_BOOT)
 			bootcon_enabled = true;
 		else
@@ -3124,7 +3166,7 @@ void register_console(struct console *newcon)
 	if (newcon->flags & CON_BOOT && realcon_enabled) {
 		pr_info("Too late to register bootconsole %s%d\n",
 			newcon->name, newcon->index);
-		return;
+		goto unlock;
 	}
 
 	/*
@@ -3155,7 +3197,7 @@ void register_console(struct console *newcon)
 
 	/* printk() messages are not printed to the Braille console. */
 	if (err || newcon->flags & CON_BRL)
-		return;
+		goto unlock;
 
 	/*
 	 * If we have a bootconsole, and are switching to a real console,
@@ -3209,14 +3251,17 @@ void register_console(struct console *newcon)
 	if (bootcon_enabled &&
 	    ((newcon->flags & (CON_CONSDEV | CON_BOOT)) == CON_CONSDEV) &&
 	    !keep_bootcon) {
-		for_each_console(con)
+		for_each_console(con) {
 			if (con->flags & CON_BOOT)
-				unregister_console(con);
+				console_unregister_locked(con);
+		}
 	}
+unlock:
+	console_list_unlock();
 }
 EXPORT_SYMBOL(register_console);
 
-int unregister_console(struct console *console)
+static int console_unregister_locked(struct console *console)
 {
 	struct console *con;
 	int res;
@@ -3269,6 +3314,16 @@ int unregister_console(struct console *console)
 
 	return res;
 }
+
+int unregister_console(struct console *console)
+{
+	int res;
+
+	console_list_lock();
+	res = console_unregister_locked(console);
+	console_list_unlock();
+	return res;
+}
 EXPORT_SYMBOL(unregister_console);
 
 /*
@@ -3320,7 +3375,8 @@ static int __init printk_late_init(void)
 	struct console *con;
 	int ret;
 
-	for_each_console(con) {
+	console_list_lock();
+	for_each_registered_console(con) {
 		if (!(con->flags & CON_BOOT))
 			continue;
 
@@ -3337,9 +3393,11 @@ static int __init printk_late_init(void)
 			 */
 			pr_warn("bootconsole [%s%d] uses init memory and must be disabled even before the real one is ready\n",
 				con->name, con->index);
-			unregister_console(con);
+			console_unregister_locked(con);
 		}
 	}
+	console_list_unlock();
+
 	ret = cpuhp_setup_state_nocalls(CPUHP_PRINTK_DEAD, "printk:dead", NULL,
 					console_cpu_notify);
 	WARN_ON(ret < 0);
-- 
2.30.2

Resending my review on this patch also in this patchset.
I sent the review also to the RFC patchset by mistake, see
https://lore.kernel.org/r/YzLIy4emYX6JpzuN@alley

Please, continue the discussion here where I did review of the other patches.

On Sat 2022-09-24 02:10:42, John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Unprotected list walks are a brilliant idea. Especially in the context of
> hotpluggable consoles.

Yeah, it is crazy. And it is there probably since the beginning.

> The new list lock provides not only synchronization for console list
> manipulation, but also for manipulation of console->flags:
> 
>     console_list_lock();
>     console_lock();
> 
>     /* may now manipulate the console list and/or console->flags */
> 
>     console_unlock();
>     console_list_unlock();
> 
> Therefore it is safe to iterate the console list and read console->flags
> if holding either the console lock or the console list lock.
> 
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -79,10 +79,17 @@ int oops_in_progress;
>  EXPORT_SYMBOL(oops_in_progress);
>  
>  /*
> - * console_sem protects the console_drivers list, and also
> - * provides serialisation for access to the entire console
> - * driver system.
> + * console_sem protects the console_drivers list, and also provides
> + * serialization for access to the entire console driver system.
> + *
> + * console_mutex serializes register/unregister.
> + *
> + * console_sem must be taken inside a console_mutex locked section
> + * for any list manipulation in order to keep the console BKL
> + * machinery happy. This requirement also applies to manipulation
> + * of console->flags.
>   */
> +static DEFINE_MUTEX(console_mutex);
>  static DEFINE_SEMAPHORE(console_sem);
>  struct console *console_drivers;
>  EXPORT_SYMBOL_GPL(console_drivers);
> @@ -220,6 +233,28 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  }
>  #endif /* CONFIG_PRINTK && CONFIG_SYSCTL */
>  
> +/**
> + * console_list_lock - Lock the console list
> + *
> + * For non-console related list walks, e.g. procfs, sysfs...
> + */
> +void console_list_lock(void)
> +{
> +	mutex_lock(&console_mutex);
> +}
> +EXPORT_SYMBOL(console_list_lock);
> +
> +/**
> + * console_list_unlock - Unlock the console list
> + *
> + * Counterpart to console_list_lock()
> + */
> +void console_list_unlock(void)
> +{
> +	mutex_unlock(&console_mutex);
> +}
> +EXPORT_SYMBOL(console_list_unlock);
> +
>  /*
>   * Helper macros to handle lockdep when locking/unlocking console_sem. We use
>   * macros instead of functions so that _RET_IP_ contains useful information.
> @@ -3081,6 +3120,8 @@ static void try_enable_default_console(struct console *newcon)
>  	       (con->flags & CON_BOOT) ? "boot" : "",	\
>  	       con->name, con->index, ##__VA_ARGS__)
>  
> +static int console_unregister_locked(struct console *console);
> +
>  /*
>   * The console driver calls this routine during kernel initialization
>   * to register the console printing procedure with printk() and to
> @@ -3107,13 +3148,14 @@ void register_console(struct console *newcon)
>  	bool realcon_enabled = false;
>  	int err;
>  
> -	for_each_console(con) {
> +	console_list_lock();

Hmm, the new mutex is really nasty. It has very strange semantic.
It makes the locking even more complicated.

The ideal solution would be take console_lock() here. We (me and
Sergey) never did it because con->match() and con->setup()
callbacks were called in try_enable_*console(). We were afraid
that some might want to take console_lock() and it could create
a deadlock. There were too many drivers and we did not found time
to check them all. And it had low priority because nobody reported
problems.

A good enough solution might be call this under the later
added srcu_read_lock(&console_srcu) and use for_each_console_srcu().

The srcu walk would prevent seeing broken list. Obviously,
the code might see outdated list and do bad decisions:

  + try to enable the same console twice

  + enable more consoles by default in try_enable_default_console()

  + associate more consoles with /dev/console, see CON_CONSDEV in
    try_enable_preferred_console() and try_enable_default_console()

If we race then we could end up with more consoles enabled by default
and with more consoles with CON_CONSDEV flag.

IMHO, the rcu walk is an acceptable and conservative solution.
Registering the same driver twice is hard to imagine at all.
And I have never seen reports about too many default consoles
or CON_CONSDEV flags.

Anyway, I would like to avoid adding console_mutex. From my POV,
it is a hack that complicates the code. Taking console_lock()
should be enough. Using rcu walk would be good enough.

Do I miss something, please?

Or is this part of some strategy to remove console_sem later, please?

> +	for_each_registered_console(con) {
>  		if (WARN(con == newcon, "console '%s%d' already registered\n",
>  					 con->name, con->index))
> -			return;
> +			goto unlock;
>  	}
>  
> -	for_each_console(con) {
> +	for_each_registered_console(con) {
>  		if (con->flags & CON_BOOT)
>  			bootcon_enabled = true;
>  		else

Best Regards,
Petr

On 2022-09-27, Petr Mladek <pmladek@suse.com> wrote:
> Hmm, the new mutex is really nasty. It has very strange semantic.
> It makes the locking even more complicated.

We are working to replace the BKL-console_lock with new separate clearly
defined mechanisms.

The new mutex provides full synchronization for list changes as well as
changes to items of that list. (Really console->flags is the only change
to items of the list.)

For some places in the code it is very clear that the console_lock can
be completely replaced (either with srcu or the new mutex). For other
places, it is not yet clear why the console_lock is being used and so
both console_lock and mutex are used.

> The ideal solution would be take console_lock() here.

We should be looking where we can remove console_lock, not identifying
new locations to add it.

> A good enough solution might be call this under the later added
> srcu_read_lock(&console_srcu) and use for_each_console_srcu().

@console_srcu does not allow safe reading of console->flags. It only
provides safe list iteration and reading of immutable fields. The new
mutex must be used for reading console->flags.

Note that for the NOBKL consoles (not part of this series), a new atomic
state variable is used so that console->flags is not needed. That means
for NOBKL consoles the new mutex is further reduced in scope to provide
only list synchronization.

> Or is this part of some strategy to remove console_sem later, please?

Yes! One of the main points of this final phase of the rework is to
remove console_sem usage (for NOBKL consoles). If a system is running
with only NOBKL consoles registered, ideally that system should never
call console_lock()/console_trylock(). Once all drivers have converted
over to the NOBKL interface, console_sem will serve no purpose for the
printk and console frameworks, so it can be removed.

John

On Thu 2022-09-29 01:48:29, John Ogness wrote:
> On 2022-09-27, Petr Mladek <pmladek@suse.com> wrote:
> > Hmm, the new mutex is really nasty. It has very strange semantic.
> > It makes the locking even more complicated.
> 
> We are working to replace the BKL-console_lock with new separate clearly
> defined mechanisms.
>
> The new mutex provides full synchronization for list changes as well as
> changes to items of that list. (Really console->flags is the only change
> to items of the list.)

OK.


> For some places in the code it is very clear that the console_lock can
> be completely replaced (either with srcu or the new mutex). For other
> places, it is not yet clear why the console_lock is being used and so
> both console_lock and mutex are used.

One important and tricky location is console_trylock() in
vprintk_emit(). And the related for_each_console() called from
console_unlock()->console_flush_all().

It is the legacy mode that tries to print to the consoles immediately.
I am not sure if we could _ever_ remove this mode.

And it is most likely the main reason why semaphore is used instead
of a mutex:

    + printk() can be called in atomic context

    + also there is the console_trylock_spinning() trick that allows
      to transfer the semaphore to another owner without locking.


Do you see any RT-friendly solution for the legacy mode, please?

Maybe, an atomic variable (cmpxchg) can be used together with
the SRCU list. But I am not sure if srcu_read_lock can be
transferred to another context. Also this would not solve priority
inversion. Not to say that it might kill SRCU performance on
the entire system.


> > The ideal solution would be take console_lock() here.
>
> We should be looking where we can remove console_lock, not identifying
> new locations to add it.

Yes, we do not want this big kernel lock. Honestly, I am not
completely sure what is the exact purpose. My guess is that
console_lock() is used to prevent calling con->write() when
some internal console driver state is manipulated.

If the above is true then it might be solvable by some
driver-specific lock. The question is where the lock should
be. It is possible that it might require adding
the lock into struct console.

Anyway, some lock will still be needed to synchronize the list.
But could it be mutex? What about the legacy mode of printk_emit()?

> > A good enough solution might be call this under the later added
> > srcu_read_lock(&console_srcu) and use for_each_console_srcu().
> 
> @console_srcu does not allow safe reading of console->flags. It only
> provides safe list iteration and reading of immutable fields. The new
> mutex must be used for reading console->flags.
> 
> Note that for the NOBKL consoles (not part of this series), a new atomic
> state variable is used so that console->flags is not needed. That means
> for NOBKL consoles the new mutex is further reduced in scope to provide
> only list synchronization.

Good to know.

> > Or is this part of some strategy to remove console_sem later, please?
> 
> Yes! One of the main points of this final phase of the rework is to
> remove console_sem usage (for NOBKL consoles). If a system is running
> with only NOBKL consoles registered, ideally that system should never
> call console_lock()/console_trylock(). Once all drivers have converted
> over to the NOBKL interface, console_sem will serve no purpose for the
> printk and console frameworks, so it can be removed.

Is this realistic?

And even if we convert all console drivers then people still might
want the legacy mode.

My understanding is that some atomic consoles would be real hacks.
They might be good enough for panic(). But what about running system.
It seems that people might want the legacy more even on running
system. Will it be doable with mutex?


I am sorry. I was about to answer this mail with "fair enough". But
then I thought more about it...

I would really like to avoid state where we have two locks (semaphore
and mutex) serializing the same thing (console list).

Best Regards,
Petr

On 2022-09-29, Petr Mladek <pmladek@suse.com> wrote:
> It is the legacy mode that tries to print to the consoles immediately.
> I am not sure if we could _ever_ remove this mode.

We are not trying to remove this mode. We are trying to introduce a new
mode. Once all the console drivers have moved over to the new mode, the
old mode can disappear. If some console drivers never move over, the old
mode can hang around.

It is important to understand that we are not trying to change the old
mode. This was our big mistake leading to the 5.19-revert.

> And it is most likely the main reason why semaphore is used instead
> of a mutex:
>
>     + printk() can be called in atomic context
>
>     + also there is the console_trylock_spinning() trick that allows
>       to transfer the semaphore to another owner without locking.
>
> Do you see any RT-friendly solution for the legacy mode, please?

No. Legacy mode will never work for RT because the console drivers are
using spinlocks, which for RT requires that preemption is enabled.

> Anyway, some lock will still be needed to synchronize the list.
> But could it be mutex? What about the legacy mode of printk_emit()?

For list updates a mutex is fine. All list updates already require
may_sleep contexts. For just iterating the list, SRCU is fine.

But we really need an atomic variable (or separate data-race bools) for
the properties that are not immutable. AFAIK this is only CON_ENABLED
and CON_CONSDEV (and I seriously question the usefulness/correctness of
CON_CONSDEV). If console_is_enabled() could be safely called without a
lock, neither console_lock nor console_list_lock would be needed to
safely iterate and act on the console list.

The NOBKL consoles (not included in this series) use a separate atomic
state variable to handle this. Perhaps the legacy consoles could
(mis)use that variable so that CON_ENABLED is atomic for them as well.

>> Yes! One of the main points of this final phase of the rework is to
>> remove console_sem usage (for NOBKL consoles). If a system is running
>> with only NOBKL consoles registered, ideally that system should never
>> call console_lock()/console_trylock(). Once all drivers have
>> converted over to the NOBKL interface, console_sem will serve no
>> purpose for the printk and console frameworks, so it can be removed.
>
> And even if we convert all console drivers then people still might
> want the legacy mode.

For converted drivers there is no use for the pseudo-synchronous legacy
mode. Converted drivers can run in true synchronous mode if the user
wants.

> My understanding is that some atomic consoles would be real hacks.

Well, it is up to the maintainers to make sure they are not real
hacks. We are not mandating that all drivers are converted. But I think
when devs start seeing the benefits of the converted drivers (and will
have many working examples to be inspired by) there will be honest
efforts to correctly convert the driver.

> They might be good enough for panic(). But what about running system.
> It seems that people might want the legacy more even on running
> system. Will it be doable with mutex?

I'm not sure what you mean here, but I think you are referencing
situations that are not valid. Either drivers are legacy (and continue
using the BKL) or they are correctly converted to the new atomic/thread
model (and have nothing to do with the BKL).

There will be some exceptions (such as fbdev), which is why we are also
considering special alternatives for this class of drivers (such as BSoD
splash on panic, rather than an atomic console).

> I would really like to avoid state where we have two locks (semaphore
> and mutex) serializing the same thing (console list).

I understand. I will look into this more closely. But it may just mean
adding comments above each console_lock() to say:

1. that it is being using to stop all console printing

2. why all console printing needs to stop

Notice that the above list does not include "provide synchronization for
the console list".

John

On Thu 2022-09-29 17:43:17, Petr Mladek wrote:
> On Thu 2022-09-29 01:48:29, John Ogness wrote:
> > On 2022-09-27, Petr Mladek <pmladek@suse.com> wrote:
> > > Hmm, the new mutex is really nasty. It has very strange semantic.
> > > It makes the locking even more complicated.
> > 
> > We are working to replace the BKL-console_lock with new separate clearly
> > defined mechanisms.
> >
> > The new mutex provides full synchronization for list changes as well as
> > changes to items of that list. (Really console->flags is the only change
> > to items of the list.)

We should actually make the the reading of console->flags safe under
srcu_read_lock(). It would allow to use the SRCU walk by all the
readers.

> > For some places in the code it is very clear that the console_lock can
> > be completely replaced (either with srcu or the new mutex). For other
> > places, it is not yet clear why the console_lock is being used and so
> > both console_lock and mutex are used.
> 
> One important and tricky location is console_trylock() in
> vprintk_emit(). And the related for_each_console() called from
> console_unlock()->console_flush_all().
> 
> It is the legacy mode that tries to print to the consoles immediately.
> I am not sure if we could _ever_ remove this mode.
> 
> I would really like to avoid state where we have two locks (semaphore
> and mutex) serializing the same thing (console list).

That said, I could imagine implementing console_lock() so that it
would be implemented by mutex when the legacy mode is disabled and
semaphore when it is allowed.

You were talking about command-line option that would allow to
disable the legacy mode on production RT systems. And I guess
that you added mutex because it behaves better on RT.

Also I could imagine using console_list_lock() as a wrapper
to console_lock(). It might help to distinguish locations where
the list is traversed and where the console_lock() is used for
another reason. I mean to remove the big-kernel-lock character
of the console_lock().

You know, the more locks we have, the bigger is the risk of
deadlocks, and the more hacks would be needed in
console_flush_on_panic(). And I am afraid
that console_lock() will be with us for many years and
maybe forever.

Does it make any sense, please?

Best Regards,
Petr

On 2022-09-30, Petr Mladek <pmladek@suse.com> wrote:
> We should actually make the the reading of console->flags safe under
> srcu_read_lock(). It would allow to use the SRCU walk by all the
> readers.

Agreed. I will do this for the next version.

> That said, I could imagine implementing console_lock() so that it
> would be implemented by mutex when the legacy mode is disabled and
> semaphore when it is allowed.

No, let's not imagine this. It is déjà vu for the code that was
reverted.

> You were talking about command-line option that would allow to
> disable the legacy mode on production RT systems. And I guess
> that you added mutex because it behaves better on RT.

We added mutex because list updates are always in may_sleep context and
we were moving to SRCU for list iteration. I think with v2, where SRCU
will be introduced earlier, things will be much clearer.

> Also I could imagine using console_list_lock() as a wrapper
> to console_lock(). It might help to distinguish locations where
> the list is traversed and where the console_lock() is used for
> another reason. I mean to remove the big-kernel-lock character
> of the console_lock().

No, locking the list should have nothing to do with console_lock(). We
want to remove the list synchronization responsibilities from
console_lock(). In this series, I did not make that clear in the commit
messages. (Perhaps it was not entirely clear to me then.) For v2 I will
make this point very clear.

> You know, the more locks we have, the bigger is the risk of
> deadlocks, and the more hacks would be needed in
> console_flush_on_panic(). And I am afraid
> that console_lock() will be with us for many years and
> maybe forever.

Sure. Removing console_lock() will be a long battle involving many
drivers. I am not trying to fight that battle right now. I just want
console_lock() out of the way of NOBKL consoles.

John

On Fri 2022-09-30 16:22:30, John Ogness wrote:
> On 2022-09-30, Petr Mladek <pmladek@suse.com> wrote:
> > You know, the more locks we have, the bigger is the risk of
> > deadlocks, and the more hacks would be needed in
> > console_flush_on_panic(). And I am afraid
> > that console_lock() will be with us for many years and
> > maybe forever.
> 
> Sure. Removing console_lock() will be a long battle involving many
> drivers. I am not trying to fight that battle right now. I just want
> console_lock() out of the way of NOBKL consoles.

There is some misunderstanding. I am going to think more about your
arguments over the weekend.

But maybe, the above is important. You want to get cosole_lock() out
of the way of NOBLK consoles. What does it exactly mean, please?
What code paths are important to achieve this?

From my POV, the most important code path is the kthread. But it
should use SRCU. I mean that the kthread will take neither
cosnole_lock() nor console_list_lock().

Is there any other code path where console_list_lock() will help
you to get console_lock() out of the way?


From my POV, the proposed code does:

register_console()
{
	console_list_lock();
	console_lock();

	/* manipulate struct console and the console_list */

	console_unlock();
	console_list_unlock();
}

register_console()
{
	console_list_lock();
	console_lock();

	/* manipulate struct console and the console_list */

	console_unlock();
	console_list_unlock();
}

printk_kthread()
{
	while() {
		srcu_read_lock();

		if (read_flags_srcu())
		     /* print line */

		srcu_read_unlock();
	}
}

vprintk_emit()
{
	/* store message */

	if (do_not_allow_sync_mode)
		return;

	if (console_trylock()) {
		console_flush_all();
		__console_unlock();
	}
}

some_other_func()
{
	console_list_lock();
	/* do something with all registered consoles */
	console_list_unlock();
}

console_flush_all()
{
	do_something_with_all_consoles();
	do_something_else_with_all_consoles();
}

What if?

do_something_with_all_consoles()
{
	console_list_lock();
	/* do something */
	console_list_unlock();
}

Wait, there is a possible ABBA deadlock because
do_something_with_all_consoles() takes console_list_lock()
under console_lock(). And register_console() does it
the other way around.

But it is less obvious because these are different locks.

From my POV, both locks serialize the same things
(console_list manipulation). SRCU walk should be
enough for most iterations over the list.

And I do not see which code path would really benefit from
having the new console_list_lock() instead of console_lock().

Best Regards,
Petr

On 2022-09-30, Petr Mladek <pmladek@suse.com> wrote:
> You want to get cosole_lock() out of the way of NOBKL consoles. What
> does it exactly mean, please?

It means that a system with only NOBKL consoles will never take the
console_lock.

> What code paths are important to achieve this?

Anything that iterates or queries the consoles is taking the
console_lock right now. We want that code to use something else for
those tasks (console_srcu, console_mutex, atomic_state).

> From my POV, the most important code path is the kthread. But
> it should use SRCU. I mean that the kthread will take neither
> cosnole_lock() nor console_list_lock().

All iterators and querying are important because they need a safe
interface that does not rely on console_lock.

> Is there any other code path where console_list_lock() will help
> you to get console_lock() out of the way?

The difficulty arises because we are trying to share as much code as
possible. So, for example, NOBKL consoles are sitting on the same list
as legacy consoles. And since currently the list is protected by the
console_lock, we need to change how that list is protected.

> From my POV, both locks serialize the same things
> (console_list manipulation).

My v2 will hopefully change your POV. I will make it clear (in comments
and implementation) that the console_lock does _not_ protect the console
list. All iteration and querying will have no choice but to use the new
mechanisms for list iteration and checking/setting CON_ENABLED.

Then the console_lock's only function is to block legacy consoles from
printing and making sure that multiple legacy consoles are not printing
in parallel. And, of course, it will still function as a general BKL
lock for fbdev, which may be relying on its locking function to
synchronize some fbdev data.

Note that the end result will be no change in behavior for legacy
consoles. But it allows legacy and NOBKL consoles to run simultaneously
while sharing significant amounts of code, and provides a clear path for
console drivers to begin converting. As a side-effect, the first step of
reducing the scope of the console_lock will have been taken.

John

On Fri 2022-09-30 22:32:32, John Ogness wrote:
> On 2022-09-30, Petr Mladek <pmladek@suse.com> wrote:
> > You want to get cosole_lock() out of the way of NOBKL consoles. What
> > does it exactly mean, please?
> 
> It means that a system with only NOBKL consoles will never take the
> console_lock.

What is exactly wrong with console_lock, please?

Is the main problem that it is a semaphore?

Or is it a problem that it is used in some console
drivers for other purposes?

My view:

If you use only NOBLK consoles then you should never take
console_lock via con->write(). Also the printk kthread
main-loop does not need to take console_lock.

If the above is true then console_lock should be needed
only by register_console() and unregister_console(). Anything
else should be doable via srcu_read_lock.

Is it a problem when console_lock is needed in register_console
and unregister_console on RT?


> > What code paths are important to achieve this?
> 
> Anything that iterates or queries the consoles is taking the
> console_lock right now. We want that code to use something else for
> those tasks (console_srcu, console_mutex, atomic_state).

IMHO, console_srcu should be enough for any query.

And even when the write lock was needed from some
reasons why mutex is needed? Is semaphore completely
unacceptable on RT? Do you want to avoid semaphore on RT
at any cost?


> My v2 will hopefully change your POV. I will make it clear (in comments
> and implementation) that the console_lock does _not_ protect the console
> list. All iteration and querying will have no choice but to use the new
> mechanisms for list iteration and checking/setting CON_ENABLED.

Before you spend too much time on it then please try to solve
the problem below.

> Then the console_lock's only function is to block legacy consoles from
> printing and making sure that multiple legacy consoles are not printing
> in parallel. And, of course, it will still function as a general BKL
> lock for fbdev, which may be relying on its locking function to
> synchronize some fbdev data.
> 
> Note that the end result will be no change in behavior for legacy
> consoles. But it allows legacy and NOBKL consoles to run simultaneously
> while sharing significant amounts of code, and provides a clear path for
> console drivers to begin converting. As a side-effect, the first step of
> reducing the scope of the console_lock will have been taken.

OK, there are people that want to disable kthreads by some command line
option. There is a non-trivial possibility that this "feature" will
be there forever.

How exactly do you want to support this legacy mode, please?

The above proposal suggests that it might be something like:

register_console()
{
	console_list_lock();

	if (!need_console())
		goto out;

	if (!try_enable_console())
		goto out;

	if (!(con->flags & CON_NOBLK))
		console_lock()

	add_console_into_the_list();

	if (!(con->flags & CON_NOBLK))
		console_unlock()

out:
	console_list_unlock();
}


vprintk_emit()
{
	vprintk_store();

	wake_up_klogd();

	if (only_noblk_consoles || in_sched)
		return;

	if (console_trylock()) {
		console_flush_all();
		__console_unlock();
}


console_flush_all()
{
	/*
	 * !!! WARNING !!!
	 *  Must take srcu_read_lock(&console_src) here.
	 *  Must never take console_list_lock() here.
	 */
	srcu_read_lock(&console_srcu);

	for_each_console() {
		...
	}

	srcu_read_unlock(&console_srcu);
}

The srcu_read_lock() is needed because NOBKL consoles are
added into the list without console_lock().

There are actually two reasons why we could not take
console_list_lock() in console_flush_all():

    + it is a sleeping lock and vprintk_emit() might
      be called in atomic context

    + it might cause ABBA deadlock with console_lock


IMHO, this is not obvious. The rules for using the three global
locks (console_lock(), console_list_lock(), console_srcu)
look quite complicated to me.

Anyway, are you able to implement vprintk_emit()/console_flush_all()
without console_lock()?

If we need to keep console_trylock() in vprintk_emit() forever
then we really need a good justification why console_list_lock()
is needed.

Please, show me a code path where console_mutex is needed
as the only acceptable solution for RT.

Best Regards,
Petr

On 2022-10-03, Petr Mladek <pmladek@suse.com> wrote:
> What is exactly wrong with console_lock, please?

It is ambiguously performing multiple tasks:

- protecting the console list
- protecting individual console fields
- serializing console printing
- stopping all console printing

That last item is actually quite complex because nobody really knows
_why_ all consoles need to be stopped. It is mostly because fbdev is
using the console_lock to protect itself from its own write()
callback. But (as has been mentioned in this thread) there are other
code sites where we are not sure which part of the above tasks it is
used for and why.

> Is the main problem that it is a semaphore?

A semaphore has been needed because we are performing global locking for
ambiguous reasons in all possible contexts. We should be using
fine-grained lock and synchronization mechanisms that are appropriate
for their used contexts to precisely lock/synchronize exactly what needs
to be locked/synchronized.

Your first question is literally, "what is wrong with a BKL".

And the answer to that is: A BKL is preventing us from optimizing the
kernel by decoupling unrelated activities.

> The above proposal suggests that it might be something like:
>
> register_console()
> {
> 	console_list_lock();
>
> 	if (!need_console())
> 		goto out;
>
> 	if (!try_enable_console())
> 		goto out;
>
> 	if (!(con->flags & CON_NOBLK))
> 		console_lock()

Why are you taking the console_lock here? The console_list_lock needs to
replace this responsibility. I realize the RFC and this v1 series does
not do this. For v2, it will be clear.

> 	add_console_into_the_list();
>
> 	if (!(con->flags & CON_NOBLK))
> 		console_unlock()

I would request that you continue reviewing the later patches in the
series. Particularly 13-18. My v2 will involve a significantly reworked
version of patches 6-12, not only changing the order of presentation,
but also explicitly removing console list update protection by the
console_lock. I think having actual code to discuss will greatly help us
continue this discussion.

Patches 13-18 will not change much for v2, unless I get some feedback
otherwise.

John

On Mon 2022-10-03 21:41:22, John Ogness wrote:
> On 2022-10-03, Petr Mladek <pmladek@suse.com> wrote:
> > What is exactly wrong with console_lock, please?
> 
> It is ambiguously performing multiple tasks:
> 
> - protecting the console list
> - protecting individual console fields
> - serializing console printing
> - stopping all console printing
> 
> And the answer to that is: A BKL is preventing us from optimizing the
> kernel by decoupling unrelated activities.
>
> > The above proposal suggests that it might be something like:
> >
> > register_console()
> > {
> > 	console_list_lock();
> >
> > 	if (!need_console())
> > 		goto out;
> >
> > 	if (!try_enable_console())
> > 		goto out;
> >
> > 	if (!(con->flags & CON_NOBLK))
> > 		console_lock()
> 
> Why are you taking the console_lock here? The console_list_lock needs to
> replace this responsibility. I realize the RFC and this v1 series does
> not do this. For v2, it will be clear.

This is the important information that I missed. It is a great idea.
I agree that console_list_lock() would be a step forward if this worked.

As you say, in the RFC and this v1, console_lock() was still used
to synchronize the list and the metadata manipulation. It means that
console_lock() was as complex as before. In fact, it was even
more complex because console_list_lock() appeared in its lock
dependency chains. And it was not clear that v2 would be
any different in this regard.

Best Regards,
Petr

On (22/10/03 21:41), John Ogness wrote:
> A semaphore has been needed because we are performing global locking for
> ambiguous reasons in all possible contexts. We should be using
> fine-grained lock and synchronization mechanisms that are appropriate
> for their used contexts to precisely lock/synchronize exactly what needs
> to be locked/synchronized.
> 
> Your first question is literally, "what is wrong with a BKL".
> 
> And the answer to that is: A BKL is preventing us from optimizing the
> kernel by decoupling unrelated activities.
> 
> > The above proposal suggests that it might be something like:
> >
> > register_console()
> > {
> > 	console_list_lock();
> >
> > 	if (!need_console())
> > 		goto out;
> >
> > 	if (!try_enable_console())
> > 		goto out;
> >
> > 	if (!(con->flags & CON_NOBLK))
> > 		console_lock()
> 
> Why are you taking the console_lock here? The console_list_lock needs to
> replace this responsibility. I realize the RFC and this v1 series does
> not do this. For v2, it will be clear.

So tty/VT code also needs to take list_lock? list_lock does not look
precisely relevant to vt, which has it's own "list" of "struct vc" to
maintain.

On (22/09/27 17:16), Petr Mladek wrote:
[..]
> > +static int console_unregister_locked(struct console *console);
> > +
> >  /*
> >   * The console driver calls this routine during kernel initialization
> >   * to register the console printing procedure with printk() and to
> > @@ -3107,13 +3148,14 @@ void register_console(struct console *newcon)
> >  	bool realcon_enabled = false;
> >  	int err;
> >  
> > -	for_each_console(con) {
> > +	console_list_lock();
> 
> Hmm, the new mutex is really nasty. It has very strange semantic.
> It makes the locking even more complicated.

[..]

I fully agree with everything you said. This lock nesting made me
scratch my head wondering was it previous CPU hotplug code that had
multiple nested locks or was it something else?

> Anyway, I would like to avoid adding console_mutex. From my POV,
> it is a hack that complicates the code. Taking console_lock()
> should be enough. Using rcu walk would be good enough.
> 
> Do I miss something, please?
> 
> Or is this part of some strategy to remove console_sem later, please?

So I can only explain what potential I saw in list lock: the idea
that third party that iterates over consoles lists does not stop
entire console output machinery, and, moreover, that third party
does not flush pending messages once it's done with the business
it had to do under console_sem. E.g. it can be a systemd or any
other user-space process doing something with /dev/tty, which can
suddenly stop all consoles output (console_lock()) and then also
has to flush pending kernel messages (console_unlock()). Was this
goal, however, fully achieved - no, a third party that wants to
->flags &= ~CON_ENABLED a particular console still stops the entire
console output (and flushes pending messages, unless handover-ed).

I like what you suggested with srcu.

On (22/09/24 02:10), John Ogness wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> 
> Unprotected list walks are a brilliant idea. Especially in the context of
> hotpluggable consoles.
> 
> The new list lock provides not only synchronization for console list
> manipulation, but also for manipulation of console->flags:
> 
>     console_list_lock();
>     console_lock();
> 
>     /* may now manipulate the console list and/or console->flags */
> 
>     console_unlock();
>     console_list_unlock();
> 
> Therefore it is safe to iterate the console list and read console->flags
> if holding either the console lock or the console list lock.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>