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

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

[Sebastian Andrzej Siewior]

In commit b845b517b5e37 ("printk: robustify printk") initially got its
delayed wake up which is today implemented via the irq_work. Back then
it was a variable which was assigned to 1 if a pending wake up was
waiting. This variable was checked on each timer tick. Therefore it
makes sense to assign a per-CPU variable with this task to avoid cache
line bouncing when each CPU reads the variable on each timer tick
simultaneously.

Today we have a irq_work which is enqueued onto a list and handled if it
has work items assigned. This list is already per-CPU.

We could replace the per-CPU irq_work and the per-CPU printk_pending with
two irq_work structs: One for PRINTK_PENDING_WAKEUP and the other for
PRINTK_PENDING_OUTPUT.
While unrolling the conditions in __wake_up_klogd() it becomes obvious
that PRINTK_PENDING_WAKEUP is always passed as an argument but never
checked. The return value wq_has_sleeper() of does not really matter
it's sole purpose is to enforce the needed memory barrier.

Redo the irq_work by:
- Use one irq_work (pending_wakeup_work) for klogd wakes. Schedule it
  only if there is a sleeper waiting.
- Use one irq_work (&pending_output_work) for output printing. Schedule
  it only if PRINTK_PENDING_OUTPUT passed.
- Drop unused PRINTK_PENDING_WAKEUP.
- Remove the preempt_disable() section. Its purpose was to ensure that
  printk_pending and wake_up_klogd_work is accessed from the same CPU.

Worst case is defer_console_output() where two irq_work are scheduled but
those get processed one after the other during the HZ tick so it
shouldn't get worse.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---

v1…v2: https://lore.kernel.org/all/20260911103832.w6C8cT4L@linutronix.de/
  - Merge the preempt_disable() removal bits.
  - Keep the printk_percpu_data_ready() check. It is needed to to
    corrupt the irq_work lists.

 kernel/printk/printk.c | 53 ++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 30 deletions(-)

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);
 }
 
 /**
@@ -4663,7 +4656,7 @@ static void __wake_up_klogd(int val)
  */
 void wake_up_klogd(void)
 {
-	__wake_up_klogd(PRINTK_PENDING_WAKEUP);
+	__wake_up_klogd(0);
 }
 
 /**
@@ -4684,7 +4677,7 @@ void defer_console_output(void)
 	 * New messages may have been added directly to the ringbuffer
 	 * using vprintk_store(), so wake any waiters as well.
 	 */
