[patch] cache pipe buf page address for non-highmem arch

[patch] cache pipe buf page address for non-highmem arch

[Ken Chen]

It is really sad that we always call kmap and friends for every pipe
buffer page on 64-bit arch that doesn't use HIGHMEM, or on
configuration that doesn't turn on HIGHMEM.

The effect of calling kmap* is visible in the execution profile when
pipe code is being stressed.  It is especially true on amd's x86-64
platform where kmap() has to traverse through numa node index
calculation in order to convert struct page * to kernel virtual
address.  It is fairly pointless to perform that calculation repeatly
on system with no highmem (i.e., 64-bit arch like x86-64).  This patch
caches kernel pipe buffer page's kernel vaddr to speed up pipe buffer
mapping functions.

There is another suboptimal block in pipe_read() where wake_up is
called twice.  I think it was an oversight since in pipe_write(), it
looks like it is doing the right thing.

Signed-off-by: Ken Chen <kenchen@google.com>


--- linus-2.6.git/fs/pipe.c.orig	2007-03-01 12:41:06.000000000 -0800
+++ linus-2.6.git/fs/pipe.c	2007-03-22 16:28:03.000000000 -0700
@@ -20,6 +20,22 @@

 #include <asm/uaccess.h>
 #include <asm/ioctls.h>
+#include <asm/kmap_types.h>
+
+#ifdef CONFIG_HIGHMEM
+#define pipe_kmap		kmap
+#define pipe_kmap_atomic	kmap_atomic
+#else	/* CONFIG_HIGHMEM */
+static inline void *pipe_kmap(struct page *page)
+{
+	return (void *) page->private;
+}
+static inline void *pipe_kmap_atomic(struct page *page, enum km_type type)
+{
+	pagefault_disable();
+	return pipe_kmap(page);
+}
+#endif

 /*
  * We use a start+len construction, which provides full use of the
@@ -169,10 +185,10 @@ void *generic_pipe_buf_map(struct pipe_i
 {
 	if (atomic) {
 		buf->flags |= PIPE_BUF_FLAG_ATOMIC;
-		return kmap_atomic(buf->page, KM_USER0);
+		return pipe_kmap_atomic(buf->page, KM_USER0);
 	}

-	return kmap(buf->page);
+	return pipe_kmap(buf->page);
 }

 void generic_pipe_buf_unmap(struct pipe_inode_info *pipe,
@@ -316,6 +332,7 @@ redo:
 		if (do_wakeup) {
 			wake_up_interruptible_sync(&pipe->wait);
  			kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
+			do_wakeup = 0;
 		}
 		pipe_wait(pipe);
 	}
@@ -423,6 +440,8 @@ redo1:
 					ret = ret ? : -ENOMEM;
 					break;
 				}
+				page->private = (unsigned long)
+							page_address(page);
 				pipe->tmp_page = page;
 			}
 			/* Always wake up, even if the copy fails. Otherwise
@@ -438,9 +457,9 @@ redo1:
 			iov_fault_in_pages_read(iov, chars);
 redo2:
 			if (atomic)
-				src = kmap_atomic(page, KM_USER0);
+				src = pipe_kmap_atomic(page, KM_USER0);
 			else
-				src = kmap(page);
+				src = pipe_kmap(page);

 			error = pipe_iov_copy_from_user(src, iov, chars,
 							atomic);

Re: [patch] cache pipe buf page address for non-highmem arch

[Andrew Morton]

On Thu, 22 Mar 2007 17:51:11 -0700 "Ken Chen" <kenchen@google.com> wrote:

> +#ifdef CONFIG_HIGHMEM
> +#define pipe_kmap		kmap
> +#define pipe_kmap_atomic	kmap_atomic
> +#else	/* CONFIG_HIGHMEM */
> +static inline void *pipe_kmap(struct page *page)
> +{
> +	return (void *) page->private;
> +}
> +static inline void *pipe_kmap_atomic(struct page *page, enum km_type type)
> +{
> +	pagefault_disable();
> +	return pipe_kmap(page);
> +}
> +#endif

