Commit 23aee091 authored by Jeff Moyer's avatar Jeff Moyer Committed by Linus Torvalds

dio: don't zero out the pages array inside struct dio

Intel reported a performance regression caused by the following commit:

commit 848c4dd5
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:

      6a648fa7

    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%).

[akpm@linux-foundation.org: add comment]
Signed-off-by: default avatarJeff Moyer <jmoyer@redhat.com>
Acked-by: default avatarZach Brown <zach.brown@oracle.com>
Signed-off-by: default avatarAndrew Morton <akpm@linux-foundation.org>
Signed-off-by: default avatarLinus Torvalds <torvalds@linux-foundation.org>
parent fac046ad
...@@ -104,6 +104,18 @@ struct dio { ...@@ -104,6 +104,18 @@ struct dio {
unsigned cur_page_len; /* Nr of bytes at cur_page_offset */ unsigned cur_page_len; /* Nr of bytes at cur_page_offset */
sector_t cur_page_block; /* Where it starts */ sector_t cur_page_block; /* Where it starts */
/* BIO completion state */
spinlock_t bio_lock; /* protects BIO fields below */
unsigned long refcount; /* direct_io_worker() and bios */
struct bio *bio_list; /* singly linked via bi_private */
struct task_struct *waiter; /* waiting task (NULL if none) */
/* AIO related stuff */
struct kiocb *iocb; /* kiocb */
int is_async; /* is IO async ? */
int io_error; /* IO error in completion path */
ssize_t result; /* IO result */
/* /*
* Page fetching state. These variables belong to dio_refill_pages(). * Page fetching state. These variables belong to dio_refill_pages().
*/ */
...@@ -115,22 +127,16 @@ struct dio { ...@@ -115,22 +127,16 @@ struct dio {
* Page queue. These variables belong to dio_refill_pages() and * Page queue. These variables belong to dio_refill_pages() and
* dio_get_page(). * dio_get_page().
*/ */
struct page *pages[DIO_PAGES]; /* page buffer */
unsigned head; /* next page to process */ unsigned head; /* next page to process */
unsigned tail; /* last valid page + 1 */ unsigned tail; /* last valid page + 1 */
int page_errors; /* errno from get_user_pages() */ int page_errors; /* errno from get_user_pages() */
/* BIO completion state */ /*
spinlock_t bio_lock; /* protects BIO fields below */ * pages[] (and any fields placed after it) are not zeroed out at
unsigned long refcount; /* direct_io_worker() and bios */ * allocation time. Don't add new fields after pages[] unless you
struct bio *bio_list; /* singly linked via bi_private */ * wish that they not be zeroed.
struct task_struct *waiter; /* waiting task (NULL if none) */ */
struct page *pages[DIO_PAGES]; /* page buffer */
/* AIO related stuff */
struct kiocb *iocb; /* kiocb */
int is_async; /* is IO async ? */
int io_error; /* IO error in completion path */
ssize_t result; /* IO result */
}; };
/* /*
...@@ -1151,10 +1157,16 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode, ...@@ -1151,10 +1157,16 @@ __blockdev_direct_IO(int rw, struct kiocb *iocb, struct inode *inode,
} }
} }
dio = kzalloc(sizeof(*dio), GFP_KERNEL); dio = kmalloc(sizeof(*dio), GFP_KERNEL);
retval = -ENOMEM; retval = -ENOMEM;
if (!dio) if (!dio)
goto out; goto out;
/*
* Believe it or not, zeroing out the page array caused a .5%
* performance regression in a database benchmark. So, we take
* care to only zero out what's needed.
*/
memset(dio, 0, offsetof(struct dio, pages));
/* /*
* For block device access DIO_NO_LOCKING is used, * For block device access DIO_NO_LOCKING is used,
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment