Re: [Qemu-devel] [RFC] queue_work proposal

[Avi Kivity <avi@redhat.com>]

On 09/03/2009 03:11 PM, Glauber Costa wrote:
> On Thu, Sep 03, 2009 at 02:32:45PM +0300, Avi Kivity wrote:
>    
>> On 09/03/2009 02:15 PM, Glauber Costa wrote:
>>      
>>>        
>>>> on_vcpu() and queue_work() are fundamentally different (yes, I see the
>>>> wait parameter, and I think there should be two separate functions for
>>>> such different behaviours).
>>>>
>>>>          
>>> Therefore, the name change. The exact on_vcpu behaviour, however, can be
>>> implemented ontop of queue_work().
>>>        
>> Will there be any use for asynchronous queue_work()?
>>
>> It's a dangerous API.
>>      
> Initially, I thought we could use it for batching, if we forced a flush in the end of
> a sequence of operations. This can makes things faster if we are doing a bunch of calls
> in a row, from the wrong thread.
>    

It's a lot easier and safer to write a function that does your batch job 
and on_vcpu() it.

>> I think it's reasonable to demand that whoever calls kvm_vcpu_ioctl()
>> know what they are doing (and they'll get surprising results if it
>> switches threads implicitly).
>>      
> I respectfully disagree. Not that I want people not to know what they are doing, but I
> believe that, forcing something that can only run in a specific thread to be run there,
> provides us with a much saner interface, that will make code a lot more readable and
> maintainable.
>    

Example:

   kvm_vcpu_ioctl(get_regs)
   regs->rflags |= some_flag
   kvm_vcpu_ioctl(set_regs)

This looks totally sane but is racy if kvm_vcpu_ioctl() does an implicit 
on_vcpu().

>> One of them is synchronous, meaning the data can live on stack and no
>> special synchronization is needed, while the other is synchronous,
>> meaning explicit memory management and end-of-work synchronization is
>> needed.
>>      
> I will assume you meant "the other is assynchronous". It does not need to be.
> I though about including the assynchronous version in this RFC to let doors
> open for performance improvements *if* we needed them. But again: the absolute
> majority of the calls will be local. So it is not that important.
>    

Then there's even less reason to include the async version.

>> on_vcpu() is a subset of queue_work().  I meant, why to we need the
>> extra functionality?
>>      
> As I said, if you oppose it hardly, we don't really need to.
>    

I do oppose it, but the reason for not including it should be the 
reasons I cited, not that I oppose it.

>> A more powerful API comes with increased responsibilities.
>>      
> You suddenly sounds like spider man.
>    

I hate it when I get unmasked.

-- 
error compiling committee.c: too many arguments to function