/proc/stat vs. failed order-4 allocation

From stat_open:

Hi all,

I'm just wondering why /proc/stat is a single_open() seq_file and not a regular seq_file with an iterator (say 48 online cpus for each iteration or something similar).

-----Original Message-----
From: [email protected] [mailto:linux-kernel- [email protected]] On Behalf Of Christoph Hellwig
Sent: Wednesday, May 21, 2014 9:32 AM
To: Heiko Carstens
Cc: Andrew Morton; KAMEZAWA Hiroyuki; Andrea Righi; Eric Dumazet; linux- [email protected]; [email protected]; Hendrik Brueckner; Thorsten Diehl
Subject: Re: /proc/stat vs. failed order-4 allocation

Quoted 2 lines · Heiko Carstens

Hi all,

Quoted 3 lines

I'm just wondering why /proc/stat is a single_open() seq_file and not a regular seq_file with an iterator (say 48 online cpus for each iteration or something similar).

Probably because no one sent a patch for it. I'm pretty sure it used the even more horrible old proc ops before and was converted in batch with various other files.

Quoted 5 lines · Heiko Carstens

Hi all,

I'm just wondering why /proc/stat is a single_open() seq_file and not a regular seq_file with an iterator (say 48 online cpus for each iteration or something similar).

Probably because no one sent a patch for it. I'm pretty sure it used the even more horrible old proc ops before and was converted in batch with various other files.

The number of bytes contained 'within' /proc/stat depends on the number of online cpus and not of the numbe of possible cpus.

This reduces the number of bytes requested for the initial buffer allocation within stat_open(). Which is usually way too high and for nr_possible_cpus() == 256 cpus would result in an order 4 allocation.

Order 4 allocations however may fail if memory is fragmented and we would end up with an unreadable /proc/stat file:

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 <[email protected]>

---
 fs/proc/stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/proc/stat.c +0 -0

index 9d231e9e5f0e..3898ca5f1e92 100644

fs/proc/stat.c +0 -0

 	char *buf;
 	struct seq_file *m;
 	int res;

Quoted 40 lines · Heiko Carstens

The number of bytes contained 'within' /proc/stat depends on the number of online cpus and not of the numbe of possible cpus.

This reduces the number of bytes requested for the initial buffer allocation within stat_open(). Which is usually way too high and for nr_possible_cpus() == 256 cpus would result in an order 4 allocation.

Order 4 allocations however may fail if memory is fragmented and we would end up with an unreadable /proc/stat file:

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 <[email protected]>

---
 fs/proc/stat.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/proc/stat.c +0 -0

index 9d231e9e5f0e..3898ca5f1e92 100644

fs/proc/stat.c static int show_stat(struct seq_file *p, void *v) · line 187

Quoted 5 lines

fs/proc/stat.c +1 -1

@@ -184,7 +184,7 @@ 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();
+	size_t size = 1024 + 128 * num_online_cpus();

Yes, I thought of this too when I was looking at the problem but was concerned about the number of online cpus changing during the read.

If a system can hotplug cpus then I guess we don't care much about the number of cpus increasing during the read, we'll just see incorrect data once, but what would happen if some cpus were removed? Do we even care about that case?

Quoted 4 lines

fs/proc/stat.c +0 -0

 	char *buf;
 	struct seq_file *m;
 	int res;

Now, /proc/stat uses single_open() for showing information. This means the all data will be gathered and buffered once to a (big) buf.

Now, /proc/stat shows stats per cpu and stats per IRQs. To get information in once-shot, it allocates a big buffer (until KMALLOC_MAX_SIZE).

Eric Dumazet reported that the bufsize calculation doesn't take the numner of IRQ into account and the information cannot be got in one-shot. (By this, seq_read() will allocate buffer again and read the whole data gain...)

This patch changes /proc/stat to use seq_open() rather than single_open() and provides ->start(), ->next(), ->stop(), ->show().

By this, /proc/stat will not need to take care of size of buffer.