If we're going to do this then we should also implement pipe_kunmap_atomic().
Relying upon kunmap_atomic() internals like this is weird-looking, and is fragile
against future changes to kunmap_atomic().

Re: [patch] cache pipe buf page address for non-highmem arch

[Ken Chen]

On 3/22/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> If we're going to do this then we should also implement pipe_kunmap_atomic().
> Relying upon kunmap_atomic() internals like this is weird-looking, and is fragile
> against future changes to kunmap_atomic().

OK, I added the matching pipe_kunmap variants.  Now that we have the
matching pair, I made further optimization to make both pipe_kmap and
pipe_kmap_atomic the same because pipe_kmap does not hold any critical
resource that can't sleep.  There is no reason to make them different.

Full patch and changelog:


It is really sad that we always call kmap and friends for every pipe
buffer page on 64-bit arch that doesn't use HIGHMEM, or on
configuration that doesn't turn on HIGHMEM.

The effect of calling kmap* is visible in the execution profile when
pipe code is being stressed.  It is especially true on amd's x86-64
platform where kmap() has to traverse through numa node index
calculation in order to convert struct page * to kernel virtual
address.  It is fairly pointless to perform that calculation repeatly
on system with no highmem (i.e., 64-bit arch like x86-64).  This patch
caches kernel pipe buffer page's kernel vaddr to speed up pipe buffer
mapping functions.

There is another suboptimal block in pipe_read() where wake_up is
called twice.  I think it was an oversight since in pipe_write(), it
looks like it is doing the right thing.

Signed-off-by: Ken Chen <kenchen@google.com>

diff --git a/fs/pipe.c b/fs/pipe.c
index ebafde7..4cd1d68 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -21,6 +21,21 @@ #include <linux/audit.h>
 #include <asm/uaccess.h>
 #include <asm/ioctls.h>

