[PATCH 0/3] Replace kmalloc with vmalloc in seq_files

[PATCH 0/3] Replace kmalloc with vmalloc in seq_files

[Colin Cross]

When a seq_file is implemented with single_open, the show function will
be called with a kmalloc'd PAGE_SIZE buffer.  If the show function
produces more data than can fit in the buffer, the buffer will be thrown
away, and the show function will be called again with a buffer twice as
large.  This process repeats until the show function does not overflow
the buffer, or kmalloc fails.

seq_files are often used for debugging data.  When the system is under
memory pressure, and dumping debugging data starts trying to allocate
large physically contiguous buffers, it often makes the problem worse.

Since there is no need for a physically contiguous buffer, this patch
set converts the kmalloc'd buffers into vmallocs.  There are two
seq_file users that kmalloc buffer and place it directly into the
seq_file structure, later to be freed by seq_release.  Convert those
to call a new seq_reserve function that will do the correct allocation
for them.

[PATCH 1/3] fs: seq_file: add seq_reserve

[Colin Cross]

Adds a seq_reserve function to allow users of the seq_file interface to
increase the initial size of the buffer.  Avoids repeated allocations and
calls to the show function in cases where large buffers of known size are
needed.

Signed-off-by: Colin Cross <ccross@android.com>
---
 fs/seq_file.c            |   30 ++++++++++++++++++++++++++++++
 include/linux/seq_file.h |    1 +
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 05d6b0e..8d9778c 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -62,6 +62,36 @@ int seq_open(struct file *file, const struct seq_operations *op)
 }
 EXPORT_SYMBOL(seq_open);
 
