lkml.org 
[lkml]   [2009]   [Oct]   [30]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch] aio: Don't zero out the pages array inside struct dio
On Fri, 30 Oct 2009 09:39:55 -0400
Jeff Moyer <jmoyer@redhat.com> wrote:

> Hi,
>
> Intel reported a performance regression caused by the following commit:
>
> commit 848c4dd5153c7a0de55470ce99a8e13a63b4703f
> Author: Zach Brown <zach.brown@oracle.com>
> Date: Mon Aug 20 17:12:01 2007 -0700
>
> dio: zero struct dio with kzalloc instead of manually
>
> This patch uses kzalloc to zero all of struct dio rather than
> manually trying to track which fields we rely on being zero. It
> passed aio+dio stress testing and some bug regression testing on
> ext3.
>
> This patch was introduced by Linus in the conversation that lead up
> to Badari's minimal fix to manually zero .map_bh.b_state in commit:
>
> 6a648fa72161d1f6468dabd96c5d3c0db04f598a
>
> It makes the code a bit smaller. Maybe a couple fewer cachelines to
> load, if we're lucky:
>
> text data bss dec hex filename
> 3285925 568506 1304616 5159047 4eb887 vmlinux
> 3285797 568506 1304616 5158919 4eb807 vmlinux.patched
>
> I was unable to measure a stable difference in the number of cpu
> cycles spent in blockdev_direct_IO() when pushing aio+dio 256K reads
> at ~340MB/s.
>
> So the resulting intent of the patch isn't a performance gain but to
> avoid exposing ourselves to the risk of finding another field like
> .map_bh.b_state where we rely on zeroing but don't enforce it in the
> code.
>
> Zach surmised that zeroing out the page array was what caused most of
> the problem, and suggested the approach taken in the attached patch for
> resolving the issue. Intel re-tested with this patch and saw a 0.6%
> performance gain (the original regression was 0.5%).
>
> Comments, as always, are appreciated.
>

You forgot something:

--- a/fs/direct-io.c~aio-dont-zero-out-the-pages-array-inside-struct-dio-fix
+++ a/fs/direct-io.c
@@ -130,6 +130,12 @@ struct dio {
unsigned head; /* next page to process */
unsigned tail; /* last valid page + 1 */
int page_errors; /* errno from get_user_pages() */
+
+ /*
+ * pages[] (and any fields placed after it) are not zeroed out at
+ * allocation time. Don't add new fields after pages[] unless you
+ * wish that they not be zeroed.
+ */
struct page *pages[DIO_PAGES]; /* page buffer */
};

_


\
 
 \ /
  Last update: 2009-10-30 22:21    [from the cache]
©2003-2011 Jasper Spaans