diff mbox series

[printk,v3,39/40] printk: relieve console_lock of list synchronization duties

Message ID 20221107141638.3790965-40-john.ogness@linutronix.de
State New
Headers
Series reduce console_lock scope |

Commit Message

John Ogness Nov. 7, 2022, 2:16 p.m. UTC 
  The console_list_lock provides synchronization for console list and
console->flags updates. All call sites that were using the console_lock
for this synchronization have either switched to use the
console_list_lock or the SRCU list iterator.

Remove console_lock usage for console list updates and console->flags
updates.

Signed-off-by: John Ogness <john.ogness@linutronix.de>
---
 kernel/printk/printk.c | 36 ++++++++++++------------------------
 1 file changed, 12 insertions(+), 24 deletions(-)

Comments

John Ogness Nov. 7, 2022, 4:30 p.m. UTC | #1 
On 2022-11-07, John Ogness <john.ogness@linutronix.de> wrote:
> @@ -3344,7 +3340,6 @@ void register_console(struct console *newcon)
>  	 * Put this console in the list - keep the
>  	 * preferred driver at the head of the list.
>  	 */
> -	console_lock();
>  	if (hlist_empty(&console_list)) {
>  		/* Ensure CON_CONSDEV is always set for the head. */
>  		newcon->flags |= CON_CONSDEV;
> @@ -3358,7 +3353,6 @@ void register_console(struct console *newcon)
>  	} else {
>  		hlist_add_behind_rcu(&newcon->node, console_list.first);
>  	}
> -	console_unlock();
>  
>  	/*
>  	 * No need to synchronize SRCU here! The caller does not rely

I just realized that because of the new @seq initialization (patch 5/40)
that we cannot completely remove the console_lock from
register_console(). It will still be needed for @seq synchronization
when registering non-boot/non-printbuffer consoles. So something like
the patch below will need to be folded into this one.

I am not happy with this. If an enabled boot console is behind, the
console_unlock() will probably catch it up and we will end up with some
repeat messages. But maybe this is "good enough" until we implement some
real coordination between boot console and normal console takeovers.

John Ogness

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 17765166ac42..bb119001df56 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -3328,12 +3328,21 @@ void register_console(struct console *newcon)
 		 * that message instead. That boot console will be
 		 * unregistered shortly and may be the same device.
 		 */
+
+		/*
+		 * Hold the console_lock to guarantee safe access to
+		 * console->seq.
+		 */
+		console_lock();
+
 		for_each_console(con) {
 			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
 			    con->seq < newcon->seq) {
 				newcon->seq = con->seq;
 			}
 		}
+
+		console_unlock();
 	}
 
 	/*
Petr Mladek Nov. 11, 2022, 10:27 a.m. UTC | #2 
On Mon 2022-11-07 17:36:48, John Ogness wrote:
> On 2022-11-07, John Ogness <john.ogness@linutronix.de> wrote:
> > @@ -3344,7 +3340,6 @@ void register_console(struct console *newcon)
> >  	 * Put this console in the list - keep the
> >  	 * preferred driver at the head of the list.
> >  	 */
> > -	console_lock();
> >  	if (hlist_empty(&console_list)) {
> >  		/* Ensure CON_CONSDEV is always set for the head. */
> >  		newcon->flags |= CON_CONSDEV;
> > @@ -3358,7 +3353,6 @@ void register_console(struct console *newcon)
> >  	} else {
> >  		hlist_add_behind_rcu(&newcon->node, console_list.first);
> >  	}
> > -	console_unlock();
> >  
> >  	/*
> >  	 * No need to synchronize SRCU here! The caller does not rely
> 
> I just realized that because of the new @seq initialization (patch 5/40)
> that we cannot completely remove the console_lock from
> register_console(). It will still be needed for @seq synchronization
> when registering non-boot/non-printbuffer consoles. So something like
> the patch below will need to be folded into this one.

Great catch!

> I am not happy with this. If an enabled boot console is behind, the
> console_unlock() will probably catch it up and we will end up with some
> repeat messages. But maybe this is "good enough" until we implement some
> real coordination between boot console and normal console takeovers.

The same problem actually has been there even before. The new console
was added in console_list under console_lock(). console_unlock() was
called before the early consoles were unregistered.

A solution would be to call pr_flush() before. But it should be
done in a separate patch.

> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 17765166ac42..bb119001df56 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3328,12 +3328,21 @@ void register_console(struct console *newcon)
>  		 * that message instead. That boot console will be
>  		 * unregistered shortly and may be the same device.
>  		 */
> +
> +		/*
> +		 * Hold the console_lock to guarantee safe access to
> +		 * console->seq.
> +		 */
> +		console_lock();
> +
>  		for_each_console(con) {
>  			if ((con->flags & (CON_BOOT | CON_ENABLED)) == (CON_BOOT | CON_ENABLED) &&
>  			    con->seq < newcon->seq) {
>  				newcon->seq = con->seq;
>  			}
>  		}
> +
> +		console_unlock();
>  	}

