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

Groups > linux.kernel > #1280103 > unrolled thread

Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer

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: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer yalin wang <yalin.wang2010@gmail.com> - 2015-11-30 17:50 +0100
    RE: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary  buffer David Laight <David.Laight@ACULAB.COM> - 2015-12-02 17:30 +0100
      Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary  buffer Petr Mladek <pmladek@suse.com> - 2015-12-04 16:50 +0100
    Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary  buffer Petr Mladek <pmladek@suse.com> - 2015-12-07 15:20 +0100

#1280103 — Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer

> On Nov 27, 2015, at 19:09, Petr Mladek <pmladek@suse.com> wrote:
> 
> Testing has shown that the backtrace sometimes does not fit
> into the 4kB temporary buffer that is used in NMI context.
> 
> The warnings are gone when I double the temporary buffer size.
> 
> Note that this problem existed even in the x86-specific
> implementation that was added by the commit a9edc8809328
> ("x86/nmi: Perform a safe NMI stack trace on all CPUs").
> Nobody noticed it because it did not print any warnings.
> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/printk/nmi.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> index 8af1e4016719..6111644d5f01 100644
> --- a/kernel/printk/nmi.c
> +++ b/kernel/printk/nmi.c
> @@ -42,7 +42,7 @@ atomic_t nmi_message_lost;
> struct nmi_seq_buf {
> 	atomic_t		len;	/* length of written data */
> 	struct irq_work		work;	/* IRQ work that flushes the buffer */
> -	unsigned char		buffer[PAGE_SIZE - sizeof(atomic_t) -
> +	unsigned char		buffer[2 * PAGE_SIZE - sizeof(atomic_t) -
> 				       sizeof(struct irq_work)];
> };
> 

why not define like this:

union {
struct {atomic_t		len;	
	struct irq_work		work;
}
unsigned char		buffer[PAGE_SIZE * 2] ;
}

we can make sure the union is 2 PAGE_SIZE .

Thanks

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1281984 — RE: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer

RnJvbTogeWFsaW4gd2FuZw0KPiBTZW50OiAzMCBOb3ZlbWJlciAyMDE1IDE2OjQyDQo+ID4gT24g
Tm92IDI3LCAyMDE1LCBhdCAxOTowOSwgUGV0ciBNbGFkZWsgPHBtbGFkZWtAc3VzZS5jb20+IHdy
b3RlOg0KPiA+DQo+ID4gVGVzdGluZyBoYXMgc2hvd24gdGhhdCB0aGUgYmFja3RyYWNlIHNvbWV0
aW1lcyBkb2VzIG5vdCBmaXQNCj4gPiBpbnRvIHRoZSA0a0IgdGVtcG9yYXJ5IGJ1ZmZlciB0aGF0
IGlzIHVzZWQgaW4gTk1JIGNvbnRleHQuDQo+ID4NCj4gPiBUaGUgd2FybmluZ3MgYXJlIGdvbmUg
d2hlbiBJIGRvdWJsZSB0aGUgdGVtcG9yYXJ5IGJ1ZmZlciBzaXplLg0KDQpZb3UgYXJlIHdhc3Rp
bmcgYSBsb3Qgb2YgbWVtb3J5IGZvciBzb21ldGhpbmcgdGhhdCBpcyBpbmZyZXF1ZW50bHkgdXNl
ZC4NClRoZXJlIG91Z2h0IHRvIGJlIHNvbWUgd2F5IG9mIGNvcHlpbmcgcGFydGlhbCB0cmFjZWJh
Y2tzIGludG8gdGhlDQptYWluIGJ1ZmZlci4NCg0KCURhdmlkDQoNCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1283951 — Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer

On Wed 2015-12-02 16:20:41, David Laight wrote:
> From: yalin wang
> > Sent: 30 November 2015 16:42
> > > On Nov 27, 2015, at 19:09, Petr Mladek <pmladek@suse.com> wrote:
> > >
> > > Testing has shown that the backtrace sometimes does not fit
> > > into the 4kB temporary buffer that is used in NMI context.
> > >
> > > The warnings are gone when I double the temporary buffer size.
> 
> You are wasting a lot of memory for something that is infrequently used.
> There ought to be some way of copying partial tracebacks into the
> main buffer.

I have already tried to use a separate ring buffer that might be
shared between all CPUs. But it was rejected because it was too
complex. See
http://thread.gmane.org/gmane.linux.kernel/1700059/focus=1700066

If we would want to crate a lockless access to the main ring
buffer, we would end up with something like
kernel/trace/ring_buffer.c. It is even more complicated.
And reading of the messages is pretty slow.

Note that we already have this buffer allocated on x86 and arm.
It is used there for printing backtrace from all CPUs.
This patchset makes it usable for all NMI messages.

But I'll make the size configurable in the next version.

Thanks for review,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1285368 — Re: [PATCH v2 5/5] printk/nmi: Increase the size of the temporary buffer

On Mon 2015-11-30 08:42:04, yalin wang wrote:
> 
> > On Nov 27, 2015, at 19:09, Petr Mladek <pmladek@suse.com> wrote:
> > 
> > Testing has shown that the backtrace sometimes does not fit
> > into the 4kB temporary buffer that is used in NMI context.
> > 
> > The warnings are gone when I double the temporary buffer size.
> > 
> > Note that this problem existed even in the x86-specific
> > implementation that was added by the commit a9edc8809328
> > ("x86/nmi: Perform a safe NMI stack trace on all CPUs").
> > Nobody noticed it because it did not print any warnings.
> > 
> > Signed-off-by: Petr Mladek <pmladek@suse.com>
> > ---
> > kernel/printk/nmi.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/printk/nmi.c b/kernel/printk/nmi.c
> > index 8af1e4016719..6111644d5f01 100644
> > --- a/kernel/printk/nmi.c
> > +++ b/kernel/printk/nmi.c
> > @@ -42,7 +42,7 @@ atomic_t nmi_message_lost;
> > struct nmi_seq_buf {
> > 	atomic_t		len;	/* length of written data */
> > 	struct irq_work		work;	/* IRQ work that flushes the buffer */
> > -	unsigned char		buffer[PAGE_SIZE - sizeof(atomic_t) -
> > +	unsigned char		buffer[2 * PAGE_SIZE - sizeof(atomic_t) -
> > 				       sizeof(struct irq_work)];
> > };
> > 
> 
> why not define like this:
> 
> union {
> struct {atomic_t		len;	
> 	struct irq_work		work;
> }
> unsigned char		buffer[PAGE_SIZE * 2] ;
> }
> 
> we can make sure the union is 2 PAGE_SIZE .

IMHO, this would add more confusion. It would just move the
computation somewhere else. The union will have 2*PAGE_SIZE
but the beginning of "buffer" will be shared with "len"
and "work". Therefore, we would need to skip the beginning
of the buffer when storing the data. By other words, we still
will be able to use only (sizeof(buffer) - sizeof(atomic_t) -
sizeof(struct irq_work)] of the "buffer".

Or did I miss something, please?

Best Regards,
Petr
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web