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

[Glauber Costa]

On Thu, Sep 03, 2009 at 04:43:27PM +0300, Avi Kivity wrote:
> 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.
agree.

>
>>> 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().
Not at all. If we do queue_work once, it will execute whatever function you ask 
for
in the correct thread, and for there on, everything will be local. So even if 
you nest
functions that call queue_work themselves, only the first will involve 
synchronization at all.

  
>> 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.
I am pretty much convinced by now.

>
>>> 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.
For a masked vigilante like you, I thought it was clear enough that "fierce 
opposition"
automatically meant you got strong reasons, and were actually right.