-	__wake_up_klogd(PRINTK_PENDING_WAKEUP | PRINTK_PENDING_OUTPUT);
+	__wake_up_klogd(PRINTK_PENDING_OUTPUT);
 }
 
 /**
-- 
2.55.0

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

[Sebastian Andrzej Siewior]

On 2026-09-14 16:34:12 [+0200], To linux-kernel@vger.kernel.org wrote:
> In commit b845b517b5e37 ("printk: robustify printk") initially got its
> delayed wake up which is today implemented via the irq_work. Back then
> it was a variable which was assigned to 1 if a pending wake up was
> waiting. This variable was checked on each timer tick. Therefore it
> makes sense to assign a per-CPU variable with this task to avoid cache
> line bouncing when each CPU reads the variable on each timer tick
> simultaneously.

Sashiko complains here, https://sashiko.dev/#/patchset/20260914143412.1sMcp89V%40linutronix.de

| Does replacing the per-CPU irq_work items with global items introduce a
| system-wide starvation vulnerability on PREEMPT_RT?
| On PREEMPT_RT, IRQ_WORK_LAZY items are processed by a per-CPU irq_work thread
| running at a low SCHED_FIFO priority.
| If one CPU queues pending_wakeup_work or pending_output_work, the item is
| added to its local lazy_list and marked as pending. If that CPU runs a
| higher priority RT task for an extended period, the irq_work thread is starved.
| Since the irq_work structs are now global, their IRQ_WORK_PENDING flag remains
| set during this time. Any subsequent calls to irq_work_queue() on other CPUs
| will fail to queue the work and drop the wakeup requests.
| Could an RT workload on a single CPU starve the global printk/klogd mechanisms
| for the entire system, degrading the isolation provided by the previous per-CPU
| design?

while this is true, it is not limited to printk but any workload would
starve in such a scenario. Also, for the per-CPU variant to make a
difference, one would need to have a printk output on another CPU, which
is not busy with long running RT tasks.
Long term this is probably the "legacy" system as console drivers should
transform into nbcon interface. Here this irq-work would be reduced the
userland wake up while the printing would happen via the nbcon thread
which also has a single irq_work queue.

I don't see a problem with this.

Sebastian

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

[Petr Mladek]

On Tue 2026-09-22 09:08:34, Sebastian Andrzej Siewior wrote:
> On 2026-09-14 16:34:12 [+0200], To linux-kernel@vger.kernel.org wrote:
> > In commit b845b517b5e37 ("printk: robustify printk") initially got its
> > delayed wake up which is today implemented via the irq_work. Back then
> > it was a variable which was assigned to 1 if a pending wake up was
> > waiting. This variable was checked on each timer tick. Therefore it
> > makes sense to assign a per-CPU variable with this task to avoid cache
> > line bouncing when each CPU reads the variable on each timer tick
> > simultaneously.
> 
> Sashiko complains here, https://sashiko.dev/#/patchset/20260914143412.1sMcp89V%40linutronix.de
> 
> | Does replacing the per-CPU irq_work items with global items introduce a
> | system-wide starvation vulnerability on PREEMPT_RT?
> | On PREEMPT_RT, IRQ_WORK_LAZY items are processed by a per-CPU irq_work thread
> | running at a low SCHED_FIFO priority.
> | If one CPU queues pending_wakeup_work or pending_output_work, the item is
> | added to its local lazy_list and marked as pending. If that CPU runs a
> | higher priority RT task for an extended period, the irq_work thread is starved.
> | Since the irq_work structs are now global, their IRQ_WORK_PENDING flag remains
> | set during this time. Any subsequent calls to irq_work_queue() on other CPUs
> | will fail to queue the work and drop the wakeup requests.
> | Could an RT workload on a single CPU starve the global printk/klogd mechanisms
> | for the entire system, degrading the isolation provided by the previous per-CPU
> | design?
> 
> while this is true, it is not limited to printk but any workload would
> starve in such a scenario. Also, for the per-CPU variant to make a
> difference, one would need to have a printk output on another CPU, which
> is not busy with long running RT tasks.
> Long term this is probably the "legacy" system as console drivers should
> transform into nbcon interface. Here this irq-work would be reduced the
> userland wake up while the printing would happen via the nbcon thread
> which also has a single irq_work queue.
> 
> I don't see a problem with this.

I agree with your explanation. I do not see this as a problem either.

Best Regards,
Petr

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

[Petr Mladek]

On Mon 2026-09-14 16:34:12, Sebastian Andrzej Siewior wrote:
> In commit b845b517b5e37 ("printk: robustify printk") initially got its
> delayed wake up which is today implemented via the irq_work. Back then
> it was a variable which was assigned to 1 if a pending wake up was
> waiting. This variable was checked on each timer tick. Therefore it
> makes sense to assign a per-CPU variable with this task to avoid cache
> line bouncing when each CPU reads the variable on each timer tick
> simultaneously.
> 
> Today we have a irq_work which is enqueued onto a list and handled if it
> has work items assigned. This list is already per-CPU.
> 
> We could replace the per-CPU irq_work and the per-CPU printk_pending with
> two irq_work structs: One for PRINTK_PENDING_WAKEUP and the other for
> PRINTK_PENDING_OUTPUT.
> While unrolling the conditions in __wake_up_klogd() it becomes obvious
> that PRINTK_PENDING_WAKEUP is always passed as an argument but never
> checked. The return value wq_has_sleeper() of does not really matter
> it's sole purpose is to enforce the needed memory barrier.
> 
> Redo the irq_work by:
> - Use one irq_work (pending_wakeup_work) for klogd wakes. Schedule it
>   only if there is a sleeper waiting.
> - Use one irq_work (&pending_output_work) for output printing. Schedule
>   it only if PRINTK_PENDING_OUTPUT passed.
> - Drop unused PRINTK_PENDING_WAKEUP.
> - Remove the preempt_disable() section. Its purpose was to ensure that
>   printk_pending and wake_up_klogd_work is accessed from the same CPU.
> 
> Worst case is defer_console_output() where two irq_work are scheduled but
> those get processed one after the other during the HZ tick so it
> shouldn't get worse.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>

I like this a lot. I always thought that using per-CPU variable was an
overkill. I had the clean up of this code on my TODO list for a
long time but I never came to it.

It looks good to me.

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr

PS: I am going to wait few more days and give others chance to look at
    it. I will commit it next week if nobody complains.

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

[Sebastian Andrzej Siewior]

On 2026-09-22 12:02:27 [+0200], Petr Mladek wrote:
> PS: I am going to wait few more days and give others chance to look at
>     it. I will commit it next week if nobody complains.

Thank you.

Sebastian

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

[John Ogness]

On 2026-09-22, Petr Mladek <pmladek@suse.com> wrote:
>> Sashiko complains here, https://sashiko.dev/#/patchset/20260914143412.1sMcp89V%40linutronix.de
>> 
>> | Does replacing the per-CPU irq_work items with global items introduce a
>> | system-wide starvation vulnerability on PREEMPT_RT?
>> | On PREEMPT_RT, IRQ_WORK_LAZY items are processed by a per-CPU irq_work thread
>> | running at a low SCHED_FIFO priority.
>> | If one CPU queues pending_wakeup_work or pending_output_work, the item is
>> | added to its local lazy_list and marked as pending. If that CPU runs a
>> | higher priority RT task for an extended period, the irq_work thread is starved.
>> | Since the irq_work structs are now global, their IRQ_WORK_PENDING flag remains
>> | set during this time. Any subsequent calls to irq_work_queue() on other CPUs
>> | will fail to queue the work and drop the wakeup requests.
>> | Could an RT workload on a single CPU starve the global printk/klogd mechanisms
>> | for the entire system, degrading the isolation provided by the previous per-CPU
>> | design?
>> 
>> while this is true, it is not limited to printk but any workload would
>> starve in such a scenario. Also, for the per-CPU variant to make a
>> difference, one would need to have a printk output on another CPU, which
>> is not busy with long running RT tasks.
>> Long term this is probably the "legacy" system as console drivers should
>> transform into nbcon interface. Here this irq-work would be reduced the
>> userland wake up while the printing would happen via the nbcon thread
>> which also has a single irq_work queue.
>> 
>> I don't see a problem with this.
>
> I agree with your explanation. I do not see this as a problem either.

I also agree with the explanation. This is not a new problem because
starvation was always possible. With nbcon we have the advantage that
if an "RT-loaded CPU" enters the emergency state, the backlog will be
flushed in atomic mode.

John

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

[John Ogness]

On 2026-09-14, Sebastian Andrzej Siewior <bigeasy@linutronix.de> 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@linutronix.de>

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

[Sebastian Andrzej Siewior]

On 2026-09-22 19:35:26 [+0200], John Ogness wrote:
> > @@ -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.

Hmm. I did not give much thinking into the ordering because it shouldn't
matter. We used to have "unlock" followed by "wakeup" in the irq-work
callback and this is what we have now given the LIFO ordering.
Having "wakeup" first might not take effect immediately because the
scheduler delays it or puts it on the current CPU and then it is delayed
until after the interrupt ("unlock") is done. So…

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

Sebastian

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

[Petr Mladek]

On Wed 2026-09-23 09:55:12, Sebastian Andrzej Siewior wrote:
> On 2026-09-22 19:35:26 [+0200], John Ogness wrote:
> > > @@ -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?

Great catch.

> > 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.
> 
> Hmm. I did not give much thinking into the ordering because it shouldn't
> matter. We used to have "unlock" followed by "wakeup" in the irq-work
> callback and this is what we have now given the LIFO ordering.
> Having "wakeup" first might not take effect immediately because the
> scheduler delays it or puts it on the current CPU and then it is delayed
> until after the interrupt ("unlock") is done. So…

Honestly, I am not sure what ordering is better. And it depends on
the internal irq_work implementation now. I would keep the patch
as is.

Best Regards,
Petr