[PATCH 0/2] /proc/stat vs. failed order-4 allocation

[PATCH 0/2] /proc/stat vs. failed order-4 allocation

[Heiko Carstens]

These two patches are supposed to "fix" failed order-4 memory
allocations which have been observed when reading /proc/stat.
The problem has been observed on s390 as well as on x86.

To address the problem change the seq_file memory allocations to
fallback to use vmalloc, so that allocations also work if memory
is fragmented.

This approach seems to be simpler and less intrusive than changing
/proc/stat to use an interator. Also it "fixes" other users as well,
which use seq_file's single_open() interface.

Heiko Carstens (2):
  proc/stat: convert to single_open_size()
  fs/seq_file: fallback to vmalloc allocation

 fs/proc/stat.c | 22 ++--------------------
 fs/seq_file.c  | 30 +++++++++++++++++++++---------
 2 files changed, 23 insertions(+), 29 deletions(-)

-- 
1.8.5.5

[PATCH 1/2] proc/stat: convert to single_open_size()

[Heiko Carstens]

Use seq_file's single_open_size() to preallocate a buffer that is large
enough to hold the whole output, instead of open coding it.
Also calculate the requested size using the number of online cpus instead
of possible cpus, since the size of the output only depends on the number
of online cpus.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/proc/stat.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 9d231e9e5f0e..bf2d03f8fd3e 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -184,29 +184,11 @@ static int show_stat(struct seq_file *p, void *v)
 
 static int stat_open(struct inode *inode, struct file *file)
 {
-	size_t size = 1024 + 128 * num_possible_cpus();
-	char *buf;
-	struct seq_file *m;
-	int res;
+	size_t size = 1024 + 128 * num_online_cpus();
 
 	/* minimum size to display an interrupt count : 2 bytes */
 	size += 2 * nr_irqs;
-
-	/* don't ask for more than the kmalloc() max size */
-	if (size > KMALLOC_MAX_SIZE)
-		size = KMALLOC_MAX_SIZE;
-	buf = kmalloc(size, GFP_KERNEL);
-	if (!buf)
-		return -ENOMEM;
-
-	res = single_open(file, show_stat, NULL);
-	if (!res) {
-		m = file->private_data;
-		m->buf = buf;
-		m->size = ksize(buf);
-	} else
-		kfree(buf);
-	return res;
+	return single_open_size(file, show_stat, NULL, size);
 }
 
 static const struct file_operations proc_stat_operations = {
-- 
1.8.5.5

[PATCH 2/2] fs/seq_file: fallback to vmalloc allocation

[Heiko Carstens]

There are a couple of seq_files which use the single_open() interface.
This interface requires that the whole output must fit into a single buffer.

E.g. for /proc/stat allocation failures have been observed because an order-4
memory allocation failed due to memory fragmentation. In such situations
reading /proc/stat is not possible anymore.

Therefore change the seq_file code to fallback to vmalloc allocations which
will usually result in a couple of order-0 allocations and hence also work
if memory is fragmented.

For reference a call trace where reading from /proc/stat failed:

[62129.701569] sadc: page allocation failure: order:4, mode:0x1040d0
[62129.701573] CPU: 1 PID: 192063 Comm: sadc Not tainted 3.10.0-123.el7.s390x #1
[...]
[62129.701586] Call Trace:
[62129.701588] ([<0000000000111fbe>] show_trace+0xe6/0x130)
[62129.701591] [<0000000000112074>] show_stack+0x6c/0xe8
[62129.701593] [<000000000020d356>] warn_alloc_failed+0xd6/0x138
[62129.701596] [<00000000002114d2>] __alloc_pages_nodemask+0x9da/0xb68
[62129.701598] [<000000000021168e>] __get_free_pages+0x2e/0x58
[62129.701599] [<000000000025a05c>] kmalloc_order_trace+0x44/0xc0
[62129.701602] [<00000000002f3ffa>] stat_open+0x5a/0xd8
[62129.701604] [<00000000002e9aaa>] proc_reg_open+0x8a/0x140
[62129.701606] [<0000000000273b64>] do_dentry_open+0x1bc/0x2c8
[62129.701608] [<000000000027411e>] finish_open+0x46/0x60
[62129.701610] [<000000000028675a>] do_last+0x382/0x10d0
[62129.701612] [<0000000000287570>] path_openat+0xc8/0x4f8
[62129.701614] [<0000000000288bde>] do_filp_open+0x46/0xa8
[62129.701616] [<000000000027541c>] do_sys_open+0x114/0x1f0
[62129.701618] [<00000000005b1c1c>] sysc_tracego+0x14/0x1a

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 fs/seq_file.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 1d641bb108d2..3857b720cb1b 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -8,8 +8,10 @@
 #include <linux/fs.h>
 #include <linux/export.h>
 #include <linux/seq_file.h>
+#include <linux/vmalloc.h>
 #include <linux/slab.h>
 #include <linux/cred.h>
+#include <linux/mm.h>
 
 #include <asm/uaccess.h>
 #include <asm/page.h>
@@ -30,6 +32,16 @@ static void seq_set_overflow(struct seq_file *m)
 	m->count = m->size;
 }
 
