Messages in this thread | | | Date | Fri, 14 Apr 2023 11:56:05 +0200 | From | Petr Mladek <> | Subject | Re: [PATCH printk v1 04/18] printk: Add per-console suspended state |
| |
On Fri 2023-03-17 14:28:04, John Ogness wrote: > On 2023-03-08, Petr Mladek <pmladek@suse.com> wrote: > >> --- a/include/linux/console.h > >> +++ b/include/linux/console.h > >> @@ -153,6 +153,8 @@ static inline int con_debug_leave(void) > >> * receiving the printk spam for obvious reasons. > >> * @CON_EXTENDED: The console supports the extended output format of > >> * /dev/kmesg which requires a larger output buffer. > >> + * @CON_SUSPENDED: Indicates if a console is suspended. If true, the > >> + * printing callbacks must not be called. > >> */ > >> enum cons_flags { > >> CON_PRINTBUFFER = BIT(0), > >> @@ -162,6 +164,7 @@ enum cons_flags { > >> CON_ANYTIME = BIT(4), > >> CON_BRL = BIT(5), > >> CON_EXTENDED = BIT(6), > >> + CON_SUSPENDED = BIT(7), > > > > We have to show it in /proc/consoles, see fs/proc/consoles.c. > > Are we supposed to show all flags in /proc/consoles? Currently > CON_EXTENDED is not shown either.
Good question. It is true that CON_SUSPENDED flag is not that useful. Userspace will likely be frozen when this flag is set. I am fine with not showing it.
Well, CON_EXTENDED might actually be useful. It defines the format of the console messages. It would be nice to fix this but it is not in the scope of this patchset.
> >> --- a/kernel/printk/printk.c > >> +++ b/kernel/printk/printk.c > >> @@ -2574,11 +2590,26 @@ void suspend_console(void) > >> > >> void resume_console(void) > >> { > >> + struct console *con; > >> + > >> if (!console_suspend_enabled) > >> return; > >> down_console_sem(); > >> console_suspended = 0; > >> console_unlock(); > >> + > >> + console_list_lock(); > >> + for_each_console(con) > >> + console_srcu_write_flags(con, con->flags & ~CON_SUSPENDED); > >> + console_list_unlock(); > >> + > >> + /* > >> + * Ensure that all SRCU list walks have completed. All printing > >> + * contexts must be able to see they are no longer suspended so > >> + * that they are guaranteed to wake up and resume printing. > >> + */ > >> + synchronize_srcu(&console_srcu); > >> + > > > > The setting of the global "console_suspended" and per-console > > CON_SUSPENDED flag is not synchronized. As a result, they might > > become inconsistent: > > They do not need to be synchronized and it doesn't matter if they become > inconsistent. With this patch they are no longer related. One is for > tracking the state of the console (CON_SUSPENDED), the other is for > tracking the suspend trick for the console_lock.
OK, the race might be theoretical because it would be a race between suspend_console() and resume_console(). But it exists:
CPU0 CPU1
suspend_console()
console_list_lock(); for_each_console(con) con->flags |= CON_SUSPENDED; console_list_unlock();
resume_console()
down_console_sem(); console_suspended = 0; console_unlock();
console_list_lock(); for_each_console(con) con->flags &= ~CON_SUSPENDED; console_list_unlock();
down_console_sem(); console_supended = 1; up_console_sem();
Result:
console_supended == 1; con->flags & CON_SUSPENDED == 0;
+ NO_BKL consoles would work because they ignore console_supend. + legacy consoles won't work because console_unlock() would return early.
This does not look right.
> > I think that we could just remove the global "console_suspended" flag. > > > > IMHO, it used to be needed to avoid moving the global "console_seq" forward > > when the consoles were suspended. But it is not needed now with the > > per-console con->seq. console_flush_all() skips consoles when > > console_is_usable() fails and it bails out when there is no progress. > > The @console_suspended flag is used to allow console_lock/console_unlock > to be called without triggering printing. This is probably so that vt > code can make use of the console_lock for its own internal locking, even > when in a state where ->write() should not be called. I would expect we > still need it, even if the consoles do not.
But it would still work. console_unlock() could always call console_flush_all() now. It just does not make any progress when all consoles have CON_SUSPENDED flag set.
Note that this is a new behavior since the commit a699449bb13b70b8bd ("printk: refactor and rework printing logic"). Before this commit, the main loop in console_unlock() always incremented console_seq even when no console was enabled. This is why console_unlock() had to skip the main loop when the consoles were suspended.
I believe that @console_suspended is not longer needed. Let's replace it with the per-console flag and do not worry about races.
Best Regards, Petr
| |