+#ifdef CONFIG_HIGHMEM
+#define pipe_kmap		kmap
+#define pipe_kmap_atomic	kmap_atomic
+#define pipe_kunmap		kunmap
+#define pipe_kunmap_atomic	kunmap_atomic
+#else	/* CONFIG_HIGHMEM */
+static inline void *pipe_kmap(struct page *page)
+{
+	return (void *) page->private;
+}
+#define pipe_kmap_atomic(page, type)	pipe_kmap(page)
+#define pipe_kunmap(page)		do { } while (0)
+#define pipe_kunmap_atomic(page, type)	do { } while (0)
+#endif
+
 /*
  * We use a start+len construction, which provides full use of the
  * allocated memory.
@@ -169,10 +184,10 @@ void *generic_pipe_buf_map(struct pipe_i
 {
 	if (atomic) {
 		buf->flags |= PIPE_BUF_FLAG_ATOMIC;
-		return kmap_atomic(buf->page, KM_USER0);
+		return pipe_kmap_atomic(buf->page, KM_USER0);
 	}

-	return kmap(buf->page);
+	return pipe_kmap(buf->page);
 }

 void generic_pipe_buf_unmap(struct pipe_inode_info *pipe,
@@ -180,9 +195,9 @@ void generic_pipe_buf_unmap(struct pipe_
 {
 	if (buf->flags & PIPE_BUF_FLAG_ATOMIC) {
 		buf->flags &= ~PIPE_BUF_FLAG_ATOMIC;
-		kunmap_atomic(map_data, KM_USER0);
+		pipe_kunmap_atomic(map_data, KM_USER0);
 	} else
-		kunmap(buf->page);
+		pipe_kunmap(buf->page);
 }

 int generic_pipe_buf_steal(struct pipe_inode_info *pipe,
@@ -316,6 +331,7 @@ redo:
 		if (do_wakeup) {
 			wake_up_interruptible_sync(&pipe->wait);
  			kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
+			do_wakeup = 0;
 		}
 		pipe_wait(pipe);
 	}
@@ -423,6 +439,8 @@ redo1:
 					ret = ret ? : -ENOMEM;
 					break;
 				}
+				page->private = (unsigned long)
+							page_address(page);
 				pipe->tmp_page = page;
 			}
 			/* Always wake up, even if the copy fails. Otherwise
@@ -438,16 +456,16 @@ redo1:
 			iov_fault_in_pages_read(iov, chars);
 redo2:
 			if (atomic)
-				src = kmap_atomic(page, KM_USER0);
+				src = pipe_kmap_atomic(page, KM_USER0);
 			else
-				src = kmap(page);
+				src = pipe_kmap(page);

 			error = pipe_iov_copy_from_user(src, iov, chars,
 							atomic);
 			if (atomic)
-				kunmap_atomic(src, KM_USER0);
+				pipe_kunmap_atomic(src, KM_USER0);
 			else
-				kunmap(page);
+				pipe_kunmap(page);

 			if (unlikely(error)) {
 				if (atomic) {

Re: [patch] cache pipe buf page address for non-highmem arch

[Andrew Morton]

On Fri, 23 Mar 2007 17:48:29 -0700 "Ken Chen" <kenchen@google.com> wrote:

> It is really sad that we always call kmap and friends for every pipe
> buffer page on 64-bit arch that doesn't use HIGHMEM, or on
> configuration that doesn't turn on HIGHMEM.
> 
> The effect of calling kmap* is visible in the execution profile when
> pipe code is being stressed.  It is especially true on amd's x86-64
> platform where kmap() has to traverse through numa node index
> calculation in order to convert struct page * to kernel virtual
> address.  It is fairly pointless to perform that calculation repeatly
> on system with no highmem (i.e., 64-bit arch like x86-64).  This patch
> caches kernel pipe buffer page's kernel vaddr to speed up pipe buffer
> mapping functions.

LTP's vmsplice01 triggers the below:

VFS: Mounted root (ext3 filesystem) readonly.
Freeing unused kernel memory: 200k freed
Unable to find swap-space signature
Unable to handle kernel NULL pointer dereference at 0000000000000130 RIP: 
 [<ffffffff8029e1b6>] pipe_to_file+0x1f3/0x2a6
PGD 10389d067 PUD 10107b067 PMD 0 
Oops: 0000 [1] PREEMPT SMP 
last sysfs file: devices/pci0000:00/0000:00:1d.7/usb1/dev
CPU 3 
Modules linked in: pcmcia firmware_class yenta_socket rsrc_nonstatic pcmcia_core
Pid: 18708, comm: vmsplice01 Not tainted 2.6.21-rc5-mm1 #2
RIP: 0010:[<ffffffff8029e1b6>]  [<ffffffff8029e1b6>] pipe_to_file+0x1f3/0x2a6
RSP: 0018:ffff81010448dd48  EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000ed0
RDX: ffff81010448dfd8 RSI: 0000000000000130 RDI: ffff810170149000
RBP: ffff81010448dda8 R08: 0000000000000004 R09: ffff810102657d40
R10: 0000000000001000 R11: 00000000002da01c R12: ffff810006c05240
R13: 0000000000000ed0 R14: ffff810103c56bb0 R15: 0000000000000ed0
FS:  00002af6b3d96b00(0000) GS:ffff810100090b90(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000130 CR3: 000000010384a000 CR4: 00000000000006e0
Process vmsplice01 (pid: 18708, threadinfo ffff81010448c000, task ffff8101045d58f0)
Stack:  000000000000000e ffff81010448ddc8 ffff810103c56b48 ffff810101168df0
 ffff81010edf9660 0000000003c56b48 0000000000000000 ffff810103c56bb0
 ffff810103c56b48 0000000000000000 0000000000000000 ffffffff804f8380
Call Trace:
 [<ffffffff8029e2d6>] __splice_from_pipe+0x6d/0x1ea
 [<ffffffff8029dfc3>] pipe_to_file+0x0/0x2a6
 [<ffffffff8029dfc3>] pipe_to_file+0x0/0x2a6
 [<ffffffff8029e554>] splice_from_pipe+0x54/0x75
 [<ffffffff8029e611>] generic_file_splice_write+0x8a/0xfc
 [<ffffffff8029dd8d>] do_splice_from+0x72/0x7e
 [<ffffffff8029f274>] sys_splice+0x105/0x216
 [<ffffffff802096fe>] system_call+0x7e/0x83


Code: f3 a4 bf 01 00 00 00 e8 0b a9 f8 ff 65 48 8b 04 25 10 00 00 

Re: [patch] cache pipe buf page address for non-highmem arch

[Ken Chen]

On 3/26/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> LTP's vmsplice01 triggers the below:
>
> Unable to handle kernel NULL pointer dereference at 0000000000000130 RIP:
>  [<ffffffff8029e1b6>] pipe_to_file+0x1f3/0x2a6

Ouch, generic_pipe_buf_map() and unmap is used by both pipe and
splice, I better constrain all changes within fs/pipe.c because from
splice path, the page->private is not initialized.

Double ouch for missing test that code path.  Here is an updated
patch, tested with LTP and FIO.  I will write more test cases to make
sure all code path are covered.


diff --git a/fs/pipe.c b/fs/pipe.c
index ebafde7..4b55452 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -21,6 +21,21 @@
 #include <asm/uaccess.h>
 #include <asm/ioctls.h>

+#ifdef CONFIG_HIGHMEM
+#define pipe_kmap		kmap
+#define pipe_kmap_atomic	kmap_atomic
+#define pipe_kunmap		kunmap
+#define pipe_kunmap_atomic	kunmap_atomic
+#else	/* CONFIG_HIGHMEM */
+static inline void *pipe_kmap(struct page *page)
+{
+	return (void *) page->private;
+}
+#define pipe_kmap_atomic(page, type)	pipe_kmap(page)
+#define pipe_kunmap(page)		do { } while (0)
+#define pipe_kunmap_atomic(page, type)	do { } while (0)
+#endif
+
 /*
  * We use a start+len construction, which provides full use of the
  * allocated memory.
@@ -185,6 +200,27 @@ void generic_pipe_buf_unmap
 		kunmap(buf->page);
 }

+static void *pipe_buf_map(struct pipe_inode_info *pipe,
+			   struct pipe_buffer *buf, int atomic)
+{
+	if (atomic) {
+		buf->flags |= PIPE_BUF_FLAG_ATOMIC;
+		return pipe_kmap_atomic(buf->page, KM_USER0);
+	}
+
+	return pipe_kmap(buf->page);
+}
+
+static void pipe_buf_unmap(struct pipe_inode_info *pipe,
+			    struct pipe_buffer *buf, void *map_data)
+{
+	if (buf->flags & PIPE_BUF_FLAG_ATOMIC) {
+		buf->flags &= ~PIPE_BUF_FLAG_ATOMIC;
+		pipe_kunmap_atomic(map_data, KM_USER0);
+	} else
+		pipe_kunmap(buf->page);
+}
+
 int generic_pipe_buf_steal(struct pipe_inode_info *pipe,
 			   struct pipe_buffer *buf)
 {
@@ -210,8 +246,8 @@ int generic_pipe_buf_pin

 static const struct pipe_buf_operations anon_pipe_buf_ops = {
 	.can_merge = 1,
-	.map = generic_pipe_buf_map,
-	.unmap = generic_pipe_buf_unmap,
+	.map = pipe_buf_map,
+	.unmap = pipe_buf_unmap,
 	.pin = generic_pipe_buf_pin,
 	.release = anon_pipe_buf_release,
 	.steal = generic_pipe_buf_steal,
@@ -316,6 +352,7 @@ redo:
 		if (do_wakeup) {
 			wake_up_interruptible_sync(&pipe->wait);
  			kill_fasync(&pipe->fasync_writers, SIGIO, POLL_OUT);
+			do_wakeup = 0;
 		}
 		pipe_wait(pipe);
 	}
@@ -423,6 +460,8 @@ redo1:
 					ret = ret ? : -ENOMEM;
 					break;
 				}
+				page->private = (unsigned long)
+							page_address(page);
 				pipe->tmp_page = page;
 			}
 			/* Always wake up, even if the copy fails. Otherwise
@@ -438,16 +477,16 @@ redo1:
 			iov_fault_in_pages_read(iov, chars);
 redo2:
 			if (atomic)
-				src = kmap_atomic(page, KM_USER0);
+				src = pipe_kmap_atomic(page, KM_USER0);
 			else
-				src = kmap(page);
+				src = pipe_kmap(page);

 			error = pipe_iov_copy_from_user(src, iov, chars,
 							atomic);
 			if (atomic)
-				kunmap_atomic(src, KM_USER0);
+				pipe_kunmap_atomic(src, KM_USER0);
 			else
-				kunmap(page);
+				pipe_kunmap(page);

 			if (unlikely(error)) {
 				if (atomic) {

Re: [patch] cache pipe buf page address for non-highmem arch

[Zach Brown]

> +#define pipe_kmap_atomic(page, type)	pipe_kmap(page)
> +#define pipe_kunmap(page)		do { } while (0)
> +#define pipe_kunmap_atomic(page, type)	do { } while (0)

Please don't drop arguments in stubs.  It can let completely broken  
code compile, like:

	pipe_kunmap(SOME_COMPLETE_NONSENSE);

Static inlines with empty bodies are the gold standard.

- z

Re: [patch] cache pipe buf page address for non-highmem arch

[Andrew Morton]

On Tue, 27 Mar 2007 15:57:53 -0700
Zach Brown <zach.brown@oracle.com> wrote:

> > +#define pipe_kmap_atomic(page, type)	pipe_kmap(page)
> > +#define pipe_kunmap(page)		do { } while (0)
> > +#define pipe_kunmap_atomic(page, type)	do { } while (0)
> 
> Please don't drop arguments in stubs.  It can let completely broken  
> code compile, like:
> 
> 	pipe_kunmap(SOME_COMPLETE_NONSENSE);
> 
> Static inlines with empty bodies are the gold standard.
> 

yup.

<fiddle, fiddle>

Does this look OK?

#ifdef CONFIG_HIGHMEM
static inline void *pipe_kmap(struct page *page)
{
	return kmap(page);
}

static inline void pipe_kunmap(struct page *page)
{
	kunmap(page);
}

static inline void *pipe_kmap_atomic(struct page *page, enum km_type type)
{
	return kmap_atomic(page, type);
}

static inline void pipe_kunmap_atomic(void *addr, enum km_type type)
{
	kunmap_atomic(addr, type);
}
#else	/* CONFIG_HIGHMEM */
static inline void *pipe_kmap(struct page *page)
{
	return (void *)page->private;
}

