On Wed 2022-04-20 01:52:36, John Ogness wrote:
> Currently threaded console printers synchronize against each
> other using console_lock(). However, different console drivers
> are unrelated and do not require any synchronization between
> each other. Removing the synchronization between the threaded
> console printers will allow each console to print at its own
> speed.
>
> But the threaded consoles printers do still need to synchronize
> against console_lock() callers. Introduce a per-console mutex
> and a new console flag CON_THD_BLOCKED to provide this
> synchronization.
>
> console_lock() is modified so that it must acquire the mutex
> of each console in order to set the CON_THD_BLOCKED flag.
> Console printing threads will acquire their mutex while
> printing a record. If CON_THD_BLOCKED was set, the thread will
> go back to sleep instead of printing.
>
> The reason for the CON_THD_BLOCKED flag is so that
> console_lock() callers do not need to acquire multiple console
> mutexes simultaneously, which would introduce unnecessary
> complexity due to nested mutex locking.
>
> The per-console mutex is also used to synchronize setting and
> checking the CON_ENABLED flag. A new console_lock() variant is
> introduced, console_lock_single_hold(), that allows acquiring
> @console_sem but only locking (and holding) the mutex of a
> single console. This allows safely enabling and disabling
> consoles without disturbing the other kthread printers. The
> mutex and @console_sem are released with
> console_unlock_single_release().
>
> Console unregistering now uses console_lock_single_hold() to
> stop the kthread. Thus con->thread is now synchronized by
> the per-console mutex. This allows consoles to be unregistered
> without disturbing the other kthread printers.
>
> Threaded console printers also need to synchronize against
> console_trylock() callers. Since console_trylock() may be
> called from any context, the per-console mutex cannot be used
> for this synchronization. (mutex_trylock() cannot be called
> from atomic contexts.) Introduce a global atomic counter to
> identify if any threaded printers are active. The threaded
> printers will also check the atomic counter to identify if the
> console has been locked by another task via console_trylock().
>
> Note that @console_sem is still used to provide synchronization
> between console_lock() and console_trylock() callers.
>
> A locking overview for console_lock(), console_trylock(), and the
> threaded printers is as follows (pseudo code):
>
> console_lock()
> {
> down(&console_sem);
> for_each_console(con) {
> mutex_lock(&con->lock);
> con->flags |= CON_THD_BLOCKED;
> mutex_unlock(&con->lock);
> }
> /* console_lock acquired */
> }
>
> console_trylock()
> {
> if (down_trylock(&console_sem) == 0) {
> if (atomic_cmpxchg(&console_kthreads_active, 0, -1) == 0) {
> /* console_lock acquired */
> }
> }
> }
>
> threaded_printer()
> {
> mutex_lock(&con->lock);
> if (!(con->flags & CON_THD_BLOCKED)) {
> /* console_lock() callers blocked */
>
> if (atomic_inc_unless_negative(&console_kthreads_active)) {
> /* console_trylock() callers blocked */
>
> con->write();
>
> atomic_dec(&console_lock_count);
> }
> }
> mutex_unlock(&con->lock);
> }
>
> The console owner and waiter logic now only applies between contexts
> that have taken the console_lock via console_trylock(). Threaded
> printers never take the console_lock, so they do not have a
> console_lock to handover. Tasks that have used console_lock() will
> block the threaded printers using a mutex and if the console_lock
> is handed over to an atomic context, it would be unable to unblock
> the threaded printers. However, the console_trylock() case is
> really the only scenario that is interesting for handovers anyway.
>
> @panic_console_dropped must change to atomic_t since it is no longer
> protected exclusively by the console_lock.
>
> Since threaded printers remain asleep if they see that the console
> is locked, they now must be explicitly woken in __console_unlock().
> This means wake_up_klogd() calls following a console_unlock() are
> no longer necessary and are removed.
>
> Also note that threaded printers no longer need to check
> @console_suspended. The check for the CON_THD_BLOCKED flag
> implicitly covers the suspended console case.
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -2603,9 +2666,10 @@ static int console_cpu_notify(unsigned int cpu)
> /* If trylock fails, someone else is doing the printing */
> if (console_trylock())
> console_unlock();
> -
> - /* Wake kthread printers. Some may have become usable. */
> - wake_up_klogd();
> + else {
> + /* Some kthread printers may have become usable. */
> + wake_up_klogd();
Do you have any particular scenario in mind, please?
Could CPU hotplug put any printk kthread into a sleep?
> + }
> }
> return 0;
> }
> @@ -2625,11 +2689,33 @@ void console_lock(void)
> down_console_sem();
> if (console_suspended)
> return;
> + console_kthreads_block();
> console_locked = 1;
> console_may_schedule = 1;
> }
> EXPORT_SYMBOL(console_lock);
>
> +/*
> + * Lock the console_lock, but rather than blocking all the kthread printers,
> + * lock a specified kthread printer and hold the lock. This is useful if
> + * console flags for a particular console need to be updated.
> + */
> +void console_lock_single_hold(struct console *con)
> +{
> + might_sleep();
> + down_console_sem();
> + mutex_lock(&con->lock);
> + console_locked = 1;
> + console_may_schedule = 1;
This looks wrong. It is a global flag that could be modified
only when all consoles are blocked. This API blocks only the single
console. The other consoles are still allowed to print actively.
Another problem will appear with the 15th patch. It will remove
console_locked variable and is_console_locked() will not longer
be aware that this console is locked. We will not know that
it might cause deadlock in the VT code.
> +}
> +
> +static void console_unlock_single_release(struct console *con)
> +{
> + console_locked = 0;
> + mutex_unlock(&con->lock);
> + up_console_sem();
> +}
> +
> /**
> * console_trylock - try to lock the console system for exclusive use.
> *
> @@ -2728,17 +2834,18 @@ static void __console_unlock(void)
> *
> * @handover will be set to true if a printk waiter has taken over the
> * console_lock, in which case the caller is no longer holding the
> - * console_lock. Otherwise it is set to false.
> + * console_lock. Otherwise it is set to false. A NULL pointer may be provided
> + * to disable allowing the console_lock to be taken over by a printk waiter.
> *
> * Returns false if the given console has no next record to print, otherwise
> * true.
> *
> - * Requires the console_lock.
> + * Requires the console_lock if @handover is non-NULL.
* Requires con->lock otherwise.
> */
> -static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
> - char *dropped_text, bool *handover)
> +static bool __console_emit_next_record(struct console *con, char *text, char *ext_text,
> + char *dropped_text, bool *handover)
> {
> - static int panic_console_dropped;
> + static atomic_t panic_console_dropped = ATOMIC_INIT(0);
> struct printk_info info;
> struct printk_record r;
> unsigned long flags;
> @@ -3261,6 +3401,8 @@ void register_console(struct console *newcon)
>
> newcon->dropped = 0;
> newcon->thread = NULL;
> + newcon->flags |= CON_THD_BLOCKED;
Just to show the complexity added by console_lock_single_hold():
It took me some time to realize that it is correct. The flag
is needed because the console will be added under console_lock().
The flag would not be needed when it was added under
console_lock_single_hold().
> + mutex_init(&newcon->lock);
>
> if (newcon->flags & CON_PRINTBUFFER) {
> /* Get a consistent copy of @syslog_seq. */
> @@ -3314,7 +3456,7 @@ int unregister_console(struct console *console)
> return 0;
>
> res = -ENODEV;
> - console_lock();
> + console_lock_single_hold(console);
> if (console_drivers == console) {
> console_drivers=console->next;
Another example of the complexity:
I though that this was not safe. console_drivers is a global list
and console_lock_single_hold() is supposed to block only a single
console. But it is actually safe because console_lock_single_hold()
holds console_sem.
Another question is why console_lock_single_hold() is enough
here and why console_lock() is used in register_console(). I think
that console_lock_single_hold() will be enough even in register_console().
All this is far from obvious. It shows how the API is confusing and
tricky. And it is another motivation to remove console_lock_single_hold().
> res = 0;
> @@ -3676,14 +3835,14 @@ static int printk_kthread_func(void *data)
> kfree(ext_text);
> kfree(text);
>
> - console_lock();
> + mutex_lock(&con->lock);
This is serialized against unregister_console() but not with
register_console() because they use different locking scheme.
> /*
> * If this kthread is being stopped by another task, con->thread will
> * already be NULL. That is fine. The important thing is that it is
> * NULL after the kthread exits.
> */
> con->thread = NULL;
> - console_unlock();
> + mutex_unlock(&con->lock);
>
> return 0;
> }
Resume:
I would prefer to get rid of console_lock_single_hold() and
console_unlock_single_release() API.
It was definitely an interesting experiment. I agree that it would
be nice to do not block the other kthreads when it is not really
needed. But from my POV, it adds more harm than good at the moment.
It is possible that we will want to do such optimizations
in the future. But it must be easier to understand what exactly
is serialized which way. At least it should be more documented.
Also the same API would need to be used on the related code
paths.
Best Regards,
Petr
On 2022-04-21, Petr Mladek <[email protected]> wrote:
>> --- a/kernel/printk/printk.c
>> +++ b/kernel/printk/printk.c
>> @@ -2603,9 +2666,10 @@ static int console_cpu_notify(unsigned int cpu)
>> /* If trylock fails, someone else is doing the printing */
>> if (console_trylock())
>> console_unlock();
>> -
>> - /* Wake kthread printers. Some may have become usable. */
>> - wake_up_klogd();
>> + else {
>> + /* Some kthread printers may have become usable. */
>> + wake_up_klogd();
>
> Do you have any particular scenario in mind, please?
> Could CPU hotplug put any printk kthread into a sleep?
I do not have a particular scenario. My reasoning was that a CPU coming
online would affect the conditions of __console_is_usable() for consoles
without CON_ANYTIME. Of course, it would mean that previously a kthread
went to sleep because it was trying to print from a CPU that was
offline. I am doubtful that such a scenario is possible. But you did
uncover some bizarre code paths where task migration could fail during
CPU offlining.
Anyway, you suggested to keep the CON_ANYTIME checks for kthreads in
there. So it seems correct to wake threads anytime the
printer_should_wake() conditions change.
>> @@ -2625,11 +2689,33 @@ void console_lock(void)
>> down_console_sem();
>> if (console_suspended)
>> return;
>> + console_kthreads_block();
>> console_locked = 1;
>> console_may_schedule = 1;
>> }
>> EXPORT_SYMBOL(console_lock);
>>
>> +/*
>> + * Lock the console_lock, but rather than blocking all the kthread printers,
>> + * lock a specified kthread printer and hold the lock. This is useful if
>> + * console flags for a particular console need to be updated.
>> + */
>> +void console_lock_single_hold(struct console *con)
>> +{
>> + might_sleep();
>> + down_console_sem();
>> + mutex_lock(&con->lock);
>> + console_locked = 1;
>> + console_may_schedule = 1;
>
> This looks wrong. It is a global flag that could be modified
> only when all consoles are blocked.
You are correct. is_console_locked() needs to return false in this
scenario. I will leave out the @console_lock setting and insert a
comment to clarify why.
> This API blocks only the single console. The other consoles are still
> allowed to print actively.
That is the point. VT does not care about the other printers. VT is
using @console_locked to protect itself against itself.
> Another problem will appear with the 15th patch. It will remove
> console_locked variable and is_console_locked() will not longer
> be aware that this console is locked. We will not know that
> it might cause deadlock in the VT code.
From the perspective of VT code the console is _not_ locked. So
is_console_locked() should return false. is_console_locked() is to make
sure that the _VT code_ has called console_lock()/console_trylock(). So
the 15th patch is still correct.
>> @@ -2728,17 +2834,18 @@ static void __console_unlock(void)
>> *
>> * @handover will be set to true if a printk waiter has taken over the
>> * console_lock, in which case the caller is no longer holding the
>> - * console_lock. Otherwise it is set to false.
>> + * console_lock. Otherwise it is set to false. A NULL pointer may be provided
>> + * to disable allowing the console_lock to be taken over by a printk waiter.
>> *
>> * Returns false if the given console has no next record to print, otherwise
>> * true.
>> *
>> - * Requires the console_lock.
>> + * Requires the console_lock if @handover is non-NULL.
>
> * Requires con->lock otherwise.
Right. I will update the comments.
>> */
>> -static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
>> - char *dropped_text, bool *handover)
>> +static bool __console_emit_next_record(struct console *con, char *text, char *ext_text,
>> + char *dropped_text, bool *handover)
>> {
>> - static int panic_console_dropped;
>> + static atomic_t panic_console_dropped = ATOMIC_INIT(0);
>> struct printk_info info;
>> struct printk_record r;
>> unsigned long flags;
>> @@ -3261,6 +3401,8 @@ void register_console(struct console *newcon)
>>
>> newcon->dropped = 0;
>> newcon->thread = NULL;
>> + newcon->flags |= CON_THD_BLOCKED;
>
> Just to show the complexity added by console_lock_single_hold():
>
> It took me some time to realize that it is correct. The flag
> is needed because the console will be added under console_lock().
> The flag would not be needed when it was added under
> console_lock_single_hold().
?? But it is not added under
console_lock_single_hold(). console_lock_single_hold() is not a
replacement for console_lock(). Their purpose is very
different. console_lock_single_hold() is an internal function to provide
synchronization for @flags and @thread updates of a single console.
Maybe we are getting caught in my "bad naming" trap again. :-/
I do not have any ideas for a function that:
- locks @console_sem to prevent console registration/deregistration
- locks con->lock to provide synchronized @flags and/or @thread updates
>> + mutex_init(&newcon->lock);
>>
>> if (newcon->flags & CON_PRINTBUFFER) {
>> /* Get a consistent copy of @syslog_seq. */
>> @@ -3314,7 +3456,7 @@ int unregister_console(struct console *console)
>> return 0;
>>
>> res = -ENODEV;
>> - console_lock();
>> + console_lock_single_hold(console);
>> if (console_drivers == console) {
>> console_drivers=console->next;
>
> Another example of the complexity:
>
> I though that this was not safe. console_drivers is a global list
> and console_lock_single_hold() is supposed to block only a single
> console. But it is actually safe because console_lock_single_hold()
> holds console_sem.
Yes. It is safe.
> Another question is why console_lock_single_hold() is enough
> here and why console_lock() is used in register_console(). I think
> that console_lock_single_hold() will be enough even in
> register_console().
?? And which console would you want to lock? @newcon? It is not
registered yet.
If you want to minimize register_console() locking, it is enough just to
down @console_sem.
> All this is far from obvious. It shows how the API is confusing and
> tricky. And it is another motivation to remove
> console_lock_single_hold().
We need a method to provide @flags synchronization between the kthreads
and console_stop(). Keep in mind that console_lock() does *not* hold the
mutexes. So a completed console_lock() call does *not* mean that the
kthreads are sleeping. They could still lock their own mutex and keep
going. It is not until the kthreads see that CON_THD_BLOCKED is set that
they realize they are not supposed to be running and go to sleep. But
console_stop() could be performing an update to @flags while that
kthread is checking it. It is a data race in code that should be
synchronized.
I spent some time trying to find a good solution for this. Here are the
ideas that I came up with:
1. Use READ_ONCE(short)/WRITE_ONCE(short) because probably that is
enough to guarantee atomic writes/reads on all platforms.
2. Make @flags atomic_t. This guarentees consistence but would require
changing how all consoles initialize that field.
3. Create a separate @enabled boolean field in struct console so that
data races do not matter. This would also change how all consoles
initialize their struct.
4. Provide a new function that uses the mutex to synchronize, since the
kthread is already using the mutex.
I ended up choosing #4 because it had the added benefit of allowing
console_start(), console_stop(), console_unregister() to avoid affecting
the other kthreads.
>> res = 0;
>> @@ -3676,14 +3835,14 @@ static int printk_kthread_func(void *data)
>> kfree(ext_text);
>> kfree(text);
>>
>> - console_lock();
>> + mutex_lock(&con->lock);
>
> This is serialized against unregister_console() but not with
> register_console() because they use different locking scheme.
?? In register_console() the thread has not been created yet. There is
nothing to synchronize against.
> Resume:
>
> I would prefer to get rid of console_lock_single_hold() and
> console_unlock_single_release() API.
>
> It was definitely an interesting experiment. I agree that it would
> be nice to do not block the other kthreads when it is not really
> needed. But from my POV, it adds more harm than good at the moment.
So we go with option #1 to solve(?) the @flags synchronization issue? Or
is there another option I missed?
> It is possible that we will want to do such optimizations
> in the future. But it must be easier to understand what exactly
> is serialized which way. At least it should be more documented.
> Also the same API would need to be used on the related code
> paths.
AFAICT it is used in all places that it is appropriate.
John
On Thu 2022-04-21 16:36:25, John Ogness wrote:
> On 2022-04-21, Petr Mladek <[email protected]> wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> @@ -2603,9 +2666,10 @@ static int console_cpu_notify(unsigned int cpu)
> >> /* If trylock fails, someone else is doing the printing */
> >> if (console_trylock())
> >> console_unlock();
> >> -
> >> - /* Wake kthread printers. Some may have become usable. */
> >> - wake_up_klogd();
> >> + else {
> >> + /* Some kthread printers may have become usable. */
> >> + wake_up_klogd();
> >
> > Do you have any particular scenario in mind, please?
> > Could CPU hotplug put any printk kthread into a sleep?
>
> I do not have a particular scenario. My reasoning was that a CPU coming
> online would affect the conditions of __console_is_usable() for consoles
> without CON_ANYTIME. Of course, it would mean that previously a kthread
> went to sleep because it was trying to print from a CPU that was
> offline. I am doubtful that such a scenario is possible. But you did
> uncover some bizarre code paths where task migration could fail during
> CPU offlining.
>
> Anyway, you suggested to keep the CON_ANYTIME checks for kthreads in
> there. So it seems correct to wake threads anytime the
> printer_should_wake() conditions change.
Makes sense. Thanks for updating the comment in v4.
> >> @@ -2625,11 +2689,33 @@ void console_lock(void)
> >> down_console_sem();
> >> if (console_suspended)
> >> return;
> >> + console_kthreads_block();
> >> console_locked = 1;
> >> console_may_schedule = 1;
> >> }
> >> EXPORT_SYMBOL(console_lock);
> >>
> >> +/*
> >> + * Lock the console_lock, but rather than blocking all the kthread printers,
> >> + * lock a specified kthread printer and hold the lock. This is useful if
> >> + * console flags for a particular console need to be updated.
> >> + */
> >> +void console_lock_single_hold(struct console *con)
> >> +{
> >> + might_sleep();
> >> + down_console_sem();
> >> + mutex_lock(&con->lock);
> >> + console_locked = 1;
> >> + console_may_schedule = 1;
> >
> > This looks wrong. It is a global flag that could be modified
> > only when all consoles are blocked.
>
> You are correct. is_console_locked() needs to return false in this
> scenario. I will leave out the @console_lock setting and insert a
> comment to clarify why.
>
> > This API blocks only the single console. The other consoles are still
> > allowed to print actively.
>
> That is the point. VT does not care about the other printers. VT is
> using @console_locked to protect itself against itself.
>
> > Another problem will appear with the 15th patch. It will remove
> > console_locked variable and is_console_locked() will not longer
> > be aware that this console is locked. We will not know that
> > it might cause deadlock in the VT code.
>
> >From the perspective of VT code the console is _not_ locked. So
> is_console_locked() should return false. is_console_locked() is to make
> sure that the _VT code_ has called console_lock()/console_trylock(). So
> the 15th patch is still correct.
I see. I was not sure about the exact purpose in the VT code. Anyway,
it is clear that the locking is really complicated here.
> >> @@ -2728,17 +2834,18 @@ static void __console_unlock(void)
> >> *
> >> * @handover will be set to true if a printk waiter has taken over the
> >> * console_lock, in which case the caller is no longer holding the
> >> - * console_lock. Otherwise it is set to false.
> >> + * console_lock. Otherwise it is set to false. A NULL pointer may be provided
> >> + * to disable allowing the console_lock to be taken over by a printk waiter.
> >> *
> >> * Returns false if the given console has no next record to print, otherwise
> >> * true.
> >> *
> >> - * Requires the console_lock.
> >> + * Requires the console_lock if @handover is non-NULL.
> >
> > * Requires con->lock otherwise.
>
> Right. I will update the comments.
>
> >> */
> >> -static bool console_emit_next_record(struct console *con, char *text, char *ext_text,
> >> - char *dropped_text, bool *handover)
> >> +static bool __console_emit_next_record(struct console *con, char *text, char *ext_text,
> >> + char *dropped_text, bool *handover)
> >> {
> >> - static int panic_console_dropped;
> >> + static atomic_t panic_console_dropped = ATOMIC_INIT(0);
> >> struct printk_info info;
> >> struct printk_record r;
> >> unsigned long flags;
> >> @@ -3261,6 +3401,8 @@ void register_console(struct console *newcon)
> >>
> >> newcon->dropped = 0;
> >> newcon->thread = NULL;
> >> + newcon->flags |= CON_THD_BLOCKED;
> >
> > Just to show the complexity added by console_lock_single_hold():
> >
> > It took me some time to realize that it is correct. The flag
> > is needed because the console will be added under console_lock().
> > The flag would not be needed when it was added under
> > console_lock_single_hold().
>
> ?? But it is not added under
> console_lock_single_hold(). console_lock_single_hold() is not a
> replacement for console_lock(). Their purpose is very
> different. console_lock_single_hold() is an internal function to provide
> synchronization for @flags and @thread updates of a single console.
>
> Maybe we are getting caught in my "bad naming" trap again. :-/
From my POV, the problem was that the real purpose was far from
clear and it was not mentioned anywhere. If it was mentioned
then it was too cryptic for me. The comments focused too
much on the possibility to do some changes without disturbing
other kthreads.
> We need a method to provide @flags synchronization between the kthreads
> and console_stop(). Keep in mind that console_lock() does *not* hold the
> mutexes. So a completed console_lock() call does *not* mean that the
> kthreads are sleeping. They could still lock their own mutex and keep
> going. It is not until the kthreads see that CON_THD_BLOCKED is set that
> they realize they are not supposed to be running and go to sleep.
> But console_stop() could be performing an update to @flags while that
> kthread is checking it. It is a data race in code that should be
> synchronized.
Nice derscription of the problem.
> I spent some time trying to find a good solution for this. Here are the
> ideas that I came up with:
>
> 1. Use READ_ONCE(short)/WRITE_ONCE(short) because probably that is
> enough to guarantee atomic writes/reads on all platforms.
>
> 2. Make @flags atomic_t. This guarentees consistence but would require
> changing how all consoles initialize that field.
>
> 3. Create a separate @enabled boolean field in struct console so that
> data races do not matter. This would also change how all consoles
> initialize their struct.
> 4. Provide a new function that uses the mutex to synchronize, since the
> kthread is already using the mutex.
>
> I ended up choosing #4 because it had the added benefit of allowing
> console_start(), console_stop(), console_unregister() to avoid affecting
> the other kthreads.
IMHO, it is actually a generic problem of the complex locking scheme
when there are too many combinations of the protected data.
In the current state, the problem seems to be only with CON_ENABLED
flag but there might be other hidden races in the future.
IMHO, it would be much easier when there are the following rules:
+ console_lock() blocks taking con->lock
+ con->lock blocks taking console_lock()
+ Different con->lock might be taken in parallel
The result would be:
+ global variables need to be guarded by the big console_lock()
+ con->lock should be enough to guard per-console variables
+ the big console_lock() would serialize also access to
per-console variables.
Of course, it is not that simple. I am not 100% that we could
even achieve this. Anyway, I think about the following
wrapper:
void single_console_lock(struct console *con)
{
for (;;) {
error = wait_event_interruptible(log_wait,
con->flags & CON_THB_BLOCKED);
if (error)
continue;
mutex_lock(&con->lock);
if (!con->flags & CON_THB_BLOCKED)
break;
mutex_unlock(&con->lock);
}
}
void single_console_unlock(struct console *con)
{
mutex_unlock(&con->lock);
}
We should use it everywhere instead of the simple mutex_lock(con->lock)
and mutex_lock(con->lock). And we could remove mutex_lock()/unlock()
from code called under the big console_lock().
How does it sound, please?
Best Regards,
Petr