+/**
+ *	seq_reserve -	increase initial buffer for sequential file
+ *	@m:	target buffer
+ *	@size: size in bytes to reserve
+ *
+ *	seq_reserve() increases the initial size of the seq_file buffer.  Can
+ *	be used to avoid future allocations and repeated calls to the show
+ *	function for large buffers of known size.
+ *	Returns 0 on success, or -ENOMEM if the buffer cannot be allocated.
+ */
+int seq_reserve(struct seq_file *m, size_t size)
+{
+	void *buf;
+
+	/* 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;
+
+	kfree(m->buf);
+	m->buf = buf;
+	m->size = size;
+
+	return 0;
+}
+EXPORT_SYMBOL(seq_reserve);
+
 static int traverse(struct seq_file *m, loff_t offset)
 {
 	loff_t pos = 0, index;
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 03c0232..ad93319 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -75,6 +75,7 @@ static inline void seq_commit(struct seq_file *m, int num)
 
 char *mangle_path(char *s, char *p, char *esc);
 int seq_open(struct file *, const struct seq_operations *);
+int seq_reserve(struct seq_file *m, size_t size);
 ssize_t seq_read(struct file *, char __user *, size_t, loff_t *);
 loff_t seq_lseek(struct file *, loff_t, int);
 int seq_release(struct inode *, struct file *);
-- 
1.7.4.1

Re: [PATCH 1/3] fs: seq_file: add seq_reserve

[Alan Cox]

On Thu, 22 Sep 2011 13:57:07 -0700
Colin Cross <ccross@android.com> wrote:

> Adds a seq_reserve function to allow users of the seq_file interface to
> increase the initial size of the buffer.  Avoids repeated allocations and
> calls to the show function in cases where large buffers of known size are
> needed.

Would it not be better to have something akin to


	void seq_set_buffer(struct seq_file *m, void *ptr, size_t size)
	{
		m->buf = ptr;
		m->size = size;
		m->private = 1;
	}

and let the caller simply provide a buffer by any means it wishes (even
statically) and free it again itself. That keeps the caller fully in
control when it wishes to be.

Alan

Re: [PATCH 1/3] fs: seq_file: add seq_reserve

[Colin Cross]

On Thu, Sep 22, 2011 at 3:54 PM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Thu, 22 Sep 2011 13:57:07 -0700
> Colin Cross <ccross@android.com> wrote:
>
>> Adds a seq_reserve function to allow users of the seq_file interface to
>> increase the initial size of the buffer.  Avoids repeated allocations and
>> calls to the show function in cases where large buffers of known size are
>> needed.
>
> Would it not be better to have something akin to
>
>
>        void seq_set_buffer(struct seq_file *m, void *ptr, size_t size)
>        {
>                m->buf = ptr;
>                m->size = size;
>                m->private = 1;
>        }
>
> and let the caller simply provide a buffer by any means it wishes (even
> statically) and free it again itself. That keeps the caller fully in
> control when it wishes to be.

What would happen when seq_read overflowed the buffer?  It wouldn't be
able to reallocate it.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/3] sched_stats: use the new seq_reserve function

[Colin Cross]

Instead of writing directly to private members of the seq_file struct,
use the new seq_reserve function to specify the predicted size of the
seq buffer.

Signed-off-by: Colin Cross <ccross@android.com>
---
 fs/proc/stat.c       |   23 ++++++++---------------
 kernel/sched_stats.h |   18 +++++++++---------
 2 files changed, 17 insertions(+), 24 deletions(-)

diff --git a/fs/proc/stat.c b/fs/proc/stat.c
index 9758b65..1fdf158 100644
--- a/fs/proc/stat.c
+++ b/fs/proc/stat.c
@@ -6,7 +6,6 @@
 #include <linux/proc_fs.h>
 #include <linux/sched.h>
 #include <linux/seq_file.h>
-#include <linux/slab.h>
 #include <linux/time.h>
 #include <linux/irqnr.h>
 #include <asm/cputime.h>
@@ -134,24 +133,18 @@ static int show_stat(struct seq_file *p, void *v)
 static int stat_open(struct inode *inode, struct file *file)
 {
 	unsigned size = 4096 * (1 + num_possible_cpus() / 32);
-	char *buf;
 	struct seq_file *m;
 	int res;
 
-	/* 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 = size;
-	} else
-		kfree(buf);
+	if (res)
+		return res;
+
+	m = file->private_data;
+	res = seq_reserve(m, size);
+	if (res)
+		single_release(inode, file);
+
 	return res;
 }
 
diff --git a/kernel/sched_stats.h b/kernel/sched_stats.h
index 331e01b..ee05143 100644
--- a/kernel/sched_stats.h
+++ b/kernel/sched_stats.h
@@ -74,19 +74,19 @@ static int show_schedstat(struct seq_file *seq, void *v)
 static int schedstat_open(struct inode *inode, struct file *file)
 {
 	unsigned int size = PAGE_SIZE * (1 + num_online_cpus() / 32);
-	char *buf = kmalloc(size, GFP_KERNEL);
 	struct seq_file *m;
 	int res;
 
-	if (!buf)
-		return -ENOMEM;
 	res = single_open(file, show_schedstat, NULL);
-	if (!res) {
-		m = file->private_data;
-		m->buf = buf;
-		m->size = size;
-	} else
-		kfree(buf);
+	if (res)
+		return res;
+
+
+	m = file->private_data;
+	res = seq_reserve(m, size);
+	if (res)
+		single_release(inode, file);
+
 	return res;
 }
 
-- 
1.7.4.1

[PATCH 3/3] seq_file: convert seq buffer to vmalloc

[Colin Cross]

seq_files are often used for debugging.  When things are going wrong
due to failed physically contiguous allocations, the exponentially
growing physically contiguous allocations in seq_read can make things
worse.  There is no need for physically contiguous memory, so switch
to virtually contiguous memory instead.

Signed-off-by: Colin Cross <ccross@android.com>
---
 fs/seq_file.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/fs/seq_file.c b/fs/seq_file.c
index 8d9778c..7045656 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -9,6 +9,7 @@
 #include <linux/module.h>
 #include <linux/seq_file.h>
 #include <linux/slab.h>
+#include <linux/vmalloc.h>
 
 #include <asm/uaccess.h>
 #include <asm/page.h>
@@ -76,15 +77,11 @@ int seq_reserve(struct seq_file *m, size_t size)
 {
 	void *buf;
 
-	/* don't ask for more than the kmalloc() max size */
-	if (size > KMALLOC_MAX_SIZE)
-		size = KMALLOC_MAX_SIZE;
-
-	buf = kmalloc(size, GFP_KERNEL);
+	buf = vmalloc(size);
 	if (!buf)
 		return -ENOMEM;
 
-	kfree(m->buf);
+	vfree(m->buf);
 	m->buf = buf;
 	m->size = size;
 
@@ -106,7 +103,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 = vmalloc(m->size = PAGE_SIZE);
 		if (!m->buf)
 			return -ENOMEM;
 	}