static inline void pipe_kunmap(struct page *page)
{
}

static inline void *pipe_kmap_atomic(struct page *page, enum km_type type)
{
	return (void *)page->private;
}

static inline void pipe_kunmap_atomic(struct page *page, enum km_type type)
{
}
#endif

Re: [patch] cache pipe buf page address for non-highmem arch

[Zach Brown]

> Does this look OK?

Almost...

> #ifdef CONFIG_HIGHMEM
> static inline void pipe_kunmap_atomic(void *addr, enum km_type type)
> #else	/* CONFIG_HIGHMEM */
> static inline void pipe_kunmap_atomic(struct page *page, enum  
> km_type type)

- z

Re: [patch] cache pipe buf page address for non-highmem arch

[Andrew Morton]

On Wed, 28 Mar 2007 16:21:04 -0700
Zach Brown <zach.brown@oracle.com> wrote:

> > Does this look OK?
> 
> Almost...
> 
> > #ifdef CONFIG_HIGHMEM
> > static inline void pipe_kunmap_atomic(void *addr, enum km_type type)
> > #else	/* CONFIG_HIGHMEM */
> > static inline void pipe_kunmap_atomic(struct page *page, enum  
> > km_type type)
> 

OK, I give up.  What are you telling me here?

<compiles it>

