[PATCH] printk: Enough to disable preemption in printk deferred context

[PATCH] printk: Enough to disable preemption in printk deferred context

[Petr Mladek]

The comment above printk_deferred_enter()/exit() definition claims
that it can be used only when interrupts are disabled.

It was required by the original printk_safe_log_store() implementation.
The code provided lockless synchronization between a single writer and
a single reader. The interrupt and the normal context shared the same
buffer.

The commit 93d102f094be ("printk: remove safe buffers") removed
these temporary buffers. Instead, the messages are stored directly into
the new global lockless buffer which supports multiple parallel writers.

As a result, it is safe to interrupt one writer now. The preemption still
has to be disabled because the deferred context is CPU specific.

Fixes: 93d102f094be ("printk: remove safe buffers")
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 include/linux/printk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/printk.h b/include/linux/printk.h
index 8ef499ab3c1e..915a321b491e 100644
--- a/include/linux/printk.h
+++ b/include/linux/printk.h
@@ -161,7 +161,7 @@ extern void __printk_safe_enter(void);
 extern void __printk_safe_exit(void);
 /*
  * The printk_deferred_enter/exit macros are available only as a hack for
- * some code paths that need to defer all printk console printing. Interrupts
+ * some code paths that need to defer all printk console printing. Preemption
  * must be disabled for the deferred duration.
  */
 #define printk_deferred_enter __printk_safe_enter
-- 
2.35.3

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[Sergey Senozhatsky]

On (23/04/19 09:42), Petr Mladek wrote:
> The comment above printk_deferred_enter()/exit() definition claims
> that it can be used only when interrupts are disabled.
> 
> It was required by the original printk_safe_log_store() implementation.
> The code provided lockless synchronization between a single writer and
> a single reader. The interrupt and the normal context shared the same
> buffer.
> 
> The commit 93d102f094be ("printk: remove safe buffers") removed
> these temporary buffers. Instead, the messages are stored directly into
> the new global lockless buffer which supports multiple parallel writers.
> 
> As a result, it is safe to interrupt one writer now. The preemption still
> has to be disabled because the deferred context is CPU specific.
> 
> Fixes: 93d102f094be ("printk: remove safe buffers")
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Looks good to me

Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[Michal Hocko]

On Wed 19-04-23 09:42:10, Petr Mladek wrote:
> The comment above printk_deferred_enter()/exit() definition claims
> that it can be used only when interrupts are disabled.
> 
> It was required by the original printk_safe_log_store() implementation.
> The code provided lockless synchronization between a single writer and
> a single reader. The interrupt and the normal context shared the same
> buffer.
> 
> The commit 93d102f094be ("printk: remove safe buffers") removed
> these temporary buffers. Instead, the messages are stored directly into
> the new global lockless buffer which supports multiple parallel writers.
> 
> As a result, it is safe to interrupt one writer now. The preemption still
> has to be disabled because the deferred context is CPU specific.

Thanks for the clarification and explanation.

> Fixes: 93d102f094be ("printk: remove safe buffers")

Is this a fix though? I would expect some users to be changed from irq
to preempt to disabling to be considered a fix.

> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  include/linux/printk.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/printk.h b/include/linux/printk.h
> index 8ef499ab3c1e..915a321b491e 100644
> --- a/include/linux/printk.h
> +++ b/include/linux/printk.h
> @@ -161,7 +161,7 @@ extern void __printk_safe_enter(void);
>  extern void __printk_safe_exit(void);
>  /*
>   * The printk_deferred_enter/exit macros are available only as a hack for
> - * some code paths that need to defer all printk console printing. Interrupts
> + * some code paths that need to defer all printk console printing. Preemption
>   * must be disabled for the deferred duration.
>   */
>  #define printk_deferred_enter __printk_safe_enter
> -- 
> 2.35.3

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[John Ogness]

On 2023-04-19, Petr Mladek <pmladek@suse.com> wrote:
> it is safe to interrupt one writer now. The preemption still
> has to be disabled because the deferred context is CPU specific.

Really it is enough to disable migration.

We need to keep an eye on the usage of this function. By allowing
interrupts and preemption, it means that other printk's on that CPU will
also be deferred if the context interrupted within the deferred block.

John

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[Petr Mladek]

On Wed 2023-04-19 10:23:33, Michal Hocko wrote:
> On Wed 19-04-23 09:42:10, Petr Mladek wrote:
> > The comment above printk_deferred_enter()/exit() definition claims
> > that it can be used only when interrupts are disabled.
> > 
> > It was required by the original printk_safe_log_store() implementation.
> > The code provided lockless synchronization between a single writer and
> > a single reader. The interrupt and the normal context shared the same
> > buffer.
> > 
> > The commit 93d102f094be ("printk: remove safe buffers") removed
> > these temporary buffers. Instead, the messages are stored directly into
> > the new global lockless buffer which supports multiple parallel writers.
> > 
> > As a result, it is safe to interrupt one writer now. The preemption still
> > has to be disabled because the deferred context is CPU specific.
> 
> Thanks for the clarification and explanation.
> 
> > Fixes: 93d102f094be ("printk: remove safe buffers")
> 
> Is this a fix though? I would expect some users to be changed from irq
> to preempt to disabling to be considered a fix.

Yeah, I am not sure about the Fixes tag either. I wanted to cross-link
the two commits. But it is probably enough to mention it in the commit
message.

Best Regards,
Petr

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[Petr Mladek]

On Wed 2023-04-19 11:11:52, John Ogness wrote:
> On 2023-04-19, Petr Mladek <pmladek@suse.com> wrote:
> > it is safe to interrupt one writer now. The preemption still
> > has to be disabled because the deferred context is CPU specific.
> 
> Really it is enough to disable migration.

True. But it gets too far to my taste. As you describe below.
It affects all printk's on the CPU.

Sigh, even the enabled intrrupts might be questionable. For example,
when the iterrupt is from a watchdog and want's to report a stall.

> We need to keep an eye on the usage of this function. By allowing
> interrupts and preemption, it means that other printk's on that CPU will
> also be deferred if the context interrupted within the deferred block.

A solution would be to make this more clear in the comment.
Something like:

/*
 * The printk_deferred_enter/exit macros are available only as a hack.
 * They define a per-CPU context where all printk console printing is
 * deferred because it might cause a deadlock otherwise.
 *
 * The API user is responsible for calling the corresponding enter/exit
 * pair on the same CPU. It is highly recommended to use them only in
 * a context with interrupts disabled. Otherwise, other unrelated
 * printk() calls might be deferred when they interrupt/preempt
 * the deferred code section.
 */

Another solution would be to stay on the "safe" side and keep the
comment as is or even enforce disabling interrupts by the API.

I would personally just improve the comment. It is good to describe
the situation correctly. We could always add restrictions when
there are problems in practice.

Best Regards,
Petr

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[Michal Hocko]

On Wed 19-04-23 14:03:29, Petr Mladek wrote:
> On Wed 2023-04-19 11:11:52, John Ogness wrote:
> > On 2023-04-19, Petr Mladek <pmladek@suse.com> wrote:
> > > it is safe to interrupt one writer now. The preemption still
> > > has to be disabled because the deferred context is CPU specific.
> > 
> > Really it is enough to disable migration.
> 
> True. But it gets too far to my taste. As you describe below.
> It affects all printk's on the CPU.
> 
> Sigh, even the enabled intrrupts might be questionable. For example,
> when the iterrupt is from a watchdog and want's to report a stall.
> 
> > We need to keep an eye on the usage of this function. By allowing
> > interrupts and preemption, it means that other printk's on that CPU will
> > also be deferred if the context interrupted within the deferred block.
> 
> A solution would be to make this more clear in the comment.
> Something like:
> 
> /*
>  * The printk_deferred_enter/exit macros are available only as a hack.
>  * They define a per-CPU context where all printk console printing is
>  * deferred because it might cause a deadlock otherwise.
>  *
>  * The API user is responsible for calling the corresponding enter/exit
>  * pair on the same CPU. It is highly recommended to use them only in
>  * a context with interrupts disabled. Otherwise, other unrelated
>  * printk() calls might be deferred when they interrupt/preempt
>  * the deferred code section.
>  */

This looks better but I would argue that as a potential user of those I
would appreciate less internal implementation details and more
instructions on how/when to use it. What about something like this?

/*
 * The printk_deferred_enter/exit macros are available only as a hack
 * for code paths which are prone to printk related deadlocks. That
 * might be caused by locking context around printk which can be reused
 * directly or indirectly by lower level printk infrastructure.
 * 
 * Any new use of these MUST be consulted with printk maintainers as the
 * use might have some unexpected side effects on the printk
 * infrastructure.
 *
 * enter/exit pair must be called from the same CPU without any
 * preemption in between.
 */
-- 
Michal Hocko
SUSE Labs

Re: [PATCH] printk: Enough to disable preemption in printk deferred context

[John Ogness]

On 2023-04-19, Petr Mladek <pmladek@suse.com> wrote:
> A solution would be to make this more clear in the comment.
> Something like:
>
> /*
>  * The printk_deferred_enter/exit macros are available only as a hack.
>  * They define a per-CPU context where all printk console printing
>  * is deferred because it might cause a deadlock otherwise.
>  *
>  * The API user is responsible for calling the corresponding enter/exit
>  * pair on the same CPU. It is highly recommended to use them only in
>  * a context with interrupts disabled. Otherwise, other unrelated
>  * printk() calls might be deferred when they interrupt/preempt
>  * the deferred code section.
>  */

I an happy with this comment. I saw Michal's follow-up suggestion, but
would prefer this one. It is a more technical desciption of the issue
and clearly recommends that the user should disable interrupts.

If you use this comment:

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

> Another solution would be to stay on the "safe" side and keep the
> comment as is or even enforce disabling interrupts by the API.
>
> I would personally just improve the comment. It is good to describe
> the situation correctly. We could always add restrictions when
> there are problems in practice.

Agreed.

John