[[email protected]]: This is the forward port of a patch from KAMEZAWA Hiroyuki (https://lkml.org/lkml/2012/1/23/41).
I added a couple of simple changes like e.g. that the cpu iterator handles 32 cpus in a batch to avoid lots of iterations.

With this patch it should not happen anymore that reading /proc/stat fails because of a failing high order memory allocation.

Quoted 2 lines · On Wed, 28 May 2014 11:01:53 +0200 Heiko Carstens <[email protected]>…

With this patch it should not happen anymore that reading /proc/stat fails because of a failing high order memory allocation.

So this deletes the problematic allocation which [1/2] kind-of fixed, yes?

I agree with Ian - there's a hotplugging race. And [1/2] doesn't do anything to address the worst-case allocation size. So I think we may as well do this all in a single patch.

Without having looked closely at the code I worry a bit about the effects. /proc/pid/stat is a complex thing and its contents will vary

So.. can we take this up for 3.16-rc1? See if we can get some careful review done then and test it for a couple of months?

Meanwhile, the changelog looks a bit hastily thrown together - some smoothing would be nice, and perhaps some work spent identifying possible behavioural changes. Timing changes, locking canges, effects of concurrent fork/exit activity etc?

Well... I'll try to come up with something better. Even though I only forward ported an existing patch to address a memory allocation failure.

From 52a050f85256d7586933365da1b98c6227651449 Mon Sep 17 00:00:00 2001

Quoted 16 lines · On Wed, 28 May 2014 11:01:53 +0200 Heiko Carstens <[email protected]>…

Now, /proc/stat uses single_open() for showing information. This means the all data will be gathered and buffered once to a (big) buf.

Now, /proc/stat shows stats per cpu and stats per IRQs. To get information in once-shot, it allocates a big buffer (until KMALLOC_MAX_SIZE).

Eric Dumazet reported that the bufsize calculation doesn't take the numner of IRQ into account and the information cannot be got in one-shot. (By this, seq_read() will allocate buffer again and read the whole data gain...)

This patch changes /proc/stat to use seq_open() rather than single_open() and provides ->start(), ->next(), ->stop(), ->show().

By this, /proc/stat will not need to take care of size of buffer.

Quoted 7 lines

[[email protected]]: This is the forward port of a patch from KAMEZAWA Hiroyuki (https://lkml.org/lkml/2012/1/23/41).
I added a couple of simple changes like e.g. that the cpu iterator handles 32 cpus in a batch to avoid lots of iterations.

With this patch it should not happen anymore that reading /proc/stat fails because of a failing high order memory allocation.

So this deletes the problematic allocation which [1/2] kind-of fixed, yes?

I agree with Ian - there's a hotplugging race. And [1/2] doesn't do anything to address the worst-case allocation size. So I think we may as well do this all in a single patch.

Without having looked closely at the code I worry a bit about the effects. /proc/pid/stat is a complex thing and its contents will vary in strange ways as the things it is reporting on undergo concurrent changes. This patch will presumably replace one set of bizarre behaviour with a new set of bizarre behaviour and there may be unforseen consequences to established userspace.

So we're going to need a lot of testing and a lot of testing time to identify issues and weed them out.

So.. can we take this up for 3.16-rc1? See if we can get some careful review done then and test it for a couple of months?

Meanwhile, the changelog looks a bit hastily thrown together - some smoothing would be nice, and perhaps some work spent identifying possible behavioural changes. Timing changes, locking canges, effects of concurrent fork/exit activity etc?

Quoted 43 lines · Andrew Morton

Quoted 16 lines · On Wed, 28 May 2014 11:01:53 +0200 Heiko Carstens <[email protected]>…

Now, /proc/stat uses single_open() for showing information. This means the all data will be gathered and buffered once to a (big) buf.

Now, /proc/stat shows stats per cpu and stats per IRQs. To get information in once-shot, it allocates a big buffer (until KMALLOC_MAX_SIZE).

Eric Dumazet reported that the bufsize calculation doesn't take the numner of IRQ into account and the information cannot be got in one-shot. (By this, seq_read() will allocate buffer again and read the whole data gain...)

This patch changes /proc/stat to use seq_open() rather than single_open() and provides ->start(), ->next(), ->stop(), ->show().

By this, /proc/stat will not need to take care of size of buffer.

Quoted 7 lines

[[email protected]]: This is the forward port of a patch from KAMEZAWA Hiroyuki (https://lkml.org/lkml/2012/1/23/41).
I added a couple of simple changes like e.g. that the cpu iterator handles 32 cpus in a batch to avoid lots of iterations.

With this patch it should not happen anymore that reading /proc/stat fails because of a failing high order memory allocation.

So this deletes the problematic allocation which [1/2] kind-of fixed, yes?

I agree with Ian - there's a hotplugging race. And [1/2] doesn't do anything to address the worst-case allocation size. So I think we may as well do this all in a single patch.

Without having looked closely at the code I worry a bit about the effects. /proc/pid/stat is a complex thing and its contents will vary in strange ways as the things it is reporting on undergo concurrent changes. This patch will presumably replace one set of bizarre behaviour with a new set of bizarre behaviour and there may be unforseen consequences to established userspace.

So we're going to need a lot of testing and a lot of testing time to identify issues and weed them out.

Quoted 8 lines

So.. can we take this up for 3.16-rc1? See if we can get some careful review done then and test it for a couple of months?

Meanwhile, the changelog looks a bit hastily thrown together - some smoothing would be nice, and perhaps some work spent identifying possible behavioural changes. Timing changes, locking canges, effects of concurrent fork/exit activity etc?

Umm ... I didn't expect this to turn into such a rant, apologies in advance.

Certainly using the usual seq_file ops is desirable in the long run and that change should be worked on by those that maintain this area of code but, as Andrew says, it's too large a change to put in without considerable testing.

Now the problem has been exposed by a change which sets the number of CPUs to the maximum number the architecture (s390) can have, 256, when no value has been specified and the kernel default configuration is used rather than 32 when hotplug is not set or 64 when it is.

The allocation problem doesn't occur when the number of CPUs is set to the previous default of 64, even for low end systems with 2 CPUs and 2G RAM (like the one for which this problem was reported), but becomes a problem when the number of CPUs is set to 256 on these systems.

Also, I believe the s390 maintainers are committed to keeping the the default configured number of CPUs at 256.

So the actual problem is the heuristic used to calculate a initial buffer size not accounting for a configured number of CPUs much greater than the hardware can sensibly accommodate.

If we assume that the current implementation functions correctly when the buffer overflows, ie. doubles the allocation size and restarts, then an interim solution to the problem comes down to not much more than what is in patch 1 above.

Looking at the current heuristic allocation sizes, without taking the allocation for irqs into account we get:
cpus: 32 size: 5k
cpus: 64 size: 9k
cpus: 128 size: 17k
cpus: 256 size: 33k

I don't know how many irqs need to be accounted for or if that will make a difference to my comments below. Someone else will need to comment on that.

We know (from the order 4 allocation fail) that with 256 CPUs kmalloc() is looking for a 64k slab chunk IIUC and on low memory systems will frequently fail to get it.

And for the previous default of 64 CPUs kmalloc() would be looking for a 16k slab which we have no evidence it doesn't always get even on low end systems.

So why even use a heuristic calculation, since it can be quite wasteful anyway or, as in this case badly wrong, why not just allocate 8k or 16k in the open every time knowing that if the actual number of CPUs is large we can reasonably expect the system RAM to be correspondingly large which should avoid allocation failures upon a read retry.

Comments please?

Alternatively we could also change the seqfile code to fall back to vmalloc allocations. That would probably "fix" all single_open usages where large contiguous memory areas are needed and later on fail due to memory fragmentation.
Does anybody like that approach (sample patch below)?

---
 fs/seq_file.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

fs/seq_file.c +2 -0

index 1d641bb108d2..fca78a04c0d1 100644
@@ -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>

@@ -82,6 +84,31 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }

fs/seq_file.c +18 -1

+static void seq_free(struct seq_file *m)
+{
+	if (unlikely(is_vmalloc_addr(m->buf)))
+		vfree(m->buf);
+	else
+		kfree(m->buf);
+}
+
+static void seq_realloc(struct seq_file *m)
+{
+	seq_free(m);
+	m->size <<= 1;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+
 static int traverse(struct seq_file *m, loff_t offset)
 {
 	loff_t pos = 0, index;

@@ -96,7 +123,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);
+		seq_alloc(m);
 		if (!m->buf)
 			return -ENOMEM;
 	}

fs/seq_file.c +8 -5

@@ -192,7 +218,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);
+		seq_alloc(m);
 		if (!m->buf)
 			goto Enomem;
 	}

