relayfs: support larger relay buffer

PATCH In review Masami Hiramatsu 5 participants 13 messages Apr 15, 2008 – Apr 17, 2008 Relay

Share & tools

Masami Hiramatsu

author

2008-04-15 15:28 UTC #0 lore raw

PATCH 1/1 relayfs: support larger relay buffer

Use vmalloc() and memset() instead of kcalloc() to allocate a page* array when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.

Signed-off-by Masami Hiramatsu <[email protected]>

---
This is useful for a 64-bit system which has a plenty of memory (tens of giga bytes) and a large kernel memory space.

I tested it on x86-64 and ia64.

 kernel/relay.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

kernel/relay.c +18 -4

Index: 2.6.25-rc8-mm2/kernel/relay.c
===================================================================
@@ -104,12 +104,20 @@ static int relay_mmap_buf(struct rchan_b
 static void *relay_alloc_buf(struct rchan_buf *buf, size_t *size)
 {
 	void *mem;
-	unsigned int i, j, n_pages;
+	unsigned int i, j, n_pages, pa_size;
 
 	*size = PAGE_ALIGN(*size);
 	n_pages = *size >> PAGE_SHIFT;
+	pa_size = n_pages * sizeof(struct page *);
 
-	buf->page_array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	if (pa_size > PAGE_SIZE) {
+		buf->page_array = vmalloc(pa_size);
+		if (buf->page_array)
+			memset(buf->page_array, 0, pa_size);
+	} else {
+		buf->page_array = kcalloc(n_pages, sizeof(struct page *),
+					  GFP_KERNEL);
+	}
 	if (!buf->page_array)
 		return NULL;

@@ -130,7 +138,10 @@ static void *relay_alloc_buf(struct rcha
 depopulate:
 	for (j = 0; j < i; j++)
 		__free_page(buf->page_array[j]);
-	kfree(buf->page_array);
+	if (pa_size > PAGE_SIZE)
+		vfree(buf->page_array);
+	else
+		kfree(buf->page_array);
 	return NULL;
 }

@@ -189,7 +200,10 @@ static void relay_destroy_buf(struct rch
 		vunmap(buf->start);
 		for (i = 0; i < buf->page_count; i++)
 			__free_page(buf->page_array[i]);
-		kfree(buf->page_array);
+		if (buf->page_count * sizeof(struct page *) > PAGE_SIZE)
+			vfree(buf->page_array);
+		else
+			kfree(buf->page_array);
 	}
 	chan->buf[buf->cpu] = NULL;
 	kfree(buf->padding);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Tom Zanussi

2008-04-16 04:23 UTC #1 lore raw

Quoted 11 lines · Masami Hiramatsu

Use vmalloc() and memset() instead of kcalloc() to allocate a page* array when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.
Signed-off-by Masami Hiramatsu <[email protected]>
---
This is useful for a 64-bit system which has a plenty of memory (tens of giga bytes) and a large kernel memory space.
I tested it on x86-64 and ia64.

Hi,

It looks ok to me, but it might be a little cleaner and avoid some duplication if you add the new code as a couple of functions instead. Just a suggestion...

Tom

Quoted 54 lines

 kernel/relay.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

Index: 2.6.25-rc8-mm2/kernel/relay.c
===================================================================

kernel/relay.c +18 -4

@@ -104,12 +104,20 @@ static int relay_mmap_buf(struct rchan_b
 static void *relay_alloc_buf(struct rchan_buf *buf, size_t *size)
 {
 	void *mem;
-	unsigned int i, j, n_pages;
+	unsigned int i, j, n_pages, pa_size;
 
 	*size = PAGE_ALIGN(*size);
 	n_pages = *size >> PAGE_SHIFT;
+	pa_size = n_pages * sizeof(struct page *);
 
-	buf->page_array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	if (pa_size > PAGE_SIZE) {
+		buf->page_array = vmalloc(pa_size);
+		if (buf->page_array)
+			memset(buf->page_array, 0, pa_size);
+	} else {
+		buf->page_array = kcalloc(n_pages, sizeof(struct page *),
+					  GFP_KERNEL);
+	}
 	if (!buf->page_array)
 		return NULL;

@@ -130,7 +138,10 @@ static void *relay_alloc_buf(struct rcha
 depopulate:
 	for (j = 0; j < i; j++)
 		__free_page(buf->page_array[j]);
-	kfree(buf->page_array);
+	if (pa_size > PAGE_SIZE)
+		vfree(buf->page_array);
+	else
+		kfree(buf->page_array);
 	return NULL;
 }

@@ -189,7 +200,10 @@ static void relay_destroy_buf(struct rch
 		vunmap(buf->start);
 		for (i = 0; i < buf->page_count; i++)
 			__free_page(buf->page_array[i]);
-		kfree(buf->page_array);
+		if (buf->page_count * sizeof(struct page *) > PAGE_SIZE)
+			vfree(buf->page_array);
+		else
+			kfree(buf->page_array);
 	}
 	chan->buf[buf->cpu] = NULL;
 	kfree(buf->padding);

Pekka Enberg

2008-04-16 08:33 UTC #11 lore raw

In reply to Masami Hiramatsu

Quoted 4 lines · Masami Hiramatsu

Use vmalloc() and memset() instead of kcalloc() to allocate a page* array
when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.

Quoted 8 lines

Signed-off-by Masami Hiramatsu <[email protected]>
---
@@ -130,7 +138,10 @@ static void *relay_alloc_buf(struct rcha
depopulate:
        for (j = 0; j < i; j++)
                __free_page(buf->page_array[j]);
- kfree(buf->page_array);
+ if (pa_size > PAGE_SIZE)

You can use is_vmalloc_addr() here.

Quoted 6 lines

+ vfree(buf->page_array);
+ else
 +               kfree(buf->page_array);
        return NULL;
}

Masami Hiramatsu

author

2008-04-16 14:44 UTC #12 lore raw

Hi Pekka,

Quoted 14 lines · Pekka Enberg

Quoted 4 lines · Masami Hiramatsu
Use vmalloc() and memset() instead of kcalloc() to allocate a page* array
when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.
Quoted 8 lines
Signed-off-by Masami Hiramatsu <[email protected]>
---
@@ -130,7 +138,10 @@ static void *relay_alloc_buf(struct rcha
depopulate:
        for (j = 0; j < i; j++)
                __free_page(buf->page_array[j]);
- kfree(buf->page_array);
+ if (pa_size > PAGE_SIZE)

Quoted 1 line

You can use is_vmalloc_addr() here.

Thank you for your good advice!
I'll use that.

Quoted 7 lines

Quoted 7 lines
+ vfree(buf->page_array);
+ else
 +               kfree(buf->page_array);
        return NULL;
}

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Masami Hiramatsu

author

2008-04-16 16:21 UTC #2 lore raw

In reply to Tom Zanussi

Hi Tom,

Quoted 10 lines · Tom Zanussi

Quoted 9 lines · Masami Hiramatsu
Use vmalloc() and memset() instead of kcalloc() to allocate a page* array when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.
Signed-off-by Masami Hiramatsu <[email protected]>
---
This is useful for a 64-bit system which has a plenty of memory (tens of giga bytes) and a large kernel memory space.

Quoted 8 lines

Quoted 2 lines
I tested it on x86-64 and ia64.
Hi,
It looks ok to me, but it might be a little cleaner and avoid some duplication if you add the new code as a couple of functions instead. Just a suggestion...

Sure, that is a good idea, I'll renew my patch.
Thank you,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Pekka J Enberg

2008-04-16 18:21 UTC #4 lore raw

Re: [PATCH -mm] relayfs: support larger relay buffer take 2

In reply to Masami Hiramatsu

Hi Masami,

Quoted 4 lines · Masami Hiramatsu

kernel/relay.c +4 -0

+static struct page *relay_alloc_page_array(unsigned int n_pages)
+{
+	struct page *array;
+	unsigned int pa_size = n_pages * sizeof(struct page *);

kernel/relay.c line 43

Quoted 10 lines

kernel/relay.c +10 -0

+
+	if (pa_size > PAGE_SIZE) {
+		array = vmalloc(pa_size);
+		if (array)
+			memset(array, 0, pa_size);
+	} else {
+		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	}
+	return array;
+}

I think it's bit confusing to have relay_alloc_page_array() return a pointer to struct page as it's really allocating an _array_ of pointers to struct page. So why not just use void * here as the kernel memory allocators do?

kernel/relay.c line 51

Quoted 7 lines

kernel/relay.c +7 -0

+static void relay_free_page_array(struct page *array)
+{
+	if (is_vmalloc_addr(array))
+		vfree(array);
+	else
+		kfree(array);
+}

Here as well.

Masami Hiramatsu

author

2008-04-16 18:25 UTC #3 lore raw

PATCH 1/1 relayfs: support larger relay buffer take 2

In reply to Tom Zanussi

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes from take1 to take2:
- add relay_alloc_page_array() and relay_free_page_array()
- use is_vmalloc_addr() instead of checking array size.

This is useful for a 64-bit system which has a plenty of memory (tens of giga bytes) and a large kernel memory space.

I tested it on x86-64 and ia64.

 kernel/relay.c |   29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

kernel/relay.c +26 -3

Index: 2.6.25-rc8-mm2/kernel/relay.c
===================================================================
@@ -27,6 +27,29 @@
 static DEFINE_MUTEX(relay_channels_mutex);
 static LIST_HEAD(relay_channels);
 
+static struct page *relay_alloc_page_array(unsigned int n_pages)
+{
+	struct page *array;
+	unsigned int pa_size = n_pages * sizeof(struct page *);
+
+	if (pa_size > PAGE_SIZE) {
+		array = vmalloc(pa_size);
+		if (array)
+			memset(array, 0, pa_size);
+	} else {
+		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	}
+	return array;
+}
+
+static void relay_free_page_array(struct page *array)
+{
+	if (is_vmalloc_addr(array))
+		vfree(array);
+	else
+		kfree(array);
+}
+
 /*
  * close() vm_op implementation for relay file mapping.
  */

@@ -109,7 +132,7 @@ static void *relay_alloc_buf(struct rcha
 	*size = PAGE_ALIGN(*size);
 	n_pages = *size >> PAGE_SHIFT;
 
-	buf->page_array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	buf->page_array = relay_alloc_page_array(n_pages);
 	if (!buf->page_array)
 		return NULL;

@@ -130,7 +153,7 @@ static void *relay_alloc_buf(struct rcha
 depopulate:
 	for (j = 0; j < i; j++)
 		__free_page(buf->page_array[j]);
-	kfree(buf->page_array);
+	relay_free_page_array(buf->page_array);
 	return NULL;
 }

@@ -189,7 +212,7 @@ static void relay_destroy_buf(struct rch
 		vunmap(buf->start);
 		for (i = 0; i < buf->page_count; i++)
 			__free_page(buf->page_array[i]);
-		kfree(buf->page_array);
+		relay_free_page_array(buf->page_array);
 	}
 	chan->buf[buf->cpu] = NULL;
 	kfree(buf->padding);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Masami Hiramatsu

author

2008-04-16 18:36 UTC #5 lore raw

Re: [PATCH -mm] relayfs: support larger relay buffer take 2

In reply to Pekka J Enberg

Hi,

Quoted 14 lines · Pekka J Enberg

Hi Masami,

Quoted 11 lines · Masami Hiramatsu

kernel/relay.c +11 -0

+static struct page *relay_alloc_page_array(unsigned int n_pages)
+{
+	struct page *array;
+	unsigned int pa_size = n_pages * sizeof(struct page *);
+
+	if (pa_size > PAGE_SIZE) {
+		array = vmalloc(pa_size);
+		if (array)
+			memset(array, 0, pa_size);
+	} else {
+		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);

Quoted 8 lines

kernel/relay.c line 43
Quoted 3 lines
kernel/relay.c +3 -0
+	}
+	return array;
+}
I think it's bit confusing to have relay_alloc_page_array() return a pointer to struct page as it's really allocating an _array_ of pointers to struct page. So why not just use void * here as the kernel memory allocators do?

Thank you very much, it was my mistake.
It should return struct page **.

Quoted 9 lines

kernel/relay.c line 51

Quoted 7 lines

kernel/relay.c +7 -0

+static void relay_free_page_array(struct page *array)
+{
+	if (is_vmalloc_addr(array))
+		vfree(array);
+	else
+		kfree(array);
+}

Here as well.

Thanks,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Pekka Enberg

Reviewed-by on v1 1/1

2008-04-16 19:56 UTC #7 lore raw

Re: [PATCH -mm] relayfs: support larger relay buffer take 3

In reply to Masami Hiramatsu

Quoted 5 lines · Masami Hiramatsu

Use vmalloc() and memset() instead of kcalloc() to allocate a page* array when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.
Signed-off-by Masami Hiramatsu <[email protected]>

Looks good to me!

Reviewed-by Pekka Enberg <[email protected]>

Masami Hiramatsu

author

2008-04-16 19:56 UTC #6 lore raw

PATCH 1/1 relayfs: support larger relay buffer take 3

In reply to Masami Hiramatsu

Signed-off-by Masami Hiramatsu <[email protected]>

---
Changes from take2 to take3:
- Use struct page ** instead of struct page *.
- move functions to the place before relay_mmap_buf.
- add comments.

This is useful for a 64-bit system which has a plenty of memory (tens of giga bytes) and a large kernel memory space.

I tested it on x86-64 and ia64.

 kernel/relay.c |   35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

kernel/relay.c +32 -3

Index: 2.6.25-rc8-mm2/kernel/relay.c
===================================================================
@@ -65,6 +65,35 @@ static struct vm_operations_struct relay
 	.close = relay_file_mmap_close,
 };
 
+/*
+ * allocate an array of pointers of struct page
+ */
+static struct page **relay_alloc_page_array(unsigned int n_pages)
+{
+	struct page **array;
+	unsigned int pa_size = n_pages * sizeof(struct page *);
+
+	if (pa_size > PAGE_SIZE) {
+		array = vmalloc(pa_size);
+		if (array)
+			memset(array, 0, pa_size);
+	} else {
+		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	}
+	return array;
+}
+
+/*
+ * free an array of pointers of struct page
+ */
+static void relay_free_page_array(struct page **array)
+{
+	if (is_vmalloc_addr(array))
+		vfree(array);
+	else
+		kfree(array);
+}
+
 /**
  *	relay_mmap_buf: - mmap channel buffer to process address space
  *	@buf: relay channel buffer

@@ -109,7 +138,7 @@ static void *relay_alloc_buf(struct rcha
 	*size = PAGE_ALIGN(*size);
 	n_pages = *size >> PAGE_SHIFT;
 
-	buf->page_array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	buf->page_array = relay_alloc_page_array(n_pages);
 	if (!buf->page_array)
 		return NULL;

@@ -130,7 +159,7 @@ static void *relay_alloc_buf(struct rcha
 depopulate:
 	for (j = 0; j < i; j++)
 		__free_page(buf->page_array[j]);
-	kfree(buf->page_array);
+	relay_free_page_array(buf->page_array);
 	return NULL;
 }

@@ -189,7 +218,7 @@ static void relay_destroy_buf(struct rch
 		vunmap(buf->start);
 		for (i = 0; i < buf->page_count; i++)
 			__free_page(buf->page_array[i]);
-		kfree(buf->page_array);
+		relay_free_page_array(buf->page_array);
 	}
 	chan->buf[buf->cpu] = NULL;
 	kfree(buf->padding);

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Andrew Morton

maintainer

2008-04-16 21:00 UTC #8 lore raw

Re: [PATCH -mm] relayfs: support larger relay buffer take 3

Quoted 4 lines · On Wed, 16 Apr 2008 15:51:56 -0400 Masami Hiramatsu <[email protected]> wrote:

(quoted diff) +4 -0

+static struct page **relay_alloc_page_array(unsigned int n_pages)
+{
+	struct page **array;
+	unsigned int pa_size = n_pages * sizeof(struct page *);

Review

Quoted 10 lines

(quoted diff) +10 -0

+
+	if (pa_size > PAGE_SIZE) {
+		array = vmalloc(pa_size);
+		if (array)
+			memset(array, 0, pa_size);
+	} else {
+		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	}
+	return array;
+}

It's a bit odd to multiply n_pages*sizeof() and to then call kcalloc(), which needs to do the same multiplication.

The compiler will presumably optimise that away, but still, how about this?

kernel/relay.c renamed from kernel/relay.c~relayfs-support-larger-relay-buffer-take-3-cleanup +2 -2

@@ -71,14 +71,14 @@ static struct vm_operations_struct relay
 static struct page **relay_alloc_page_array(unsigned int n_pages)
 {
 	struct page **array;
-	unsigned int pa_size = n_pages * sizeof(struct page *);
+	size_t pa_size = n_pages * sizeof(struct page *);
 
 	if (pa_size > PAGE_SIZE) {
 		array = vmalloc(pa_size);
 		if (array)
 			memset(array, 0, pa_size);
 	} else {
-		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+		array = kzalloc(pa_size, GFP_KERNEL);
 	}
 	return array;
 }

size_t is strictly the correct type for pa_size here. Even though vmalloc() takes a ulong.

Masami Hiramatsu

author Acked-by

2008-04-16 21:03 UTC #9 lore raw

Re: [PATCH -mm] relayfs: support larger relay buffer take 3

Hi Andrew,

Quoted 21 lines · Andrew Morton

Quoted 4 lines · On Wed, 16 Apr 2008 15:51:56 -0400 Masami Hiramatsu <[email protected]> wrote:
(quoted diff) +4 -0
+static struct page **relay_alloc_page_array(unsigned int n_pages)
+{
+	struct page **array;
+	unsigned int pa_size = n_pages * sizeof(struct page *);
Review
Quoted 10 lines
(quoted diff) +10 -0
+
+	if (pa_size > PAGE_SIZE) {
+		array = vmalloc(pa_size);
+		if (array)
+			memset(array, 0, pa_size);
+	} else {
+		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+	}
+	return array;
+}
It's a bit odd to multiply n_pages*sizeof() and to then call kcalloc(), which needs to do the same multiplication.

Quoted 1 line

The compiler will presumably optimise that away, but still, how about this?

Sure, it looks good to me.
Thank you so much,

Acked-by Masami Hiramatsu <[email protected]>

Quoted 23 lines

kernel/relay.c renamed from kernel/relay.c~relayfs-support-larger-relay-buffer-take-3-cleanup +2 -2

@@ -71,14 +71,14 @@ static struct vm_operations_struct relay
 static struct page **relay_alloc_page_array(unsigned int n_pages)
 {
 	struct page **array;
-	unsigned int pa_size = n_pages * sizeof(struct page *);
+	size_t pa_size = n_pages * sizeof(struct page *);
 
 	if (pa_size > PAGE_SIZE) {
 		array = vmalloc(pa_size);
 		if (array)
 			memset(array, 0, pa_size);
 	} else {
-		array = kcalloc(n_pages, sizeof(struct page *), GFP_KERNEL);
+		array = kzalloc(pa_size, GFP_KERNEL);
 	}
 	return array;
 }

Quoted 2 lines

size_t is strictly the correct type for pa_size here. Even though vmalloc() takes a ulong.

Thanks for the advice,

Signature · 7 lines

Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: [email protected]

Tom Zanussi

Reviewed-by on v1 1/1

2008-04-17 04:06 UTC #10 lore raw

Re: [PATCH -mm] relayfs: support larger relay buffer take 3

In reply to Masami Hiramatsu

Quoted 11 lines · Masami Hiramatsu

Use vmalloc() and memset() instead of kcalloc() to allocate a page* array when the array size is bigger than one page. This enables relayfs to support bigger relay buffers than 64MB on 4k-page system, 512MB on 16k-page system.
Signed-off-by Masami Hiramatsu <[email protected]>
---
Changes from take2 to take3:
- Use struct page ** instead of struct page *.
- move functions to the place before relay_mmap_buf.
- add comments.

Quoted 5 lines

This is useful for a 64-bit system which has a plenty of memory (tens of giga bytes) and a large kernel memory space.
I tested it on x86-64 and ia64.

Hi,

Looks fine to me.

Reviewed-by Tom Zanussi <[email protected]>