argh, enum km_type isn't defined if !CONFIG_HIGHMEM, which is extravagantly
dumb.


From: Andrew Morton <akpm@linux-foundation.org>

Cc: "Ken Chen" <kenchen@google.com>
Cc: Zach Brown <zach.brown@oracle.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/pipe.c |   31 +++++++++++++++++++++++++------
 1 files changed, 25 insertions(+), 6 deletions(-)

diff -puN fs/pipe.c~cache-pipe-buf-page-address-for-non-highmem-arch-fix-tidy fs/pipe.c
--- a/fs/pipe.c~cache-pipe-buf-page-address-for-non-highmem-arch-fix-tidy
+++ a/fs/pipe.c
@@ -22,17 +22,36 @@
 #include <asm/ioctls.h>
 
 #ifdef CONFIG_HIGHMEM
-#define pipe_kmap		kmap
-#define pipe_kmap_atomic	kmap_atomic
-#define pipe_kunmap		kunmap
-#define pipe_kunmap_atomic	kunmap_atomic
+static inline void *pipe_kmap(struct page *page)
+{
+	return kmap(page);
+}
+
+static inline void pipe_kunmap(struct page *page)
+{
+	kunmap(page);
+}
+
+static inline void *pipe_kmap_atomic(struct page *page, enum km_type type)
+{
+	return kmap_atomic(page, type);
+}
+
+static inline void pipe_kunmap_atomic(void *addr, enum km_type type)
+{
+	kunmap_atomic(addr, type);
+}
 #else	/* CONFIG_HIGHMEM */
 static inline void *pipe_kmap(struct page *page)
 {
-	return (void *) page->private;
+	return (void *)page->private;
 }