@@ -232,9 +258,8 @@ 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);
 		m->count = 0;
-		m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+		seq_realloc(m);
 		if (!m->buf)
 			goto Enomem;
 		m->version = 0;

@@ -350,7 +375,7 @@ EXPORT_SYMBOL(seq_lseek);
 int seq_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *m = file->private_data;
-	kfree(m->buf);
+	seq_free(m);
 	kfree(m);
 	return 0;
 }

@@ -605,8 +630,12 @@ 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;
 	int ret;
+
+	buf = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
+	if (!buf)
+		buf = vmalloc(size);
 	if (!buf)
 		return -ENOMEM;
 	ret = single_open(file, show, data);

Quoted 5 lines · Heiko Carstens

Alternatively we could also change the seqfile code to fall back to vmalloc allocations. That would probably "fix" all single_open usages where large contiguous memory areas are needed and later on fail due to memory fragmentation.
Does anybody like that approach (sample patch below)?

Quoted 24 lines

---
 fs/seq_file.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

fs/seq_file.c +2 -0

index 1d641bb108d2..fca78a04c0d1 100644
@@ -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>

@@ -82,6 +84,31 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 

fs/seq_file.c int seq_open(struct file *file, const struct seq_operations *op) · line 94

Quoted 8 lines

fs/seq_file.c +8 -0

