[Qemu-devel] [PATCH] posix-aio-compat: Fix idle_threads counter

[Qemu-devel] [PATCH] posix-aio-compat: Fix idle_threads counter

[Kevin Wolf]

A thread should only be counted as idle when it really is waiting for new
requests. Without this patch, sometimes too few threads are started as busy
threads are counted as idle.

Not sure if it makes a difference in practice outside some artificial
qemu-io/qemu-img tests, but I think the change makes sense in any case.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 posix-aio-compat.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index 6d4df9d..f3cc868 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -322,7 +322,9 @@ static void *aio_thread(void *unused)
 
         while (QTAILQ_EMPTY(&request_list) &&
                !(ret == ETIMEDOUT)) {
+            idle_threads++;
             ret = cond_timedwait(&cond, &lock, &ts);
+            idle_threads--;
         }
 
         if (QTAILQ_EMPTY(&request_list))
@@ -331,7 +333,6 @@ static void *aio_thread(void *unused)
         aiocb = QTAILQ_FIRST(&request_list);
         QTAILQ_REMOVE(&request_list, aiocb, node);
         aiocb->active = 1;
-        idle_threads--;
         mutex_unlock(&lock);
 
         switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
@@ -353,13 +354,11 @@ static void *aio_thread(void *unused)
 
         mutex_lock(&lock);
         aiocb->ret = ret;
-        idle_threads++;
         mutex_unlock(&lock);
 
         if (kill(pid, aiocb->ev_signo)) die("kill failed");
     }
 
-    idle_threads--;
     cur_threads--;
     mutex_unlock(&lock);
 
@@ -371,7 +370,6 @@ static void spawn_thread(void)
     sigset_t set, oldset;
 
     cur_threads++;
-    idle_threads++;
 
     /* block all signals */
     if (sigfillset(&set)) die("sigfillset");
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] posix-aio-compat: Fix idle_threads counter

[Stefan Hajnoczi]

On Tue, May 3, 2011 at 2:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> A thread should only be counted as idle when it really is waiting for new
> requests. Without this patch, sometimes too few threads are started as busy
> threads are counted as idle.
>
> Not sure if it makes a difference in practice outside some artificial
> qemu-io/qemu-img tests, but I think the change makes sense in any case.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  posix-aio-compat.c |    6 ++----
>  1 files changed, 2 insertions(+), 4 deletions(-)

I think the critical change here is that idle_threads is not being
incremented by spawn_thread().  This means that we will keep spawning
threads as new requests come in and until the first thread goes idle.

Previously you could imagine a scenario where we spawn a thread but
don't schedule it yet.  Then we immediately submit more I/O and since
idle_threads was incremented we don't spawn additional threads to
handle the requests.

Are these the cases you were thinking about?

I like your patch because it reduces the number of places where we
mess with idle_threads.  I'd move the increment/decrement out of the
loop since the mutex is held here anyway so no one will test
idle_threads between loop iterations, but it's up to you if you want
to send a v2 or not.

Stefan

Re: [Qemu-devel] [PATCH] posix-aio-compat: Fix idle_threads counter

[Kevin Wolf]

Am 03.05.2011 17:56, schrieb Stefan Hajnoczi:
> On Tue, May 3, 2011 at 2:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>> A thread should only be counted as idle when it really is waiting for new
>> requests. Without this patch, sometimes too few threads are started as busy
>> threads are counted as idle.
>>
>> Not sure if it makes a difference in practice outside some artificial
>> qemu-io/qemu-img tests, but I think the change makes sense in any case.
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>  posix-aio-compat.c |    6 ++----
>>  1 files changed, 2 insertions(+), 4 deletions(-)
> 
> I think the critical change here is that idle_threads is not being
> incremented by spawn_thread().  This means that we will keep spawning
> threads as new requests come in and until the first thread goes idle.
> 
> Previously you could imagine a scenario where we spawn a thread but
> don't schedule it yet.  Then we immediately submit more I/O and since
> idle_threads was incremented we don't spawn additional threads to
> handle the requests.
> 
> Are these the cases you were thinking about?

Yes, this is the case that I noticed.

However, I'm not sure if this is really the critical change. In this
case, it would take a bit longer until you get your full 64 threads, but
you should get there eventually and then it shouldn't impact performance
any more.

However, what I saw in my test case (qemu-img always running 16
sequential read requests in parallel) was that I only got 16 threads.
This sounds logical, but in fact you seem to need always one thread more
for good performance (I don't fully understand this yet). And with this
patch, you actually get 17 threads. The difference was like 8s vs. 22s
for the same requests, and using more than 17 threads doesn't further
improve it.

This is what makes me a bit nervous about it, I don't really understand
what's going on here. The observation suggests that the race doesn't
only affect thread creation, but also operations afterwards.

> I like your patch because it reduces the number of places where we
> mess with idle_threads.  I'd move the increment/decrement out of the
> loop since the mutex is held here anyway so no one will test
> idle_threads between loop iterations, but it's up to you if you want
> to send a v2 or not.

Probably doesn't really make a difference.

Kevin

Re: [Qemu-devel] [PATCH] posix-aio-compat: Fix idle_threads counter

[Stefan Hajnoczi]

On Tue, May 3, 2011 at 5:53 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 03.05.2011 17:56, schrieb Stefan Hajnoczi:
>> On Tue, May 3, 2011 at 2:26 PM, Kevin Wolf <kwolf@redhat.com> wrote:
>>> A thread should only be counted as idle when it really is waiting for new
>>> requests. Without this patch, sometimes too few threads are started as busy
>>> threads are counted as idle.
>>>
>>> Not sure if it makes a difference in practice outside some artificial
>>> qemu-io/qemu-img tests, but I think the change makes sense in any case.
>>>
>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>>> ---
>>>  posix-aio-compat.c |    6 ++----
>>>  1 files changed, 2 insertions(+), 4 deletions(-)
>>
>> I think the critical change here is that idle_threads is not being
>> incremented by spawn_thread().  This means that we will keep spawning
>> threads as new requests come in and until the first thread goes idle.
>>
>> Previously you could imagine a scenario where we spawn a thread but
>> don't schedule it yet.  Then we immediately submit more I/O and since
>> idle_threads was incremented we don't spawn additional threads to
>> handle the requests.
>>
>> Are these the cases you were thinking about?
>
> Yes, this is the case that I noticed.
>
> However, I'm not sure if this is really the critical change. In this
> case, it would take a bit longer until you get your full 64 threads, but
> you should get there eventually and then it shouldn't impact performance
> any more.
>
> However, what I saw in my test case (qemu-img always running 16
> sequential read requests in parallel) was that I only got 16 threads.
> This sounds logical, but in fact you seem to need always one thread more
> for good performance (I don't fully understand this yet). And with this
> patch, you actually get 17 threads. The difference was like 8s vs. 22s
> for the same requests, and using more than 17 threads doesn't further
> improve it.

Wow, 8s vs 22s is a big difference.  Did you run any guest benchmarks?

Stefan