+
+static inline void pipe_kunmap(struct page *page)
+{
+}
+
 #define pipe_kmap_atomic(page, type)	pipe_kmap(page)
-#define pipe_kunmap(page)		do { } while (0)
 #define pipe_kunmap_atomic(page, type)	do { } while (0)
 #endif
 
_

Re: [patch] cache pipe buf page address for non-highmem arch

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Wed, 28 Mar 2007 16:21:04 -0700
> Zach Brown <zach.brown@oracle.com> wrote:
>
>   
>>> Does this look OK?
>>>       
>> Almost...
>>
>>     
>>> #ifdef CONFIG_HIGHMEM
>>> static inline void pipe_kunmap_atomic(void *addr, enum km_type type)
>>> #else	/* CONFIG_HIGHMEM */
>>> static inline void pipe_kunmap_atomic(struct page *page, enum  
>>> km_type type)
>>>       
>
> OK, I give up.  What are you telling me here?
>   

Also void *addr vs struct page *page.

    J

Re: [patch] cache pipe buf page address for non-highmem arch

[Christoph Hellwig]

On Thu, Mar 22, 2007 at 05:51:11PM -0700, Ken Chen wrote:
> It is really sad that we always call kmap and friends for every pipe
> buffer page on 64-bit arch that doesn't use HIGHMEM, or on
> configuration that doesn't turn on HIGHMEM.
> 
> The effect of calling kmap* is visible in the execution profile when
> pipe code is being stressed.  It is especially true on amd's x86-64
> platform where kmap() has to traverse through numa node index
> calculation in order to convert struct page * to kernel virtual
> address.  It is fairly pointless to perform that calculation repeatly
> on system with no highmem (i.e., 64-bit arch like x86-64).  This patch
> caches kernel pipe buffer page's kernel vaddr to speed up pipe buffer
> mapping functions.
> 
> There is another suboptimal block in pipe_read() where wake_up is
> called twice.  I think it was an oversight since in pipe_write(), it
> looks like it is doing the right thing.

I think you're fixing the symptom here and not the cause.  If calculating
the virtual address of a page is so expensive on your setup it should
define WANT_PAGE_VIRTUAL and we should always cache the virtual address
in struct page.  There's a lot more code, epecially in filesystems that's
rather upset about a slow page_address.

Re: [patch] cache pipe buf page address for non-highmem arch

[Ken Chen]

On 3/23/07, Christoph Hellwig <hch@infradead.org> wrote:
> I think you're fixing the symptom here and not the cause.  If calculating
> the virtual address of a page is so expensive on your setup it should
> define WANT_PAGE_VIRTUAL and we should always cache the virtual address
> in struct page.  There's a lot more code, epecially in filesystems that's
> rather upset about a slow page_address.