@@ -145,8 +142,8 @@ static int traverse(struct seq_file *m, loff_t offset)
 
 Eoverflow:
 	m->op->stop(m, p);
-	kfree(m->buf);
-	m->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+	vfree(m->buf);
+	m->buf = vmalloc(m->size <<= 1);
 	return !m->buf ? -ENOMEM : -EAGAIN;
 }
 
@@ -199,7 +196,7 @@ ssize_t seq_read(struct file *file, char __user *buf, size_t size, loff_t *ppos)
 	m->version = file->f_version;
 	/* grab buffer if we didn't have one */
 	if (!m->buf) {
-		m->buf = kmalloc(m->size = PAGE_SIZE, GFP_KERNEL);
+		m->buf = vmalloc(m->size = PAGE_SIZE);
 		if (!m->buf)
 			goto Enomem;
 	}
@@ -239,8 +236,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->buf = kmalloc(m->size <<= 1, GFP_KERNEL);
+		vfree(m->buf);
+		m->buf = vmalloc(m->size <<= 1);
 		if (!m->buf)
 			goto Enomem;
 		m->count = 0;
@@ -355,7 +352,7 @@ EXPORT_SYMBOL(seq_lseek);
 int seq_release(struct inode *inode, struct file *file)
 {
 	struct seq_file *m = file->private_data;
-	kfree(m->buf);
+	vfree(m->buf);
 	kfree(m);
 	return 0;
 }
-- 
1.7.4.1

Re: [PATCH 3/3] seq_file: convert seq buffer to vmalloc

[Joe Perches]

On Thu, 2011-09-22 at 13:57 -0700, Colin Cross wrote:
> seq_files are often used for debugging.  When things are going wrong
> due to failed physically contiguous allocations, the exponentially
> growing physically contiguous allocations in seq_read can make things
> worse.  There is no need for physically contiguous memory, so switch
> to virtually contiguous memory instead.

vmalloc's are relatively expensive.
Perhaps use kmalloc when appropriate instead?

[]
> -	/* don't ask for more than the kmalloc() max size */
> -	if (size > KMALLOC_MAX_SIZE)
> -		size = KMALLOC_MAX_SIZE;
> -
> -	buf = kmalloc(size, GFP_KERNEL);
> +	buf = vmalloc(size);
>  	if (!buf)
>  		return -ENOMEM;

	if (size > KMALLOC_MAX_SIZE)
		buf = vmalloc(size, GFP_KERNEL)
	else
		buf = kmalloc(size, GFP_KERNEL);
	
> +	vfree(m->buf);

	if (m->size > KMALLOC_MAX_SIZE)
		vfree(m->buf);
	else
		kfree(m->buf);

>  	m->buf = buf;
>  	m->size = size;
>  
> @@ -106,7 +103,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 = vmalloc(m->size = PAGE_SIZE);

embedding the set of m->size like this is ugly.

[do the same as above kmalloc/vmalloc based on size]

etc.

Re: [PATCH 3/3] seq_file: convert seq buffer to vmalloc

[Colin Cross]

