diff mbox series

[printk,v2,37/38] printk: relieve console_lock of list synchronization duties

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

Commit Message

John Ogness Oct. 19, 2022, 2:55 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 | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

Comments

Petr Mladek Oct. 27, 2022, 12:40 p.m. UTC | #1 
On Wed 2022-10-19 17:01:59, 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.
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 3615ee6d68fd..840d581c4b23 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -3370,8 +3363,6 @@ static int unregister_console_locked(struct console *console)
>  	if (res > 0)
>  		return 0;
>  
> -	console_lock();
> -
>  	/* Disable it unconditionally */
>  	console->flags &= ~CON_ENABLED;
>  
> @@ -3394,8 +3385,6 @@ static int unregister_console_locked(struct console *console)
>  	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
>  		console_first()->flags |= CON_CONSDEV;
>  
> -	console_unlock();
> -
>  	/* Ensure that all SRCU list walks have completed */
>  	synchronize_srcu(&console_srcu);

In addition, in unregister_console_locked(), we need to remove goto out_unlock:

	if (hlist_unhashed(&console->node)) {
		res = -ENODEV;
		goto out_unlock;
	}

out_unlock:
	console_unlock();
	return res;


I did my best to check all users that either iterate over the list
and/or access console->flags. And I did not find any real problems.

With the removal of the above mentioned out_unlock:

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


Best Regards,
Petr

PS: Two ideas for possible future improvement:

  1. It might make sense to synchronize also @console_cmdline using
     the console_list_lock. It is modified only in
     __add_preferred_console() and read only from register_console().

     It would remove another possible race. Well, it is probably hard
     to hit, especially when new entries are always added at the end.


  2. It might make sense to create an API to modify and check
     console->flags. It might print a warning when the flags
     are accessed without console_list_lock or console_srcu.

     These days, it is pretty hard to catch all users.
diff mbox series

Patch

diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
index 3615ee6d68fd..840d581c4b23 100644
--- a/kernel/printk/printk.c
+++ b/kernel/printk/printk.c
@@ -82,8 +82,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);
@@ -2635,7 +2635,7 @@  static int console_cpu_notify(unsigned int cpu)
  * console_lock - lock the console system for exclusive use.
  *
  * Acquires a lock which guarantees that the caller has
- * exclusive access to the console system and console_list.
+ * exclusive access to the console system.
  *
  * Can sleep, returns nothing.
  */
@@ -2655,7 +2655,7 @@  EXPORT_SYMBOL(console_lock);
  * console_trylock - try to lock the console system for exclusive use.
  *
  * Try to acquire a lock which guarantees that the caller has exclusive
- * access to the console system and console_list.
+ * access to the console system.
  *
  * returns 1 on success, and 0 on failure to acquire the lock.
  */
@@ -2911,8 +2911,7 @@  static bool console_flush_all(bool do_cond_resched, u64 *next_seq, bool *handove
 /**
  * console_unlock - unlock the console system
  *
- * 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 on the console system.
  *
  * While the console_lock was held, console output may have been buffered
  * by printk().  If this is the case, console_unlock(); emits
@@ -3098,9 +3097,7 @@  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();
 
 	/* Ensure that all SRCU list walks have completed */
@@ -3111,9 +3108,7 @@  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);
 }
@@ -3315,7 +3310,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;
@@ -3329,7 +3323,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! */
 
@@ -3370,8 +3363,6 @@  static int unregister_console_locked(struct console *console)
 	if (res > 0)
 		return 0;
 
-	console_lock();
-
 	/* Disable it unconditionally */
 	console->flags &= ~CON_ENABLED;
 
@@ -3394,8 +3385,6 @@  static int unregister_console_locked(struct console *console)
 	if (!hlist_empty(&console_list) && console->flags & CON_CONSDEV)
 		console_first()->flags |= CON_CONSDEV;
 
-	console_unlock();
-
 	/* Ensure that all SRCU list walks have completed */
 	synchronize_srcu(&console_srcu);