Adding WANT_PAGE_VIRTUAL would be too costly I think as it will expand
the sizeof struct page.  I would rather go incremental fix on these
issues and not blindly increase page struct size.

Re: [patch] cache pipe buf page address for non-highmem arch

[Benjamin LaHaise]

On Fri, Mar 23, 2007 at 10:14:52AM +0000, Christoph Hellwig wrote:
> I think you're fixing the symptom here and not the cause.  If calculating
> the virtual address of a page is so expensive on your setup it should
> define WANT_PAGE_VIRTUAL and we should always cache the virtual address
> in struct page.  There's a lot more code, epecially in filesystems that's
> rather upset about a slow page_address.

Andi shot that down when I brought it up a while ago, as it does show 
up in profiles for networking and other code paths.  His argument is that 
the loss of memory is excessive.  Personally, I think the benefits of a 
64 byte struct page on x86-64 outweigh the memory loss, as it means page 
index to address translations are a simple shift.

		-ben
-- 
"Time is of no importance, Mr. President, only life is important."
Don't Email: <zyntrop@kvack.org>.

Re: [patch] cache pipe buf page address for non-highmem arch

[Andi Kleen]

Benjamin LaHaise <bcrl@kvack.org> writes:

> On Fri, Mar 23, 2007 at 10:14:52AM +0000, Christoph Hellwig wrote:
> > I think you're fixing the symptom here and not the cause.  If calculating
> > the virtual address of a page is so expensive on your setup it should
> > define WANT_PAGE_VIRTUAL and we should always cache the virtual address
> > in struct page.  There's a lot more code, epecially in filesystems that's
> > rather upset about a slow page_address.
> 
> Andi shot that down when I brought it up a while ago, as it does show 
> up in profiles for networking and other code paths.  His argument is that 
> the loss of memory is excessive.  Personally, I think the benefits of a 
> 64 byte struct page on x86-64 outweigh the memory loss, as it means page 
> index to address translations are a simple shift.

At some point I had plans to make virt_to_page() etc. faster
by using multiple look up tables that precompute some of the computation
depending on the caller. But it might possibly cause more cache misses
and the function didn't seem that time critical back then.

But if you can really measure it so clearly it might be worth revisiting
that. 

-Andi

Re: [patch] cache pipe buf page address for non-highmem arch

[Andi Kleen]

"Ken Chen" <kenchen@google.com> writes:

> It is really sad that we always call kmap and friends for every pipe
> buffer page on 64-bit arch that doesn't use HIGHMEM, or on
> configuration that doesn't turn on HIGHMEM.
> 
> The effect of calling kmap* is visible in the execution profile when
> pipe code is being stressed.  It is especially true on amd's x86-64
> platform where kmap() has to traverse through numa node index
> calculation in order to convert struct page * to kernel virtual
> address.  

What is the problem? You have cache misses on the the hash table
or are the instructions really an issue on a modern CPU?

e.g. i out of lined virt_to_page to save space, but it could be probably
inlined again if it was severly time critical. 

> +
> +#ifdef CONFIG_HIGHMEM
> +#define pipe_kmap		kmap
> +#define pipe_kmap_atomic	kmap_atomic

I think it would be better to have a somewhat generic kmap_caching() 
or similar interface that could be used by more subsystems.

-Andi

Re: [patch] cache pipe buf page address for non-highmem arch

[Ken Chen]

On 27 Mar 2007 17:01:01 +0200, Andi Kleen <andi@firstfloor.org> wrote:
> What is the problem? You have cache misses on the the hash table
> or are the instructions really an issue on a modern CPU?

It was page_to_pfn() on numa system.  I think it's cache misses on per
node pgdat lookup with node index calculation on each pipe buffer
page.


> e.g. i out of lined virt_to_page to save space, but it could be probably
> inlined again if it was severly time critical.

That would be a problem too, though not showing up in the pipe code path.