+static void seq_alloc(struct seq_file *m)
+{
+	m->size = PAGE_SIZE;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+

If m->size is unconditionally PAGE_SIZE, then how is vmalloc() going to allocate this if kmalloc() fails?

Quoted 32 lines

fs/seq_file.c +18 -1

+static void seq_free(struct seq_file *m)
+{
+	if (unlikely(is_vmalloc_addr(m->buf)))
+		vfree(m->buf);
+	else
+		kfree(m->buf);
+}
+
+static void seq_realloc(struct seq_file *m)
+{
+	seq_free(m);
+	m->size <<= 1;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+
 static int traverse(struct seq_file *m, loff_t offset)
 {
 	loff_t pos = 0, index;

@@ -96,7 +123,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);
+		seq_alloc(m);
 		if (!m->buf)
 			return -ENOMEM;
 	}

@@ -135,9 +162,8 @@ static int traverse(struct seq_file *m, loff_t offset)
 

fs/seq_file.c static int traverse(struct seq_file *m, loff_t offset) · line 169

Quoted 9 lines

fs/seq_file.c +1 -2

 Eoverflow:
 	m->op->stop(m, p);
-	kfree(m->buf);
 	m->count = 0;
-	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+	seq_realloc(m);
 	return !m->buf ? -ENOMEM : -EAGAIN;
 }
 

It seems like traverse() could be rewritten to use krealloc() which does a memcpy() to simplify the calling code.

Quoted 44 lines

fs/seq_file.c +8 -5

@@ -192,7 +218,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);
+		seq_alloc(m);
 		if (!m->buf)
 			goto Enomem;
 	}

@@ -232,9 +258,8 @@ 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);
 		m->count = 0;
-		m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+		seq_realloc(m);
 		if (!m->buf)
 			goto Enomem;
 		m->version = 0;

@@ -350,7 +375,7 @@ EXPORT_SYMBOL(seq_lseek);
 int seq_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *m = file->private_data;
-	kfree(m->buf);
+	seq_free(m);
 	kfree(m);
 	return 0;
 }

@@ -605,8 +630,12 @@ 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;
 	int ret;
+
+	buf = kmalloc(size, GFP_KERNEL | __GFP_NOWARN);
+	if (!buf)
+		buf = vmalloc(size);
 	if (!buf)
 		return -ENOMEM;
 	ret = single_open(file, show, data);

Quoted 19 lines

Quoted 19 lines

fs/seq_file.c +2 -0

index 1d641bb108d2..fca78a04c0d1 100644
@@ -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>

@@ -82,6 +84,31 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 

Quoted 11 lines

fs/seq_file.c int seq_open(struct file *file, const struct seq_operations *op) · line 94

Quoted 8 lines

fs/seq_file.c +8 -0

