Re: [PATCH v2] printk: Use two irq_works instead per-CPU

[John Ogness]

On 2026-09-14, Sebastian Andrzej Siewior <bigeasy@xxxxxxxxxxxxx> wrote:
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 6d3d18a50da74..d5cfd56f1a001 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -4593,31 +4593,26 @@ bool pr_flush(int timeout_ms, bool reset_on_progress)
>  /*
>   * Delayed printk version, for scheduler-internal messages:
>   */
> -#define PRINTK_PENDING_WAKEUP	0x01
> -#define PRINTK_PENDING_OUTPUT	0x02
> +#define PRINTK_PENDING_OUTPUT	0x01
>  
> -static DEFINE_PER_CPU(int, printk_pending);
> -
> -static void wake_up_klogd_work_func(struct irq_work *irq_work)
> +static void pending_wake_fn(struct irq_work *irq_work)
>  {
> -	int pending = this_cpu_xchg(printk_pending, 0);
> -
> -	if (pending & PRINTK_PENDING_OUTPUT) {
> -		if (force_legacy_kthread()) {
> -			if (printk_legacy_kthread)
> -				wake_up_interruptible(&legacy_wait);
> -		} else {
> -			if (console_trylock())
> -				console_unlock();
> -		}
> -	}
> -
> -	if (pending & PRINTK_PENDING_WAKEUP)
> -		wake_up_interruptible(&log_wait);
> +	wake_up_interruptible(&log_wait);
>  }
>  
> -static DEFINE_PER_CPU(struct irq_work, wake_up_klogd_work) =
> -	IRQ_WORK_INIT_LAZY(wake_up_klogd_work_func);
> +static void pending_output_fn(struct irq_work *irq_work)
> +{
> +	if (force_legacy_kthread()) {
> +		if (printk_legacy_kthread)
> +			wake_up_interruptible(&legacy_wait);
> +	} else {
> +		if (console_trylock())
> +			console_unlock();
> +	}
> +}
> +
> +static struct irq_work pending_wakeup_work = IRQ_WORK_INIT_LAZY(pending_wake_fn);
> +static struct irq_work pending_output_work = IRQ_WORK_INIT_LAZY(pending_output_fn);
>  
>  static void __wake_up_klogd(int val)
>  {
> @@ -4631,7 +4626,6 @@ static void __wake_up_klogd(int val)
>  	if (WARN_ON_ONCE(console_irqwork_blocked))
>  		return;
>  
> -	preempt_disable();
>  	/*
>  	 * Guarantee any new records can be seen by tasks preparing to wait
>  	 * before this context checks if the wait queue is empty.
> @@ -4643,12 +4637,11 @@ static void __wake_up_klogd(int val)
>  	 *
>  	 * This pairs with devkmsg_read:A and syslog_print:A.
>  	 */
> -	if (wq_has_sleeper(&log_wait) || /* LMM(__wake_up_klogd:A) */
> -	    (val & PRINTK_PENDING_OUTPUT)) {
> -		this_cpu_or(printk_pending, val);
> -		irq_work_queue(this_cpu_ptr(&wake_up_klogd_work));
> -	}
> -	preempt_enable();
> +	if (wq_has_sleeper(&log_wait)) /* LMM(__wake_up_klogd:A) */
> +		irq_work_queue(&pending_wakeup_work);
> +
> +	if (val & PRINTK_PENDING_OUTPUT)
> +		irq_work_queue(&pending_output_work);

The ordering of operations has been reverse queued. Perhaps because
irq_work is LIFO (implementation internal detail) and you wanted to
preserve the current ordering? Or maybe this ordering was chosen because
the code looks nicer. Either way, I think it doesn't matter if the
legacy flushing occurs before/after waking the klogd waiter.

Reviewed-by: John Ogness <john.ogness@xxxxxxxxxxxxx>