Message-ID: <Yk8WD2vZEm880fo4@alley>
Date:   Thu, 7 Apr 2022 18:49:19 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <senozhatsky@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        linux-kernel@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: console_is_usable() check: was: Re: [PATCH printk v2 10/12] printk:
 add kthread console printers

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

Powered by blists - more mailing lists