Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1571685 > unrolled thread

Re: [PATCHv7 4/8] printk: always use deferred printk when flush printk_safe lines

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Steven Rostedt <rostedt@goodmis.org> - 2017-02-01 17:10 +0100
    Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-02-02 07:10 +0100
      Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Petr Mladek <pmladek@suse.com> - 2017-02-02 16:40 +0100
        Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Petr Mladek <pmladek@suse.com> - 2017-02-02 17:00 +0100
          Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-02-03 03:10 +0100
            Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Petr Mladek <pmladek@suse.com> - 2017-02-03 12:20 +0100
              Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-02-06 02:50 +0100
                Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-02-06 03:10 +0100
                  Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Petr Mladek <pmladek@suse.com> - 2017-02-06 13:20 +0100
                    Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Steven Rostedt <rostedt@goodmis.org> - 2017-02-06 14:30 +0100
                      Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Sergey Senozhatsky <sergey.senozhatsky@gmail.com> - 2017-02-06 17:50 +0100
                        Re: [PATCHv7 4/8] printk: always use deferred printk when flush  printk_safe lines Steven Rostedt <rostedt@goodmis.org> - 2017-02-06 18:00 +0100

#1571685 — Re: [PATCHv7 4/8] printk: always use deferred printk when flush printk_safe lines

On Tue, 27 Dec 2016 23:16:07 +0900
Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> Always use printk_deferred() in printk_safe_flush_line().
> Flushing can be done from NMI or printk_safe contexts (when
> we are in panic), so we can't call console drivers, yet still
> want to store the messages in the logbuf buffer. Therefore we
> use a deferred printk version.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> ---
>  kernel/printk/printk_safe.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index efc89a4e9df5..054c9128ade0 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -113,14 +113,12 @@ static int printk_safe_log_store(struct printk_safe_seq_buf *s,
>  static void printk_safe_flush_line(const char *text, int len)
>  {
>  	/*
> -	 * The buffers are flushed in NMI only on panic.  The messages must
> -	 * go only into the ring buffer at this stage.  Consoles will get
> -	 * explicitly called later when a crashdump is not generated.
> +	 * Avoid any console drivers calls from here, because we may be
> +	 * in NMI or printk_safe context (when in panic). The messages
> +	 * must go only into the ring buffer at this stage.  Consoles will
> +	 * get explicitly called later when a crashdump is not generated.
>  	 */
> -	if (in_nmi())
> -		printk_deferred("%.*s", len, text);
> -	else
> -		printk("%.*s", len, text);
> +	printk_deferred("%.*s", len, text);
>  }

The helper function was there because of the if statement. Just nuke
this function and call printk_deferred() directly. You can move the
comment to the caller.

-- Steve

>  
>  /* printk part of the temporary buffer line by line */

[toc] | [next] | [standalone]

#1572155

On (02/01/17 11:06), Steven Rostedt wrote:
[..]
> >  static void printk_safe_flush_line(const char *text, int len)
> >  {
> >  	/*
> > -	 * The buffers are flushed in NMI only on panic.  The messages must
> > -	 * go only into the ring buffer at this stage.  Consoles will get
> > -	 * explicitly called later when a crashdump is not generated.
> > +	 * Avoid any console drivers calls from here, because we may be
> > +	 * in NMI or printk_safe context (when in panic). The messages
> > +	 * must go only into the ring buffer at this stage.  Consoles will
> > +	 * get explicitly called later when a crashdump is not generated.
> >  	 */
> > -	if (in_nmi())
> > -		printk_deferred("%.*s", len, text);
> > -	else
> > -		printk("%.*s", len, text);
> > +	printk_deferred("%.*s", len, text);
> >  }
> 
> The helper function was there because of the if statement. Just nuke
> this function and call printk_deferred() directly. You can move the
> comment to the caller.

indeed. thanks.

Petr, how would you prefer to handle this?

	-ss

[toc] | [prev] | [next] | [standalone]

#1572474

On Thu 2017-02-02 15:05:38, Sergey Senozhatsky wrote:
> On (02/01/17 11:06), Steven Rostedt wrote:
> [..]
> > >  static void printk_safe_flush_line(const char *text, int len)
> > >  {
> > >  	/*
> > > -	 * The buffers are flushed in NMI only on panic.  The messages must
> > > -	 * go only into the ring buffer at this stage.  Consoles will get
> > > -	 * explicitly called later when a crashdump is not generated.
> > > +	 * Avoid any console drivers calls from here, because we may be
> > > +	 * in NMI or printk_safe context (when in panic). The messages
> > > +	 * must go only into the ring buffer at this stage.  Consoles will
> > > +	 * get explicitly called later when a crashdump is not generated.
> > >  	 */
> > > -	if (in_nmi())
> > > -		printk_deferred("%.*s", len, text);
> > > -	else
> > > -		printk("%.*s", len, text);
> > > +	printk_deferred("%.*s", len, text);
> > >  }
> > 
> > The helper function was there because of the if statement. Just nuke
> > this function and call printk_deferred() directly. You can move the
> > comment to the caller.
> 
> indeed. thanks.
> 
> Petr, how would you prefer to handle this?

I am still learning how to manage the patches in the printk.git tree.

I would like to add Steven's Reviewed-by tags, links to the mails,
and list of CC-ed people. IMHO, the only way is to rebase the for-4.11
and for-next branches. Then I could do the above suggested change during
the rebase.

Any other solutions are welcome. I will wait with the rebase at least
until Monday. We are discussing the lockdep in the 6th patch anyway.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1572484

On Thu 2017-02-02 16:37:53, Petr Mladek wrote:
> On Thu 2017-02-02 15:05:38, Sergey Senozhatsky wrote:
> > On (02/01/17 11:06), Steven Rostedt wrote:
> > [..]
> > > >  static void printk_safe_flush_line(const char *text, int len)
> > > >  {
> > > >  	/*
> > > > -	 * The buffers are flushed in NMI only on panic.  The messages must
> > > > -	 * go only into the ring buffer at this stage.  Consoles will get
> > > > -	 * explicitly called later when a crashdump is not generated.
> > > > +	 * Avoid any console drivers calls from here, because we may be
> > > > +	 * in NMI or printk_safe context (when in panic). The messages
> > > > +	 * must go only into the ring buffer at this stage.  Consoles will
> > > > +	 * get explicitly called later when a crashdump is not generated.
> > > >  	 */
> > > > -	if (in_nmi())
> > > > -		printk_deferred("%.*s", len, text);
> > > > -	else
> > > > -		printk("%.*s", len, text);
> > > > +	printk_deferred("%.*s", len, text);
> > > >  }
> > > 
> > > The helper function was there because of the if statement. Just nuke
> > > this function and call printk_deferred() directly. You can move the
> > > comment to the caller.
> > 
> > indeed. thanks.
> > 
> > Petr, how would you prefer to handle this?
> 
> I am still learning how to manage the patches in the printk.git tree.
> 
> I would like to add Steven's Reviewed-by tags, links to the mails,
> and list of CC-ed people. IMHO, the only way is to rebase the for-4.11
> and for-next branches. Then I could do the above suggested change during
> the rebase.

Alternatively you could send this other solution as a replay to this
thread. Then we could comment, add reviewed tags. And I could use
the new approved variant during the rebase.

This is probably better approach for non-oneliner changes.

> Any other solutions are welcome. I will wait with the rebase at least
> until Monday. We are discussing the lockdep in the 6th patch anyway.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1572853

On (02/02/17 16:52), Petr Mladek wrote:
[..]
> > I am still learning how to manage the patches in the printk.git tree.
> > 
> > I would like to add Steven's Reviewed-by tags, links to the mails,
> > and list of CC-ed people. IMHO, the only way is to rebase the for-4.11
> > and for-next branches. Then I could do the above suggested change during
> > the rebase.
> 
> Alternatively you could send this other solution as a replay to this
> thread. Then we could comment, add reviewed tags. And I could use
> the new approved variant during the rebase.
> 
> This is probably better approach for non-oneliner changes.

by "the new approved variant" do you mean resend of the entire
patch set (v8) or just 0004 patch?

	-ss

[toc] | [prev] | [next] | [standalone]

#1573033

On Fri 2017-02-03 11:04:18, Sergey Senozhatsky wrote:
> On (02/02/17 16:52), Petr Mladek wrote:
> [..]
> > > I am still learning how to manage the patches in the printk.git tree.
> > > 
> > > I would like to add Steven's Reviewed-by tags, links to the mails,
> > > and list of CC-ed people. IMHO, the only way is to rebase the for-4.11
> > > and for-next branches. Then I could do the above suggested change during
> > > the rebase.
> > 
> > Alternatively you could send this other solution as a replay to this
> > thread. Then we could comment, add reviewed tags. And I could use
> > the new approved variant during the rebase.
> > 
> > This is probably better approach for non-oneliner changes.
> 
> by "the new approved variant" do you mean resend of the entire
> patch set (v8) or just 0004 patch?

I depends on how many and how complicated are possible conflicts
with the followup patches.

It is perfectly fine to send just 0004 if there are no conflicts
or if they are trivial to resolve.

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1574321

On (02/03/17 12:18), Petr Mladek wrote:
[..]
> > by "the new approved variant" do you mean resend of the entire
> > patch set (v8) or just 0004 patch?
> 
> I depends on how many and how complicated are possible conflicts
> with the followup patches.
> 
> It is perfectly fine to send just 0004 if there are no conflicts
> or if they are trivial to resolve.

so there are no conflicts at all. 0004 can be replaced with the patch below.
but the thing is - I want to keep printk_safe_flush_line() around.

we call

static void printk_safe_flush_line(const char *text, int len)
{
	/*
	 * Avoid any console drivers calls from here, because we may be
	 * in NMI or printk_safe context (when in panic). The messages
	 * must go only into the ring buffer at this stage.  Consoles will
	 * get explicitly called later when a crashdump is not generated.
	 */
	printk_deferred("%.*s", len, text);
}

from many places:

  4x   printk_safe_flush_buffer()
  1x   __printk_safe_flush()


replacing printk_safe_flush_line() with printk_deferred() produces things
like these

               printk_deferred("%.*s", end - start, start);
               printk_deferred("%.*s", strlen(newline), newline);

which, I think, makes the code worse. any objections?

just in case, the patch (which I prefer to be ignored)

===8<======8<===

From daa9bd8c2659a91036c00e22bb218be6871a371d Mon Sep 17 00:00:00 2001
From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Date: Tue, 27 Dec 2016 23:16:07 +0900
Subject: [PATCH] printk: always use deferred printk when flush printk_safe
 lines

Always use printk_deferred() in printk_safe_flush_line().
Flushing can be done from NMI or printk_safe contexts (when
we are in panic), so we can't call console drivers, yet still
want to store the messages in the logbuf buffer. Therefore we
use a deferred printk version.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Suggested-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk_safe.c | 30 ++++++++++++------------------
 1 file changed, 12 insertions(+), 18 deletions(-)

diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index efc89a4e9df5..801f0f1c7547 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -110,19 +110,6 @@ static int printk_safe_log_store(struct printk_safe_seq_buf *s,
 	return add;
 }
 
-static void printk_safe_flush_line(const char *text, int len)
-{
-	/*
-	 * The buffers are flushed in NMI only on panic.  The messages must
-	 * go only into the ring buffer at this stage.  Consoles will get
-	 * explicitly called later when a crashdump is not generated.
-	 */
-	if (in_nmi())
-		printk_deferred("%.*s", len, text);
-	else
-		printk("%.*s", len, text);
-}
-
 /* printk part of the temporary buffer line by line */
 static int printk_safe_flush_buffer(const char *start, size_t len)
 {
@@ -136,7 +123,14 @@ static int printk_safe_flush_buffer(const char *start, size_t len)
 	/* Print line by line. */
 	while (c < end) {
 		if (*c == '\n') {
-			printk_safe_flush_line(start, c - start + 1);
+			/*
+			 * Avoid any console drivers calls from here, because
+			 * we may be in NMI or printk_safe context (when in
+			 * panic). The messages must go only into the ring
+			 * buffer at this stage.  Consoles will get explicitly
+			 * called later when a crashdump is not generated.
+			 */
+			printk_deferred("%.*s", c - start + 1, start);
 			start = ++c;
 			header = true;
 			continue;
@@ -149,7 +143,7 @@ static int printk_safe_flush_buffer(const char *start, size_t len)
 				continue;
 			}
 
-			printk_safe_flush_line(start, c - start);
+			printk_deferred("%.*s", c - start, start);
 			start = c++;
 			header = true;
 			continue;
@@ -163,8 +157,8 @@ static int printk_safe_flush_buffer(const char *start, size_t len)
 	if (start < end && !header) {
 		static const char newline[] = KERN_CONT "\n";
 
-		printk_safe_flush_line(start, end - start);
-		printk_safe_flush_line(newline, strlen(newline));
+		printk_deferred("%.*s", end - start, start);
+		printk_deferred("%.*s", strlen(newline), newline);
 	}
 
 	return len;
@@ -206,7 +200,7 @@ static void __printk_safe_flush(struct irq_work *work)
 	if ((i && i >= len) || len > sizeof(s->buffer)) {
 		const char *msg = "printk_safe_flush: internal error\n";
 
-		printk_safe_flush_line(msg, strlen(msg));
+		printk_deferred("%.*s", strlen(msg), msg);
 		len = 0;
 	}
 
-- 
2.11.1

[toc] | [prev] | [next] | [standalone]

#1574324

On (02/06/17 10:48), Sergey Senozhatsky wrote:
[..]
> from many places:
> 
>   4x   printk_safe_flush_buffer()
>   1x   __printk_safe_flush()
> 
> 
> replacing printk_safe_flush_line() with printk_deferred() produces things
> like these
> 
>                printk_deferred("%.*s", end - start, start);
>                printk_deferred("%.*s", strlen(newline), newline);

ah... besides we need to cast length argument to "int".


> just in case, the patch (which I prefer to be ignored)

let's keep printk_safe_flush_line().

	-ss

[toc] | [prev] | [next] | [standalone]

#1574600

On Mon 2017-02-06 11:08:20, Sergey Senozhatsky wrote:
> On (02/06/17 10:48), Sergey Senozhatsky wrote:
> [..]
> > from many places:
> > 
> >   4x   printk_safe_flush_buffer()
> >   1x   __printk_safe_flush()
> > 
> > 
> > replacing printk_safe_flush_line() with printk_deferred() produces things
> > like these
> > 
> >                printk_deferred("%.*s", end - start, start);
> >                printk_deferred("%.*s", strlen(newline), newline);
> 
> ah... besides we need to cast length argument to "int".

Good point.


> > just in case, the patch (which I prefer to be ignored)
> 
> let's keep printk_safe_flush_line().

I do not have strong opinion but I would slightly prefer
to keep the helper function. The use of printk_deferred()
is a bit tricky and it is better to have only one copy.

Steven, could you live with the original patch, please?

Best Regards,
Petr

[toc] | [prev] | [next] | [standalone]

#1574680

On Mon, 6 Feb 2017 13:16:52 +0100
Petr Mladek <pmladek@suse.com> wrote:

 
> > > just in case, the patch (which I prefer to be ignored)  
> > 
> > let's keep printk_safe_flush_line().  
> 
> I do not have strong opinion but I would slightly prefer
> to keep the helper function. The use of printk_deferred()
> is a bit tricky and it is better to have only one copy.
> 
> Steven, could you live with the original patch, please?

Sure, but make it into a static inline.

-- Steve

[toc] | [prev] | [next] | [standalone]

#1574976

On (02/06/17 08:27), Steven Rostedt wrote:
[..]
> > > > just in case, the patch (which I prefer to be ignored)  
> > > 
> > > let's keep printk_safe_flush_line().  
> > 
> > I do not have strong opinion but I would slightly prefer
> > to keep the helper function. The use of printk_deferred()
> > is a bit tricky and it is better to have only one copy.
> > 
> > Steven, could you live with the original patch, please?
> 
> Sure, but make it into a static inline.

well, I'll be surprised if gcc doesn't inline that simple one-liner.

attached. no conflicts, the patch can replace 0004.

===8<===8<===

From 69bbb0f436a2a89ec41a5831c03490e0a78ce12e Mon Sep 17 00:00:00 2001
From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Date: Tue, 27 Dec 2016 23:16:07 +0900
Subject: [PATCH] printk: always use deferred printk when flush printk_safe
 lines

Always use printk_deferred() in printk_safe_flush_line().
Flushing can be done from NMI or printk_safe contexts (when
we are in panic), so we can't call console drivers, yet still
want to store the messages in the logbuf buffer. Therefore we
use a deferred printk version.

Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
Suggested-by: Petr Mladek <pmladek@suse.com>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/printk/printk_safe.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
index efc89a4e9df5..5214d326d3ba 100644
--- a/kernel/printk/printk_safe.c
+++ b/kernel/printk/printk_safe.c
@@ -110,17 +110,15 @@ static int printk_safe_log_store(struct printk_safe_seq_buf *s,
 	return add;
 }
 
-static void printk_safe_flush_line(const char *text, int len)
+static inline void printk_safe_flush_line(const char *text, int len)
 {
 	/*
-	 * The buffers are flushed in NMI only on panic.  The messages must
-	 * go only into the ring buffer at this stage.  Consoles will get
-	 * explicitly called later when a crashdump is not generated.
+	 * Avoid any console drivers calls from here, because we may be
+	 * in NMI or printk_safe context (when in panic). The messages
+	 * must go only into the ring buffer at this stage.  Consoles will
+	 * get explicitly called later when a crashdump is not generated.
 	 */
-	if (in_nmi())
-		printk_deferred("%.*s", len, text);
-	else
-		printk("%.*s", len, text);
+	printk_deferred("%.*s", len, text);
 }
 
 /* printk part of the temporary buffer line by line */
-- 
2.11.0

[toc] | [prev] | [next] | [standalone]

#1574987

On Tue, 7 Feb 2017 01:42:53 +0900
Sergey Senozhatsky <sergey.senozhatsky@gmail.com> wrote:

> On (02/06/17 08:27), Steven Rostedt wrote:
> [..]
> > > > > just in case, the patch (which I prefer to be ignored)    
> > > > 
> > > > let's keep printk_safe_flush_line().    
> > > 
> > > I do not have strong opinion but I would slightly prefer
> > > to keep the helper function. The use of printk_deferred()
> > > is a bit tricky and it is better to have only one copy.
> > > 
> > > Steven, could you live with the original patch, please?  
> > 
> > Sure, but make it into a static inline.  
> 
> well, I'll be surprised if gcc doesn't inline that simple one-liner.

I never trust gcc ;-)  It's been known to uninline simple one liners
before that have been marked as "inline" :-p

> 
> attached. no conflicts, the patch can replace 0004.
> 
> ===8<===8<===
> 
> >From 69bbb0f436a2a89ec41a5831c03490e0a78ce12e Mon Sep 17 00:00:00 2001  
> From: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Date: Tue, 27 Dec 2016 23:16:07 +0900
> Subject: [PATCH] printk: always use deferred printk when flush printk_safe
>  lines
> 
> Always use printk_deferred() in printk_safe_flush_line().
> Flushing can be done from NMI or printk_safe contexts (when
> we are in panic), so we can't call console drivers, yet still
> want to store the messages in the logbuf buffer. Therefore we
> use a deferred printk version.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> Suggested-by: Petr Mladek <pmladek@suse.com>
> Signed-off-by: Petr Mladek <pmladek@suse.com>

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  kernel/printk/printk_safe.c | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/printk/printk_safe.c b/kernel/printk/printk_safe.c
> index efc89a4e9df5..5214d326d3ba 100644
> --- a/kernel/printk/printk_safe.c
> +++ b/kernel/printk/printk_safe.c
> @@ -110,17 +110,15 @@ static int printk_safe_log_store(struct printk_safe_seq_buf *s,
>  	return add;
>  }
>  
> -static void printk_safe_flush_line(const char *text, int len)
> +static inline void printk_safe_flush_line(const char *text, int len)
>  {
>  	/*
> -	 * The buffers are flushed in NMI only on panic.  The messages must
> -	 * go only into the ring buffer at this stage.  Consoles will get
> -	 * explicitly called later when a crashdump is not generated.
> +	 * Avoid any console drivers calls from here, because we may be
> +	 * in NMI or printk_safe context (when in panic). The messages
> +	 * must go only into the ring buffer at this stage.  Consoles will
> +	 * get explicitly called later when a crashdump is not generated.
>  	 */
> -	if (in_nmi())
> -		printk_deferred("%.*s", len, text);
> -	else
> -		printk("%.*s", len, text);
> +	printk_deferred("%.*s", len, text);
>  }
>  
>  /* printk part of the temporary buffer line by line */

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web