On Tue, 05 Jul 2011 16:35:21 -0400
Jeff Moyer <jmoyer@xxxxxxxxxx> wrote:
> Hi,
>
> When looking at perf data for my aio-stress test runs against some
> relatively fast storage, I noticed that __aio_get_req was taking up more
> CPU than I expected. The command line for aio-stress was:
> aio-stress -O -r 8 -d 256 -b 32 -l -s 16384 /dev/sdk
> which means we're sending 32 iocbs to io_submit at a time. Here's the
> perf report -g output:
>
> Events: 41K cycles
> - 7.33% aio-stress [kernel.kallsyms] [k] _raw_spin_lock_irq
> - _raw_spin_lock_irq
> + 75.38% scsi_request_fn
> - 10.67% __aio_get_req
> io_submit_one
>
> The spinlock is the ctx_lock, taken when allocating the request. This
> seemed like pretty low-hanging fruit to me, so I cooked up a patch to
> allocate 32 kiocbs at a time. This is the result:
>
> Events: 50K cycles
> - 5.54% aio-stress [kernel.kallsyms] [k] _raw_spin_lock_irq
> - _raw_spin_lock_irq
> + 83.23% scsi_request_fn
> - 5.14% io_submit_one
>
> So, aio-stress now takes up ~2% less CPU. This seems like a worth-while
> thing to do, but I don't have a system handy that would show any real
> performance gains from it. The higher the number of iocbs passed to
> io_submit, the more of a win this will be.
>
> FWIW, I tested this with multiple different batch sizes (1, 32, 33, 64,
> 256) and it didn't blow up.
>
So the benefit comes from reducing the frequency with which ctx_lock is
taken, rather than from reducing its per-acquisition hold times.
>
> ...
>
> +#define KIOCB_STASH_SIZE 32
> +struct kiocb_stash {
> + unsigned head;
> + unsigned tail;
> + unsigned total;
> + unsigned used;
> + struct kiocb *kiocbs[KIOCB_STASH_SIZE];
> +};
Laziness begets laziness: you didn't bother documenting how this ring
thing works and why it exists, so I didn't bother reverse-engineering
that infomation.
Once all this has been fixed, I might be wondering why it was done with
a yukky fixed-size array rather than with a list.
A pox on you for telling us there's a "potential starvation case" but
not telling us what it is! It's a kmalloc failure - why is this
described as "starvation"?
Once all this has been fixed, I might be wondering how we test the
"potential starvation case".
Various trivial fixies:
From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
- coding-style fixes
- improve variable name(s)
- fix typo(s)
- use new kmap_atomic() interface
- change comment to use the number of columns God gave us
- use "bool" for a boolean
Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
---
fs/aio.c | 30 ++++++++++++++++--------------
1 file changed, 16 insertions(+), 14 deletions(-)
diff -puN fs/aio.c~aio-allocate-kiocbs-in-batches-fix fs/aio.c
--- a/fs/aio.c~aio-allocate-kiocbs-in-batches-fix
+++ a/fs/aio.c
@@ -468,12 +468,14 @@ struct kiocb_stash {
unsigned used;
struct kiocb *kiocbs[KIOCB_STASH_SIZE];
};
-static void kiocb_stash_init(struct kiocb_stash *stash, int nr)
+
+static void kiocb_stash_init(struct kiocb_stash *stash, int total)
{
stash->head = stash->tail = 0;
- stash->total = nr;
+ stash->total = total;
stash->used = 0;
}
+
static void kiocb_stash_free(struct kiocb_stash *stash, int used)
{
int i;
@@ -481,24 +483,25 @@ static void kiocb_stash_free(struct kioc
for (i = used; i < stash->total; i++)
kmem_cache_free(kiocb_cachep, stash->kiocbs[i]);
}
+
static inline unsigned kiocb_stash_avail(struct kiocb_stash *stash)
{
return stash->tail - stash->head;
}
/*
- * Allocate nr kiocbs. This aovids taking and dropping the lock a
+ * Allocate nr kiocbs. This avoids taking and dropping the lock a
* lot during setup.
*/
static int kiocb_stash_refill(struct kioctx *ctx, struct kiocb_stash *stash)
{
int i, nr = KIOCB_STASH_SIZE;
int avail;
- int called_fput = 0;
+ bool called_fput;
struct aio_ring *ring;
/*
- * Allocate the requests up-front. Later, we'll trim the list
+ * Allocate the requests up front. Later, we'll trim the list
* if there isn't enough room in the completion ring.
*/
stash->head = 0;
@@ -509,15 +512,14 @@ static int kiocb_stash_refill(struct kio
for (i = 0; i < nr; i++) {
stash->kiocbs[i] = __aio_get_req(ctx);
if (!stash->kiocbs[i] && !called_fput) {
- /* Handle a potential starvation case --
- * should be exceedingly rare as requests will
- * be stuck on fput_head only if the
- * aio_fput_routine is delayed and the
- * requests were the last user of the struct
- * file.
+ /*
+ * Handle a potential starvation case. Should be
+ * exceedingly rare as requests will be stuck on
+ * fput_head only if the aio_fput_routine is delayed and
+ * the requests were the last user of the struct file.
*/
aio_fput_routine(NULL);
- called_fput = 1;
+ called_fput = true;
i--;
continue;
}
@@ -525,7 +527,7 @@ static int kiocb_stash_refill(struct kio
stash->tail = i;
spin_lock_irq(&ctx->ctx_lock);
- ring = kmap_atomic(ctx->ring_info.ring_pages[0], KM_USER0);
+ ring = kmap_atomic(ctx->ring_info.ring_pages[0]);
avail = aio_ring_avail(&ctx->ring_info, ring) + ctx->reqs_active;
if (avail < stash->tail) {
@@ -538,7 +540,7 @@ static int kiocb_stash_refill(struct kio
list_add(&stash->kiocbs[i]->ki_list, &ctx->active_reqs);
ctx->reqs_active++;
}
- kunmap_atomic(ring, KM_USER0);
+ kunmap_atomic(ring);
spin_unlock_irq(&ctx->ctx_lock);
if (stash->tail == 0)
_
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
