Re: [PATCH 1/3] kvm-s390: infrastructure to kick vcpus out of guest state

[Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>]

Marcelo Tosatti wrote:
> On Tue, May 26, 2009 at 10:02:59AM +0200, Christian Ehrhardt wrote:
>   
>> Marcelo Tosatti wrote:
>>     
>>> On Mon, May 25, 2009 at 01:40:49PM +0200, ehrhardt@linux.vnet.ibm.com wrote:
>>>   
>>>       
>>>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>>>
>>>> To ensure vcpu's come out of guest context in certain cases this patch adds a
>>>> s390 specific way to kick them out of guest context. Currently it kicks them
>>>> out to rerun the vcpu_run path in the s390 code, but the mechanism itself is
>>>> expandable and with a new flag we could also add e.g. kicks to userspace etc.
>>>>
>>>> Signed-off-by: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>>>     
>>>>         
>>> "For now I added the optimization to skip kicking vcpus out of guest
>>> that had the request bit already set to the s390 specific loop (sent as
>>> v2 in a few minutes).
>>>
>>> We might one day consider standardizing some generic kickout levels e.g.
>>> kick to "inner loop", "arch vcpu run", "generic vcpu run", "userspace",
>>> ... whatever levels fit *all* our use cases. And then let that kicks be
>>> implemented in an kvm_arch_* backend as it might be very different how
>>> they behave on different architectures."
>>>
>>> That would be ideal, yes. Two things make_all_requests handles: 
>>>
>>> 1) It disables preemption with get_cpu(), so it can reliably check for
>>> cpu id. Somehow you don't need that for s390 when kicking multiple
>>> vcpus?
>>>   
>>>       
>> I don't even need the cpuid as make_all_requests does, I just insert a  
>> special bit in the vcpu arch part and the vcpu will "come out to me 
>> (host)".
>> Fortunateley the kick is rare and fast so I can just insert it  
>> unconditionally (it's even ok to insert it if the vcpu is not in guest  
>> state). That prevents us from needing vcpu lock or detailed checks which  
>> would end up where we started (no guarantee that vcpu's come out of  
>> guest context while trying to aquire all vcpu locks)
>>     
>
> Let me see if I get this right: you kick the vcpus out of guest mode by
> using a special bit in the vcpu arch part. OK.
>
> What I don't understand is this: 
> "would end up where we started (no guarantee that vcpu's come out of
> guest context while trying to aquire all vcpu locks)"
>   
initially the mechanism looped over vcpu's and just aquired the vcpu 
lock and then updated the vcpu.arch infor directly.
Avi mentioned that we have no guarantee if/when the vcpu will come out 
of guest context to free a lock currently held and suggested the 
mechanism x86 uses via setting vcpu->request and kicking the vcpu. Thats 
the eason behind "end up where we (the discussion) started", if we would 
need the vcpu lock again we would be at the beginnign of the discussion.
> So you _need_ a mechanism to kick all vcpus out of guest mode?
>   
I have a mechanism to kick a vcpu, and I use it. Due to the fact that 
smp_call_* don't work as kick for us the kick is an arch specific function.
I hop ethat clarified this part :-)
>>> 2) It uses smp_call_function_many(wait=1), which guarantees that by the
>>> time make_all_requests returns no vcpus will be using stale data (the
>>> remote vcpus will have executed ack_flush).
>>>   
>>>       
>> yes this is really a part my s390 implementation doesn't fulfill yet.  
>> Currently on return vcpus might still use the old memslot information.
>> As mentioned before letting all interrupts come "too far" out of the hot  
>> loop would be a performance issue, therefore I think I will need some  
>> request&confirm mechanism. I'm not sure yet but maybe it could be as  
>> easy as this pseudo code example:
>>
>> # in make_all_requests
>> # remember we have slots_lock write here and the reentry that updates  
>> the vcpu specific data aquires slots_lock for read.
>> loop vcpus
>>  set_bit in vcpu requests
>>  kick vcpu #arch function
>> endloop
>>
>> loop vcpus
>>  until the requested bit is disappeared #as the reentry path uses  
>> test_and_clear it will disappear
>> endloop
>>
>> That would be a implicit synchronization and should work, as I wrote  
>> before setting memslots while the guest is running is rare if ever  
>> existant for s390. On x86 smp_call_many could then work without the wait  
>> flag being set.
>>     
>
> I see, yes. 
>
>   
>> But I assume that this synchronization approach is slower as it  
>> serializes all vcpus on reentry (they wait for the slots_lock to get  
>> dropped), therefore I wanted to ask how often setting memslots on  
>> runtime will occur on x86 ? Would this approach be acceptable ?
>>     
>
> For x86 we need slots_lock for two things:
>
> 1) to protect the memslot structures from changing (very rare), ie:
> kvm_set_memory.
>
> 2) to protect updates to the dirty bitmap (operations on behalf of
> guest) which take slots_lock for read versus updates to that dirty
> bitmap (an ioctl that retrieves what pages have been dirtied in the
> memslots, and clears the dirtyness info).
>
> All you need for S390 is 1), AFAICS.
>   
correct
> For 1), we can drop the slots_lock usage, but instead create an
> explicit synchronization point, where all vcpus are forced to (say
> kvm_vcpu_block) "paused" state. qemu-kvm has such notion.
>
> Same language?
>   
Yes, I think i got your point :-)
But I think by keeping slots_lock we already got our synchronization 
point and don't need an explicit one adding extra code and maybe locks.
As I mentioned above it should synchronize already implicit. When I 
looked at it once more yesterday I realized that kvm_set_memory is not 
performance critical anyway (i.e. does not have to be the fastest ioctl 
on earth) so we could be one step smarter and instead of serializing all 
vcpu's among each other we could set_memory just let do it one by one.

In case I lost you again due to my obviously confusing mainframe 
language this week you might want to my next patch submission where I 
implement that in the s390 arch code as an example. I'll put you on cc 
and in that new code we might find an implicit language synchronization 
for us :-)

[...]

-- 

Grüsse / regards, Christian Ehrhardt
IBM Linux Technology Center, Open Virtualization