Messages in this thread | | | From | John Ogness <> | Subject | Re: [PATCH printk v3 14/15] printk: extend console_lock for proper kthread support | Date | Fri, 22 Apr 2022 16:20:52 +0206 |
| |
On 2022-04-22, Petr Mladek <pmladek@suse.com> wrote: > IMHO, it is actually a generic problem of the complex locking scheme > when there are too many combinations of the protected data.
Sure. We are in a delicate situation of continuing to support the old locking scheme while transitioning to a new one.
> 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.
It looks like you are talking about nested locking. This was my original idea but I had problems relating to kthread stopping. However, the code has changed a lot since then and now when I look at it, it does not look like it would be a problem. Getting rid of CON_THD_BLOCKED would greatly simplify the relationship between console_lock and kthreads.
For this we would need the console list to become a list_head so that it is doubly linked (in order to unlock in reverse order). That probably would be a good idea anyway. It is a bit bizarre that printk implements its own linked list.
> Of course, it is not that simple. I am not 100% that we could > even achieve this.
It just might be that simple. I will explore it again.
> 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().
Hmmm. Waiting on @log_wait is not correct. A @log_wait wakeup with the kthread already in the blocked state is unusual. There would need to be a per-console waitqueue for when the kthread unlocks its mutex.
Maybe something like:
void single_console_lock(struct console *con) { for (;;) { error = wait_event_interruptible(con->lock_wait, !(con->flags & CON_THB_BLOCKED)); if (error) continue;
mutex_lock(&con->lock);
if (!(con->flags & CON_THB_BLOCKED)) break;
mutex_unlock(&con->lock); } }
And in printk_kthread_func(), after the kthread unlocks its con->lock, it calls:
if (wq_has_sleeper(&con->lock_wait)) wake_up_interruptible_all(&con->lock_wait);
But single_console_lock() would not be allowed to be called under console_lock(), so I don't see how it is useful. con->flags is always modified under @console_sem to make sure the console does not disappear.
Anyway, I will first look into the nested locking solution. That seems more promising to me and it would go a long way to simplify the locking hierarchy.
John
| |