On Thu, Sep 22, 2011 at 2:07 PM, Joe Perches <joe@perches.com> wrote:
> On Thu, 2011-09-22 at 13:57 -0700, Colin Cross wrote:
>> seq_files are often used for debugging.  When things are going wrong
>> due to failed physically contiguous allocations, the exponentially
>> growing physically contiguous allocations in seq_read can make things
>> worse.  There is no need for physically contiguous memory, so switch
>> to virtually contiguous memory instead.
>
> vmalloc's are relatively expensive.
> Perhaps use kmalloc when appropriate instead?
Talking about allocation efficiency in the context of seq_files is
silly - for a KMALLOC_MAX_SIZE buffer (8MB), you are already going to
allocate 11 times, with increasingly large buffers, and are likely to
fail long before you ever get to KMALLOC_MAX_SIZE.

> []
>> -     /* don't ask for more than the kmalloc() max size */
>> -     if (size > KMALLOC_MAX_SIZE)
>> -             size = KMALLOC_MAX_SIZE;
>> -
>> -     buf = kmalloc(size, GFP_KERNEL);
>> +     buf = vmalloc(size);
>>       if (!buf)
>>               return -ENOMEM;
>
>        if (size > KMALLOC_MAX_SIZE)
>                buf = vmalloc(size, GFP_KERNEL)
>        else
>                buf = kmalloc(size, GFP_KERNEL);
KMALLOC_MAX_SIZE is far too big for this.  KMALLOC_MAX_SIZE is the
maximum allocation that is theoretically possible, but will fail if
you don't have any completely empty pageblocks.  If I were to put a
size here, it would probably be order 3, but even that can easily fail
on a system that has under memory pressure.

>> +     vfree(m->buf);
>
>        if (m->size > KMALLOC_MAX_SIZE)
>                vfree(m->buf);
>        else
>                kfree(m->buf);
>
>>       m->buf = buf;
>>       m->size = size;
>>
>> @@ -106,7 +103,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 = vmalloc(m->size = PAGE_SIZE);
>
> embedding the set of m->size like this is ugly.
I agree, but it was there in the original file.  I could clean it up,
but that should be in a separate patch.

> [do the same as above kmalloc/vmalloc based on size]
>
> etc.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] seq_file: convert seq buffer to vmalloc

[NamJae Jeon]

2011/9/23 Colin Cross <ccross@android.com>:
> On Thu, Sep 22, 2011 at 2:07 PM, Joe Perches <joe@perches.com> wrote:
>> On Thu, 2011-09-22 at 13:57 -0700, Colin Cross wrote:
>>> seq_files are often used for debugging.  When things are going wrong
>>> due to failed physically contiguous allocations, the exponentially
>>> growing physically contiguous allocations in seq_read can make things
>>> worse.  There is no need for physically contiguous memory, so switch
>>> to virtually contiguous memory instead.
>>
>> vmalloc's are relatively expensive.
>> Perhaps use kmalloc when appropriate instead?
> Talking about allocation efficiency in the context of seq_files is
> silly - for a KMALLOC_MAX_SIZE buffer (8MB), you are already going to
> allocate 11 times, with increasingly large buffers, and are likely to
> fail long before you ever get to KMALLOC_MAX_SIZE.
>
>> []
>>> -     /* don't ask for more than the kmalloc() max size */
>>> -     if (size > KMALLOC_MAX_SIZE)
>>> -             size = KMALLOC_MAX_SIZE;
>>> -
>>> -     buf = kmalloc(size, GFP_KERNEL);
>>> +     buf = vmalloc(size);
>>>       if (!buf)
>>>               return -ENOMEM;
>>
>>        if (size > KMALLOC_MAX_SIZE)
>>                buf = vmalloc(size, GFP_KERNEL)
>>        else
>>                buf = kmalloc(size, GFP_KERNEL);
> KMALLOC_MAX_SIZE is far too big for this.  KMALLOC_MAX_SIZE is the
> maximum allocation that is theoretically possible, but will fail if
> you don't have any completely empty pageblocks.  If I were to put a
> size here, it would probably be order 3, but even that can easily fail
> on a system that has under memory pressure.
-> I agree. I think that vmalloc is better than kmalloc. because you
can face page allocation fail by high order page.
>
>>> +     vfree(m->buf);
>>
>>        if (m->size > KMALLOC_MAX_SIZE)
>>                vfree(m->buf);
>>        else
>>                kfree(m->buf);
>>
>>>       m->buf = buf;
>>>       m->size = size;
>>>
>>> @@ -106,7 +103,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 = vmalloc(m->size = PAGE_SIZE);
>>
>> embedding the set of m->size like this is ugly.
> I agree, but it was there in the original file.  I could clean it up,
> but that should be in a separate patch.
>
>> [do the same as above kmalloc/vmalloc based on size]
>>
>> etc.
>>
>>
> --
> 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/
>

