console_is_usable() check: was: Re: [PATCH printk v2 10/12] printk: add kthread console printers

Petr Mladek
2022-04-07T16:49:36Z

On Tue 2022-04-05 15:31:33, John Ogness wrote:
> Create a kthread for each console to perform console printing. During
> normal operation (@system_state == SYSTEM_RUNNING), the kthread
> printers are responsible for all printing on their respective
> consoles.
>
> During non-normal operation, console printing is done as it has been:
> within the context of the printk caller or within irq work triggered
> by the printk caller.
>
> Console printers synchronize against each other and against console
> lockers by taking the console lock for each message that is printed.
>
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> +static bool printer_should_wake(struct console *con, u64 seq)
> +{
> + short flags;
> +
> + if (kthread_should_stop() || !printk_kthreads_available)
> + return true;
> +
> + if (console_suspended)
> + return false;
> +
> + if (!con->write)
> + return false;

Hmm, the kthread for such consoles will never wake up. It probably
does not make sense to create it at all.

On the other hand, it is not a big deal. And we have "bigger" problem
how to make these checks in sync with console_is_usable(), see below.

> + /*
> + * This is an unsafe read to con->flags, but a false positive is not
> + * a problem. Worst case it would allow the printer to wake up even
> + * when it is disabled. But the printer will notice that itself when
> + * attempting to print and instead go back to sleep.
> + */
> + flags = data_race(READ_ONCE(con->flags));
> + if (!(flags & CON_ENABLED))
> + return false;
> +
> + if (atomic_read(&printk_prefer_direct))
> + return false;
> +
> + return prb_read_valid(prb, seq, NULL);
> +}
> +
> +static int printk_kthread_func(void *data)
> +{
> + struct console *con = data;
> + char *dropped_text = NULL;
> + char *ext_text = NULL;
> + bool handover;
> + u64 seq = 0;
> + char *text;
> + int error;
> +
> + text = kmalloc(CONSOLE_LOG_MAX, GFP_KERNEL);
> + if (!text) {
> + printk_console_msg(con, KERN_ERR, "failed to allocate text buffer");
> + printk_fallback_preferred_direct();
> + goto out;
> + }
> +
> + if (con->flags & CON_EXTENDED) {
> + ext_text = kmalloc(CONSOLE_EXT_LOG_MAX, GFP_KERNEL);
> + if (!ext_text) {
> + printk_console_msg(con, KERN_ERR, "failed to allocate ext_text buffer");
> + printk_fallback_preferred_direct();
> + goto out;
> + }
> + } else {
> + dropped_text = kmalloc(DROPPED_TEXT_MAX, GFP_KERNEL);
> + if (!dropped_text) {
> + printk_console_msg(con, KERN_ERR,
> + "failed to allocate dropped_text buffer");
> + printk_fallback_preferred_direct();
> + goto out;
> + }
> + }
> +
> + printk_console_msg(con, KERN_INFO, "printing thread started");
> +
> + for (;;) {
> + /*
> + * Guarantee this task is visible on the waitqueue before
> + * checking the wake condition.
> + *
> + * The full memory barrier within set_current_state() of
> + * prepare_to_wait_event() pairs with the full memory barrier
> + * within wq_has_sleeper().
> + *
> + * See __wake_up_klogd:A for the pairing memory barrier.
> + */
> + error = wait_event_interruptible(log_wait,
> + printer_should_wake(con, seq)); /* LMM(printk_kthread_func:A) */
> +
> + if (kthread_should_stop() || !printk_kthreads_available)
> + break;
> +
> + if (error)
> + continue;
> +
> + console_lock();
> +
> + if (console_suspended) {
> + __console_unlock();
> + continue;
> + }
> +
> + if (!console_is_usable(con)) {
> + __console_unlock();
> + continue;
> + }

This smells with a busy loop. We should make sure that the same
condition will make printk_kthread_func() return false. The current
approach is hard to maintain.

Hmm, it is not easy because console_is_usable(con) is supposed
to be called under console_lock().

I do not have a good solution for this. But the current approach looks
error prone. What about the following?

static inline bool __console_is_usable(struct console *con)
{
short flags;

if (!con->write)
return false;

/* Make flags checks consistent when called without console_lock. */
flags = READ_ONCE(con->flags);

if (!(con->flags & CON_ENABLED))
return false;

/*
* Console drivers may assume that per-cpu resources have been
* allocated. So unless they're explicitly marked as being able to
* cope (CON_ANYTIME) don't call them until this CPU is officially up.
*/
if (!cpu_online(raw_smp_processor_id()) &&
!(con->flags & CON_ANYTIME))
return false;

return true;
}

static inline bool console_is_usable(struct console *con)
{
WARN_ON_ONCE(!lockdep_assert_held(&console_sem));

__console_is_usable();
}

Note that we could not use lockdep_assert_held() because we will
later need to check both console_sem and con->mutex. Either of
them will be enough.

> +
> + /*
> + * Even though the printk kthread is always preemptible, it is
> + * still not allowed to call cond_resched() from within
> + * console drivers. The task may become non-preemptible in the
> + * console driver call chain. For example, vt_console_print()
> + * takes a spinlock and then can call into fbcon_redraw(),
> + * which can conditionally invoke cond_resched().
> + */
> + console_may_schedule = 0;
> + console_emit_next_record(con, text, ext_text, dropped_text, &handover);
> + if (handover)
> + continue;
> +
> + seq = con->seq;
> +
> + __console_unlock();
> + }
> +
> + printk_console_msg(con, KERN_INFO, "printing thread stopped");
> +out:
> + kfree(dropped_text);
> + kfree(ext_text);
> + kfree(text);
> + return 0;
> +}

Best Regards,
Petr