lkml.org 
[lkml]   [2023]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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

    \
     
     \ /
      Last update: 2023-04-14 11:57    [W:2.950 / U:0.344 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site