Replace kmalloc with vmalloc in seq_files

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.

Quoted 4 lines · On Thu, 22 Sep 2011 13:57:07 -0700 Colin Cross <[email protected]> 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.

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.

Quoted 5 lines · 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.

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

[]

Quoted 8 lines

-     /* 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);

Quoted 1 line

+     vfree(m->buf);

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

Quoted 3 lines

      m->buf = buf;
      m->size = size;

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

Quoted 6 lines

fs/seq_file.c +1 -1

@@ -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.

Quoted 9 lines · Colin Cross

Quoted 5 lines

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.

Quoted 10 lines

[]

Quoted 8 lines

-     /* 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;

Quoted 4 lines

       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.

Quoted 11 lines

Quoted 2 lines

+     vfree(m->buf);

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

Quoted 3 lines

      m->buf = buf;
      m->size = size;

Quoted 8 lines

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

Quoted 6 lines

fs/seq_file.c +1 -1

@@ -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.

Quoted 5 lines

[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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 5 lines · 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.

vmalloc's are relatively expensive.

Perhaps use kmalloc when appropriate instead?

[]

fs/seq_file.c int seq_reserve(struct seq_file *m, size_t size) · line 82

Quoted 8 lines

fs/seq_file.c +1 -5

-	/* 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)

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.

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.

Quoted 3 lines · Colin Cross

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.