+static void *seq_buf_alloc(unsigned long size)
+{
+	void *buf;
+
+	buf = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
+	if (!buf && size > PAGE_SIZE)
+		buf = vmalloc(size);
+	return buf;
+}
+
 /**
  *	seq_open -	initialize sequential file
  *	@file: file we initialize
@@ -96,7 +108,7 @@ static int traverse(struct seq_file *m, loff_t offset)
 		return 0;
 	}
 	if (!m->buf) {
-		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
+		m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
 		if (!m->buf)
 			return -ENOMEM;
 	}
@@ -135,9 +147,9 @@ static int traverse(struct seq_file *m, loff_t offset)
 
 Eoverflow:
 	m->op->stop(m, p);
-	kfree(m->buf);
+	kvfree(m->buf);
 	m->count = 0;
-	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+	m->buf = seq_buf_alloc(m->size <<= 1);
 	return !m->buf ? -ENOMEM : -EAGAIN;
 }
 
@@ -192,7 +204,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 
 	/* grab buffer if we didn't have one */
 	if (!m->buf) {
-		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
+		m->buf = seq_buf_alloc(m->size = PAGE_SIZE);
 		if (!m->buf)
 			goto Enomem;
 	}
@@ -232,9 +244,9 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 		if (m->count < m->size)
 			goto Fill;
 		m->op->stop(m, p);
-		kfree(m->buf);
+		kvfree(m->buf);
 		m->count = 0;
-		m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+		m->buf = seq_buf_alloc(m->size <<= 1);
 		if (!m->buf)
 			goto Enomem;
 		m->version = 0;
@@ -350,7 +362,7 @@ EXPORT_SYMBOL(seq_lseek);
 int seq_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *m = file->private_data;
-	kfree(m->buf);
+	kvfree(m->buf);
 	kfree(m);
 	return 0;
 }