This should be added already into the 5th patch that added this cycle.
We just must keep it in this patch.

Best Regards,
Petr
Petr Mladek Nov. 11, 2022, 1:06 p.m. UTC | #3 
On Mon 2022-11-07 15:22:37, John Ogness wrote:
> The console_list_lock provides synchronization for console list and
> console->flags updates. All call sites that were using the console_lock
> for this synchronization have either switched to use the
> console_list_lock or the SRCU list iterator.
> 
> Remove console_lock usage for console list updates and console->flags
> updates.
> 
> Signed-off-by: John Ogness <john.ogness@linutronix.de>

All the accesses to console->flags and all the console_list walks
looks safe, so:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr
diff mbox series

Patch

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index d74e6e609f7d..17765166ac42 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -86,8 +86,8 @@  EXPORT_SYMBOL(oops_in_progress);
 static DEFINE_MUTEX(console_mutex);
 
 /*
- * console_sem protects console_list and console->flags updates, and also
- * provides serialization for access to the entire console driver system.
+ * console_sem protects updates to console->seq and console_suspended,
+ * and also provides serialization for console printing.
  */
 static DEFINE_SEMAPHORE(console_sem);
 HLIST_HEAD(console_list);
@@ -2638,10 +2638,10 @@  static int console_cpu_notify(unsigned int cpu)
 }
 
 /**
- * console_lock - lock the console system for exclusive use.
+ * console_lock - block the console subsystem from printing
  *
- * Acquires a lock which guarantees that the caller has
- * exclusive access to the console system and console_list.
+ * Acquires a lock which guarantees that no consoles will
+ * be in or enter their write() callback.
  *
  * Can sleep, returns nothing.
  */
@@ -2658,10 +2658,10 @@  void console_lock(void)
 EXPORT_SYMBOL(console_lock);
 
 /**
- * console_trylock - try to lock the console system for exclusive use.
+ * console_trylock - try to block the console subsystem from printing
  *
- * Try to acquire a lock which guarantees that the caller has exclusive
- * access to the console system and console_list.
+ * Try to acquire a lock which guarantees that no consoles will
+ * be in or enter their write() callback.
  *
  * returns 1 on success, and 0 on failure to acquire the lock.
  */
@@ -2917,10 +2917,10 @@  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 }
 
 /**
- * console_unlock - unlock the console system
+ * console_unlock - unblock the console subsystem from printing
  *
- * Releases the console_lock which the caller holds on the console system
- * and the console driver list.
+ * Releases the console_lock which the caller holds to block printing of
+ * the console subsystem.
  *
  * While the console_lock was held, console output may have been buffered
  * by printk().  If this is the case, console_unlock(); emits
@@ -3107,9 +3107,7 @@  void console_stop(struct console *console)
 {
 	__pr_flush(console, 1000, true);
 	console_list_lock();
-	console_lock();
 	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
-	console_unlock();
 	console_list_unlock();
 
 	/*
@@ -3125,9 +3123,7 @@  EXPORT_SYMBOL(console_stop);
 void console_start(struct console *console)
 {
 	console_list_lock();
-	console_lock();
 	WRITE_ONCE(console->flags, console->flags | CON_ENABLED);
-	console_unlock();
 	console_list_unlock();
 	__pr_flush(console, 1000, true);
 }
@@ -3344,7 +3340,6 @@  void register_console(struct console *newcon)
 	 * Put this console in the list - keep the
 	 * preferred driver at the head of the list.
 	 */
-	console_lock();
 	if (hlist_empty(&console_list)) {
 		/* Ensure CON_CONSDEV is always set for the head. */
 		newcon->flags |= CON_CONSDEV;
@@ -3358,7 +3353,6 @@  void register_console(struct console *newcon)
 	} else {
 		hlist_add_behind_rcu(&newcon->node, console_list.first);
 	}
-	console_unlock();
 
 	/*
 	 * No need to synchronize SRCU here! The caller does not rely
@@ -3406,15 +3400,11 @@  static int unregister_console_locked(struct console *console)
 	if (res > 0)
 		return 0;
 
-	console_lock();
-
 	/* Disable it unconditionally */
 	WRITE_ONCE(console->flags, console->flags & ~CON_ENABLED);
 
-	if (!console_is_registered_locked(console)) {
-		console_unlock();
+	if (!console_is_registered_locked(console))
 		return -ENODEV;
-	}
 
 	hlist_del_init_rcu(&console->node);
 
@@ -3430,8 +3420,6 @@  static int unregister_console_locked(struct console *console)
 	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
 		WRITE_ONCE(console_first()->flags, console_first()->flags | CON_CONSDEV);
 
-	console_unlock();
-
 	/*
 	 * Ensure that all SRCU list walks have completed. All contexts
 	 * must not be able to see this console in the list so that any