+static void seq_alloc(struct seq_file *m)
+{
+	m->size = PAGE_SIZE;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+

If m->size is unconditionally PAGE_SIZE, then how is vmalloc() going to allocate this if kmalloc() fails?

This is just the initial allocation.
If it runs out of room the allocation size doubles.

I think 2*PAGE_SIZE is probably better here since that's closer to what the original heuristic allocation requested and is likely to avoid reallocations in most cases.

The issue of kmalloc() failing for larger allocations on low speced hardware with fragmented memory might succeed when vmalloc() is used since it doesn't require contiguous memory chunks. But I guess the added pressure on the page table might still be a problem, nevertheless it's probably worth trying before bailing out.

Quoted 19 lines · Ian Kent

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+static void seq_alloc(struct seq_file *m)
+{
+	m->size = PAGE_SIZE;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+

If m->size is unconditionally PAGE_SIZE, then how is vmalloc() going to allocate this if kmalloc() fails?

This is just the initial allocation.
If it runs out of room the allocation size doubles.

I think 2*PAGE_SIZE is probably better here since that's closer to what the original heuristic allocation requested and is likely to avoid reallocations in most cases.

Quoted 5 lines

The issue of kmalloc() failing for larger allocations on low speced hardware with fragmented memory might succeed when vmalloc() is used since it doesn't require contiguous memory chunks. But I guess the added pressure on the page table might still be a problem, nevertheless it's probably worth trying before bailing out.

I'm not quarreling about using vmalloc() for allocations that are high-order, I'm referring to the rather obvious fact that m->size is set to PAGE_SIZE unconditionally above and thus vmalloc() isn't going to help in the slightest.

Quoted 26 lines · David Rientjes

Quoted 19 lines · Ian Kent

Quoted 11 lines

Review

Quoted 8 lines

(quoted diff) +8 -0

+static void seq_alloc(struct seq_file *m)
+{
+	m->size = PAGE_SIZE;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+

If m->size is unconditionally PAGE_SIZE, then how is vmalloc() going to allocate this if kmalloc() fails?

This is just the initial allocation.
If it runs out of room the allocation size doubles.

I think 2*PAGE_SIZE is probably better here since that's closer to what the original heuristic allocation requested and is likely to avoid reallocations in most cases.

Quoted 5 lines

The issue of kmalloc() failing for larger allocations on low speced hardware with fragmented memory might succeed when vmalloc() is used since it doesn't require contiguous memory chunks. But I guess the added pressure on the page table might still be a problem, nevertheless it's probably worth trying before bailing out.

Quoted 4 lines

I'm not quarreling about using vmalloc() for allocations that are high-order, I'm referring to the rather obvious fact that m->size is set to PAGE_SIZE unconditionally above and thus vmalloc() isn't going to help in the slightest.

Yes, that doesn't make any sense. I wrote the patch together in a hurry and didn't think much about it.
So below is the what I think most simple conversion to a vmalloc fallback approach for seq files. However the question remains if this seems to be an acceptable approach at all...

Yes, that doesn't make any sense. I wrote the patch together in a hurry and didn't think much about it.
So below is the what I think most simple conversion to a vmalloc fallback approach for seq files. However the question remains if this seems to be an acceptable approach at all...

Quoted 38 lines · Ian Kent

Quoted 19 lines

Quoted 19 lines

fs/seq_file.c +2 -0

index 1d641bb108d2..fca78a04c0d1 100644
@@ -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>

@@ -82,6 +84,31 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 

Quoted 11 lines

fs/seq_file.c int seq_open(struct file *file, const struct seq_operations *op) · line 94

Quoted 8 lines

fs/seq_file.c +8 -0

+static void seq_alloc(struct seq_file *m)
+{
+	m->size = PAGE_SIZE;
+	m->buf = kmalloc(m->size, GFP_KERNEL | __GFP_NOWARN);
+	if (!m->buf)
+		m->buf = vmalloc(m->size);
+}
+

If m->size is unconditionally PAGE_SIZE, then how is vmalloc() going to allocate this if kmalloc() fails?

This is just the initial allocation.
If it runs out of room the allocation size doubles.

I think 2*PAGE_SIZE is probably better here since that's closer to what the original heuristic allocation requested and is likely to avoid reallocations in most cases.

Quoted 6 lines

The issue of kmalloc() failing for larger allocations on low speced hardware with fragmented memory might succeed when vmalloc() is used since it doesn't require contiguous memory chunks. But I guess the added pressure on the page table might still be a problem, nevertheless it's probably worth trying before bailing out.

I'm not quarreling about using vmalloc() for allocations that are high-order, I'm referring to the rather obvious fact that m->size is set to PAGE_SIZE unconditionally above and thus vmalloc() isn't going to help in the slightest.

Hi all,

I'm just wondering why /proc/stat is a single_open() seq_file and not a regular seq_file with an iterator (say 48 online cpus for each iteration or something similar).

Of course, in theory, the "intr" line may be very long as well...

With the current implementation everything must fit into a single buffer. So if memory is highly fragmented we run into failing higher order allocations (like below), which effectively means reading /proc/stat doesn't work anymore.

From stat_open:

        size_t size = 1024 + 128 * num_possible_cpus();
	[...]
        /* minimum size to display an interrupt count : 2 bytes */
        size += 2 * nr_irqs;
	[...]
        buf = kmalloc(size, GFP_KERNEL);
        if (!buf)
                return -ENOMEM;

With num_possible_cpus() = 256 we end up with an order 4 allocation.

So, would there be any objections, adding a cpu iterator to /proc/stat?

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.701574] 00000000edf27840 00000000edf27850 0000000000000002 0000000000000000 

00000000edf278e0 00000000edf27858 00000000edf27858 00000000001120c0 0000000000000000 000000000072c7c0 0000000000711836 000000000000000b 00000000edf278a0 00000000edf27840 0000000000000000 0000000000000000 00000000001040d0 00000000001120c0 00000000edf27840 00000000edf278a0

[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
[62129.701620] [<000003fffd0040a0>] 0x3fffd0040a0
[62129.701624] Mem-Info:
[62129.701625] DMA per-cpu:
[62129.701627] CPU 0: hi: 186, btch: 31 usd: 0
[62129.701628] CPU 1: hi: 186, btch: 31 usd: 0
[62129.701630] CPU 2: hi: 186, btch: 31 usd: 51
[62129.701631] Normal per-cpu:
[62129.701632] CPU 0: hi: 186, btch: 31 usd: 30
[62129.701634] CPU 1: hi: 186, btch: 31 usd: 0
[62129.701635] CPU 2: hi: 186, btch: 31 usd: 0
[62129.701639] active_anon:5416 inactive_anon:5571 isolated_anon:0

active_file:440513 inactive_file:406221 isolated_file:27 unevictable:1741 dirty:35305 writeback:0 unstable:0 free:40319 slab_reclaimable:41921 slab_unreclaimable:34553 mapped:3921 shmem:1351 pagetables:296 bounce:0 free_cma:0

[62129.701648] DMA free:25192kB min:11800kB low:14748kB high:17700kB active_anon:11032kB inactive_anon:11320kB active_file:1002092kB inactive_file:904260kB unevictable:3772kB isolated(anon):0kB isolated(file):4kB present:2097152kB managed:2070452kB mlocked:3772kB dirty:55072kB writeback:0kB mapped:6316kB shmem:1152kB slab_reclaimable:61192kB slab_unreclaimable:50108kB kernel_stack:2368kB pagetables:532kB unstable:0kB bounce:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:133 all_unreclaimable? no
[62129.701652] lowmem_reserve[]: 0 1837 1837
[62129.701658] Normal free:136084kB min:10724kB low:13404kB high:16084kB active_anon:10632kB inactive_anon:10964kB active_file:759960kB inactive_file:720624kB unevictable:3192kB isolated(anon):0kB isolated(file):4kB present:1966080kB managed:1881776kB mlocked:3192kB dirty:86148kB writeback:0kB mapped:9368kB shmem:4252kB slab_reclaimable:106492kB slab_unreclaimable:88104kB kernel_stack:5808kB pagetables:652kB unstable:0kB bounce:0kB free_cma:0kB writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[62129.701661] lowmem_reserve[]: 0 0 0
[62129.701664] DMA: 1540*4kB (UEM) 2217*8kB (UEM) 9*16kB (UEM) 1*32kB (R) 5*64kB (R) 1*128kB (R) 1*256kB (R) 1*512kB (R) 0*1024kB = 25288kB
[62129.701673] Normal: 21631*4kB (UEM) 5755*8kB (UEM) 145*16kB (UEM) 8*32kB (ER) 4*64kB (R) 2*128kB (R) 0*256kB 1*512kB (R) 0*1024kB = 136164kB
[62129.701682] Node 0 hugepages_total=0 hugepages_free=0 hugepages_surp=0 hugepages_size=1024kB
[62129.701684] 849331 total pagecache pages
[62129.701685] 131 pages in swap cache
[62129.701687] Swap cache stats: add 9956, delete 9825, find 1049/1416
[62129.701688] Free swap = 7186784kB
[62129.701689] Total swap = 7212140kB
[62129.710679] 1015808 pages RAM
[62129.710681] 23437 pages reserved
[62129.710682] 1360146 pages shared
[62129.710683] 384507 pages non-shared

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