@@ -605,13 +617,13 @@ EXPORT_SYMBOL(single_open);
 int single_open_size(struct file *file, int (*show)(struct seq_file *, void *),
 		void *data, size_t size)
 {
-	char *buf = kmalloc(size, GFP_KERNEL);
+	char *buf = seq_buf_alloc(size);
 	int ret;
 	if (!buf)
 		return -ENOMEM;
 	ret = single_open(file, show, data);
 	if (ret) {
-		kfree(buf);
+		kvfree(buf);
 		return ret;
 	}
 	((struct seq_file *)file->private_data)->buf = buf;
-- 
1.8.5.5

Re: [PATCH 0/2] /proc/stat vs. failed order-4 allocation

[Andrew Morton]

On Mon, 16 Jun 2014 11:04:50 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> These two patches are supposed to "fix" failed order-4 memory
> allocations which have been observed when reading /proc/stat.
> The problem has been observed on s390 as well as on x86.
> 
> To address the problem change the seq_file memory allocations to
> fallback to use vmalloc, so that allocations also work if memory
> is fragmented.
> 
> This approach seems to be simpler and less intrusive than changing
> /proc/stat to use an interator. Also it "fixes" other users as well,
> which use seq_file's single_open() interface.

Yes, those changes look pretty simple and effective.

I'm unclear on how urgent these fixes are.  I semi-randomly tagged them
for 3.16 with a -stable backport, but that could be changed?

Re: [PATCH 1/2] proc/stat: convert to single_open_size()

[David Rientjes]

On Mon, 16 Jun 2014, Heiko Carstens wrote:

> Use seq_file's single_open_size() to preallocate a buffer that is large
> enough to hold the whole output, instead of open coding it.
> Also calculate the requested size using the number of online cpus instead
> of possible cpus, since the size of the output only depends on the number
> of online cpus.
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Acked-by: David Rientjes <rientjes@google.com>

I have no idea why the check for size > KMALLOC_MAX_SIZE was there, all 
slab allocators will fallback to the page allocator and the slowpath will 
return NULL.

Re: [PATCH 2/2] fs/seq_file: fallback to vmalloc allocation

[David Rientjes]

On Mon, 16 Jun 2014, Heiko Carstens wrote:

> There are a couple of seq_files which use the single_open() interface.
> This interface requires that the whole output must fit into a single buffer.
> 
> E.g. for /proc/stat allocation failures have been observed because an order-4
> memory allocation failed due to memory fragmentation. In such situations
> reading /proc/stat is not possible anymore.
> 
> Therefore change the seq_file code to fallback to vmalloc allocations which
> will usually result in a couple of order-0 allocations and hence also work
> if memory is fragmented.
> 
> For reference a call trace where reading from /proc/stat failed:
> 
> [62129.701569] sadc: page allocation failure: order:4, mode:0x1040d0
> [62129.701573] CPU: 1 PID: 192063 Comm: sadc Not tainted 3.10.0-123.el7.s390x #1
> [...]
> [62129.701586] Call Trace:
> [62129.701588] ([<0000000000111fbe>] show_trace+0xe6/0x130)
> [62129.701591] [<0000000000112074>] show_stack+0x6c/0xe8
> [62129.701593] [<000000000020d356>] warn_alloc_failed+0xd6/0x138
> [62129.701596] [<00000000002114d2>] __alloc_pages_nodemask+0x9da/0xb68
> [62129.701598] [<000000000021168e>] __get_free_pages+0x2e/0x58
> [62129.701599] [<000000000025a05c>] kmalloc_order_trace+0x44/0xc0
> [62129.701602] [<00000000002f3ffa>] stat_open+0x5a/0xd8
> [62129.701604] [<00000000002e9aaa>] proc_reg_open+0x8a/0x140
> [62129.701606] [<0000000000273b64>] do_dentry_open+0x1bc/0x2c8
> [62129.701608] [<000000000027411e>] finish_open+0x46/0x60
> [62129.701610] [<000000000028675a>] do_last+0x382/0x10d0
> [62129.701612] [<0000000000287570>] path_openat+0xc8/0x4f8
> [62129.701614] [<0000000000288bde>] do_filp_open+0x46/0xa8
> [62129.701616] [<000000000027541c>] do_sys_open+0x114/0x1f0
> [62129.701618] [<00000000005b1c1c>] sysc_tracego+0x14/0x1a
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>

Tested-by: David Rientjes <rientjes@google.com>

Very nice!

Re: [PATCH 0/2] /proc/stat vs. failed order-4 allocation

[Heiko Carstens]

On Wed, Jun 18, 2014 at 02:29:31PM -0700, Andrew Morton wrote:
> On Mon, 16 Jun 2014 11:04:50 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > These two patches are supposed to "fix" failed order-4 memory
> > allocations which have been observed when reading /proc/stat.
> > The problem has been observed on s390 as well as on x86.
> > 
> > To address the problem change the seq_file memory allocations to
> > fallback to use vmalloc, so that allocations also work if memory
> > is fragmented.
> > 
> > This approach seems to be simpler and less intrusive than changing
> > /proc/stat to use an interator. Also it "fixes" other users as well,
> > which use seq_file's single_open() interface.
> 
> Yes, those changes look pretty simple and effective.
> 
> I'm unclear on how urgent these fixes are.  I semi-randomly tagged them
> for 3.16 with a -stable backport, but that could be changed?

I assume tagged for 3.16 means you intend to get it merged before 3.16
gets released?
If so, then that would be fine with me.

Re: [PATCH 0/2] /proc/stat vs. failed order-4 allocation

[Andrew Morton]

On Sat, 21 Jun 2014 11:10:58 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:

> On Wed, Jun 18, 2014 at 02:29:31PM -0700, Andrew Morton wrote:
> > On Mon, 16 Jun 2014 11:04:50 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > 
> > > These two patches are supposed to "fix" failed order-4 memory
> > > allocations which have been observed when reading /proc/stat.
> > > The problem has been observed on s390 as well as on x86.
> > > 
> > > To address the problem change the seq_file memory allocations to
> > > fallback to use vmalloc, so that allocations also work if memory
> > > is fragmented.
> > > 
> > > This approach seems to be simpler and less intrusive than changing
> > > /proc/stat to use an interator. Also it "fixes" other users as well,
> > > which use seq_file's single_open() interface.
> > 
> > Yes, those changes look pretty simple and effective.
> > 
> > I'm unclear on how urgent these fixes are.  I semi-randomly tagged them
> > for 3.16 with a -stable backport, but that could be changed?
> 
> I assume tagged for 3.16 means you intend to get it merged before 3.16
> gets released?
> If so, then that would be fine with me.

um, actually, no, sorry, I meant merge for 3.17-rc1 with a -stable backport.

We can do 3.16 of course, but for what reasons?

Re: [PATCH 0/2] /proc/stat vs. failed order-4 allocation

[David Rientjes]

On Mon, 23 Jun 2014, Andrew Morton wrote:

> On Sat, 21 Jun 2014 11:10:58 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> 
> > On Wed, Jun 18, 2014 at 02:29:31PM -0700, Andrew Morton wrote:
> > > On Mon, 16 Jun 2014 11:04:50 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > > 
> > > > These two patches are supposed to "fix" failed order-4 memory
> > > > allocations which have been observed when reading /proc/stat.
> > > > The problem has been observed on s390 as well as on x86.
> > > > 
> > > > To address the problem change the seq_file memory allocations to
> > > > fallback to use vmalloc, so that allocations also work if memory
> > > > is fragmented.
> > > > 
> > > > This approach seems to be simpler and less intrusive than changing
> > > > /proc/stat to use an interator. Also it "fixes" other users as well,
> > > > which use seq_file's single_open() interface.
> > > 
> > > Yes, those changes look pretty simple and effective.
> > > 
> > > I'm unclear on how urgent these fixes are.  I semi-randomly tagged them
> > > for 3.16 with a -stable backport, but that could be changed?
> > 
> > I assume tagged for 3.16 means you intend to get it merged before 3.16
> > gets released?
> > If so, then that would be fine with me.
> 
> um, actually, no, sorry, I meant merge for 3.17-rc1 with a -stable backport.
> 
> We can do 3.16 of course, but for what reasons?
> 

Heiko's patches should also fix 
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1319244 as reported 
by Stefan Bader.  I've pinged them to determine if there is any other 
issues with -mm.

Re: [PATCH 0/2] /proc/stat vs. failed order-4 allocation

[Heiko Carstens]

On Tue, Jun 24, 2014 at 04:52:22PM -0700, David Rientjes wrote:
> On Mon, 23 Jun 2014, Andrew Morton wrote:
> > On Sat, 21 Jun 2014 11:10:58 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > > On Wed, Jun 18, 2014 at 02:29:31PM -0700, Andrew Morton wrote:
> > > > I'm unclear on how urgent these fixes are.  I semi-randomly tagged them
> > > > for 3.16 with a -stable backport, but that could be changed?
> > > 
> > > I assume tagged for 3.16 means you intend to get it merged before 3.16
> > > gets released?
> > > If so, then that would be fine with me.
> > 
> > um, actually, no, sorry, I meant merge for 3.17-rc1 with a -stable backport.
> > 
> > We can do 3.16 of course, but for what reasons?
> 
> Heiko's patches should also fix 
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1319244 as reported 
> by Stefan Bader.  I've pinged them to determine if there is any other 
> issues with -mm.

The other distribution where this has been seen was RHEL7 (on s390).

FWIW, I thought there was a 'rule' that patches with a -stable tag shouldn't
stay in -next for a couple of rc releases. But then again that probably was
just a discussion somewhere.
Anyway, if neither Stefan or Ian speak up I'm fine with post 3.16 as well.

Re: [PATCH 0/2] /proc/stat vs. failed order-4 allocation

[Ian Kent]

On Wed, 2014-06-25 at 08:15 +0200, Heiko Carstens wrote:
> On Tue, Jun 24, 2014 at 04:52:22PM -0700, David Rientjes wrote:
> > On Mon, 23 Jun 2014, Andrew Morton wrote:
> > > On Sat, 21 Jun 2014 11:10:58 +0200 Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> > > > On Wed, Jun 18, 2014 at 02:29:31PM -0700, Andrew Morton wrote:
> > > > > I'm unclear on how urgent these fixes are.  I semi-randomly tagged them
> > > > > for 3.16 with a -stable backport, but that could be changed?
> > > > 
> > > > I assume tagged for 3.16 means you intend to get it merged before 3.16
> > > > gets released?
> > > > If so, then that would be fine with me.
> > > 
> > > um, actually, no, sorry, I meant merge for 3.17-rc1 with a -stable backport.
> > > 
> > > We can do 3.16 of course, but for what reasons?
> > 
> > Heiko's patches should also fix 
> > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1319244 as reported 
> > by Stefan Bader.  I've pinged them to determine if there is any other 
> > issues with -mm.
> 
> The other distribution where this has been seen was RHEL7 (on s390).
> 
> FWIW, I thought there was a 'rule' that patches with a -stable tag shouldn't
> stay in -next for a couple of rc releases. But then again that probably was
> just a discussion somewhere.
> Anyway, if neither Stefan or Ian speak up I'm fine with post 3.16 as well.
> 

It's very much up to Andrew, of course.

I would like to see this go in sooner rather than later, if possible,
due to the potential RHEL-7 disruption it can cause. More so now the
above shows we can see this on other architectures and higher speced
machines.

Looking at the patches it appears there's no change from the original
semantics.

So the potential for regression is fairly low now since Heiko's patches
are much simpler than the original proposal and are much more straight
forward to review, so much so that they make sense even to me!

Ian