Re: [Qemu-devel] [PATCH] Refactor AIO to allow multiple AIO implementations

[Anthony Liguori <aliguori@us.ibm.com>]

Blue Swirl wrote:
> On 9/16/08, Anthony Liguori <aliguori@us.ibm.com> wrote:
>   
>> This patch refactors the AIO layer to allow multiple AIO implementations.  It's
>>  only possible because of the recent signalfd() patch.
>>     
>
>   
>>  +/* This is a simple lock used to protect the aio_handlers list.  Specifically,
>>  + * it's used to ensure that no callbacks are removed while we're walking and
>>  + * dispatching callbacks.
>>  + */
>>  +static int walking_handlers;
>>     
>
> Shouldn't this be volatile and/or atomic_t?
>   

No, because this is just used to protect the aio_handlers list within 
the same thread.  Here's the scenario we're protecting:

Walk aio_handlers list =>
   call handler =>
      handler calls qemu_aio_set_fd_handler() to delete aio entry

Since we're walking the list, we can't delete an entry while walking.  
This is the same problem we have in vl.c with the IOHandler list.  
Unlike the IOHandler list, we don't walk the aio_handler list often 
enough to just set a flag on the entry.  In the case where 
qemu_aio_set_fd_handler() is called from something other than a handler 
callback, we need to be able to directly delete the entry element.  This 
is why we use the "lock", to detect that case.

It's not a real lock, because two threads are never trying to access it 
at the same time.  This is why we don't need volatile, atomic_t, or the 
real locking primitives.

Regards,

Anthony Liguori

> Just wondering, why don't you use real locking operations, for example
> those in qemu-lock.h?
>