Re: [PATCH 3/3] seq_file: convert seq buffer to vmalloc

[Peter Zijlstra]

On Thu, 2011-09-22 at 14:07 -0700, Joe Perches wrote:
> On Thu, 2011-09-22 at 13:57 -0700, Colin Cross wrote:
> > seq_files are often used for debugging.  When things are going wrong
> > due to failed physically contiguous allocations, the exponentially
> > growing physically contiguous allocations in seq_read can make things
> > worse.  There is no need for physically contiguous memory, so switch
> > to virtually contiguous memory instead.
> 
> vmalloc's are relatively expensive.

And very limited on certain archs..

> Perhaps use kmalloc when appropriate instead?
> 
> []
> > -	/* don't ask for more than the kmalloc() max size */
> > -	if (size > KMALLOC_MAX_SIZE)
> > -		size = KMALLOC_MAX_SIZE;
> > -
> > -	buf = kmalloc(size, GFP_KERNEL);
> > +	buf = vmalloc(size);
> >  	if (!buf)
> >  		return -ENOMEM;
> 
> 	if (size > KMALLOC_MAX_SIZE)
> 		buf = vmalloc(size, GFP_KERNEL)
> 	else
> 		buf = kmalloc(size, GFP_KERNEL)

There's wrappers for this stuff IIRC, that said all this is a horrible
idea. Just avoid the situation instead of coping with it.

Re: [PATCH 0/3] Replace kmalloc with vmalloc in seq_files

[Peter Zijlstra]

On Thu, 2011-09-22 at 13:57 -0700, Colin Cross wrote:
> 
> seq_files are often used for debugging data.  When the system is under
> memory pressure, and dumping debugging data starts trying to allocate
> large physically contiguous buffers, it often makes the problem
> worse. 

Shouldn't you be using ->next instead of dumping everything in a single
seqop? That way it can return partial buffers and keep state etc.

Re: [PATCH 0/3] Replace kmalloc with vmalloc in seq_files

[Christoph Hellwig]

On Thu, Sep 22, 2011 at 01:57:06PM -0700, Colin Cross wrote:
> seq_files are often used for debugging data.  When the system is under
> memory pressure, and dumping debugging data starts trying to allocate
> large physically contiguous buffers, it often makes the problem worse.

Please fix the instances that you see issues with by using the full
seq_file interface which was designed for his instead of the simplified
"single" interface that is only designed for small amounts of data.

Re: [PATCH 0/3] Replace kmalloc with vmalloc in seq_files

[Colin Cross]

On Thu, Sep 22, 2011 at 2:23 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Thu, Sep 22, 2011 at 01:57:06PM -0700, Colin Cross wrote:
>> seq_files are often used for debugging data.  When the system is under
>> memory pressure, and dumping debugging data starts trying to allocate
>> large physically contiguous buffers, it often makes the problem worse.
>
> Please fix the instances that you see issues with by using the full
> seq_file interface which was designed for his instead of the simplified
> "single" interface that is only designed for small amounts of data.

You're probably right, but it's not always easy.  For files that need
to show an atomic snapshot of some data, the data needs to go
somewhere.  It would be possible to allocate a smaller data structure
and atomically copy a snapshot into it, then use the iterator
interface to push the data out to userspace, but that's a lot harder
than a vmalloc and a few seq_printfs.

If seq_file used a list of buffers, it could allocate much smaller
chunks (a page?), and add new buffers to the list instead of
reallocating and calling the read function again.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html