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

Groups > linux.kernel > #1571712 > unrolled thread

Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi contexts

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 5/8] printk: report lost messages in printk safe/nmi  contexts Steven Rostedt <rostedt@goodmis.org> - 2017-02-01 17:40 +0100
    Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi  contexts Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-02-02 03:10 +0100
      Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi  contexts Steven Rostedt <rostedt@goodmis.org> - 2017-02-02 15:40 +0100
        Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi  contexts Petr Mladek <pmladek@suse.com> - 2017-02-02 17:20 +0100
        Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi  contexts Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2017-02-03 03:00 +0100
          Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi  contexts Steven Rostedt <rostedt@goodmis.org> - 2017-02-03 04:20 +0100

#1571712 — Re: [PATCHv7 5/8] printk: report lost messages in printk safe/nmi contexts

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

> Account lost messages in pritk-safe and printk-safe-nmi
> contexts and report those numbers during printk_safe_flush().
> 
> The patch also moves lost message counter to struct
> `printk_safe_seq_buf' instead of having dedicated static
> counters - this simplifies the code.
> 
> Signed-off-by: Sergey Senozhatsky <sergey.senozhatsky@gmail.com>
> ---

This looks fine, but I'm curious if you tested it. That is, added a
bunch of printks to overflow the buffer. IIRC, I did it to the original
nmi code. If you haven't you may want to just to make sure it works. I
can also test it too if I get some time.

-- Steve

[toc] | [next] | [standalone]

#1572118

On (02/01/17 11:37), Steven Rostedt wrote:
> This looks fine, but I'm curious if you tested it. That is, added a
> bunch of printks to overflow the buffer. IIRC, I did it to the original
> nmi code. If you haven't you may want to just to make sure it works. I
> can also test it too if I get some time.

Hello Steven,

yes, I have tested it on my x86 box.

	-ss

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

#1572408

On Thu, 2 Feb 2017 11:02:57 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:

> On (02/01/17 11:37), Steven Rostedt wrote:
> > This looks fine, but I'm curious if you tested it. That is, added a
> > bunch of printks to overflow the buffer. IIRC, I did it to the original
> > nmi code. If you haven't you may want to just to make sure it works. I
> > can also test it too if I get some time.  
> 
> Hello Steven,
> 
> yes, I have tested it on my x86 box.
> 

Please note how you tested it in your change log. It's not really a
requirement, and you don't really have to do it. But it helps people
have warm fuzzies about the code. I'm trying to do this in the future
in my own work too.

-- Steve

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

#1572494

On Thu 2017-02-02 09:34:09, Steven Rostedt wrote:
> On Thu, 2 Feb 2017 11:02:57 +0900
> Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> 
> > On (02/01/17 11:37), Steven Rostedt wrote:
> > > This looks fine, but I'm curious if you tested it. That is, added a
> > > bunch of printks to overflow the buffer. IIRC, I did it to the original
> > > nmi code. If you haven't you may want to just to make sure it works. I
> > > can also test it too if I get some time.  

I have tested it and actually found a bug. I was lucky because it was
a cornercase. It is already fixed upstream by the commit
4a998e322abc935e ("printk/NMI: fix up handling of the full nmi
log buffer").

> Please note how you tested it in your change log. It's not really a
> requirement, and you don't really have to do it. But it helps people
> have warm fuzzies about the code. I'm trying to do this in the future
> in my own work too.

Goot to know.

Best Regards,
Petr

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

#1572850

On (02/02/17 09:34), Steven Rostedt wrote:
> On Thu, 2 Feb 2017 11:02:57 +0900
> Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
> 
> > On (02/01/17 11:37), Steven Rostedt wrote:
> > > This looks fine, but I'm curious if you tested it. That is, added a
> > > bunch of printks to overflow the buffer. IIRC, I did it to the original
> > > nmi code. If you haven't you may want to just to make sure it works. I
> > > can also test it too if I get some time.
> > 
> > Hello Steven,
> > 
> > yes, I have tested it on my x86 box.
> > 
> 
> Please note how you tested it in your change log. It's not really a
> requirement, and you don't really have to do it. But it helps people
> have warm fuzzies about the code. I'm trying to do this in the future
> in my own work too.

well. sure, no objections, but in this particular case it's hardly possible
to hit all of those problems within a reasonable time. so what I did was just
a bunch of dirty hacks to provoke the problems. something like this

	extern int XXX;

	vprintk_emit()
	{
		spin_lock(&logbuf_lock);

			//
			// in various places
			//
			if (XXX == 1) {
				WARN_ON(1);
			}

		spin_unlock(&logbuf_lock);
	}

and I set/clear that XXX from one of sysfs attrs... yeah, I know...
but it does what I want. so I'm not really sure I want to note this
in the change log. am I wrong?

	-ss

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

#1572865

On Fri, 3 Feb 2017 10:57:57 +0900
Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> wrote:
ell. sure, no objections, but in this particular case it's hardly
possible
> to hit all of those problems within a reasonable time. so what I did was just
> a bunch of dirty hacks to provoke the problems. something like this
> 
> 	extern int XXX;
> 
> 	vprintk_emit()
> 	{
> 		spin_lock(&logbuf_lock);
> 
> 			//
> 			// in various places
> 			//
> 			if (XXX == 1) {
> 				WARN_ON(1);
> 			}
> 
> 		spin_unlock(&logbuf_lock);
> 	}
> 
> and I set/clear that XXX from one of sysfs attrs... yeah, I know...
> but it does what I want. so I'm not really sure I want to note this
> in the change log. am I wrong?

No you are not. But just seeing this in email does help. And yeah, this
is the type of testing that I do as well on things like this. No need
to put it in the change log.

-- Steve

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web