Re: [PATCH 3/3] kvm-s390: streamline memslot handling - rebased

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

Marcelo Tosatti wrote:
> On Tue, Jun 02, 2009 at 04:26:11PM +0200, ehrhardt@linux.vnet.ibm.com wrote:
>   
>> From: Christian Ehrhardt <ehrhardt@linux.vnet.ibm.com>
>>     
[...]
>> @@ -706,13 +713,19 @@ int kvm_arch_set_memory_region(struct kv
>>  
>>  	/* request update of sie control block for all available vcpus */
>>  	for (i = 0; i < KVM_MAX_VCPUS; ++i) {
>> -		if (kvm->vcpus[i]) {
>> -			if (test_and_set_bit(KVM_REQ_MMU_RELOAD,
>> -						&kvm->vcpus[i]->requests))
>> -				continue;
>> -			kvm_s390_inject_sigp_stop(kvm->vcpus[i],
>> -						  ACTION_VCPUREQUEST_ON_STOP);
>> -		}
>> +		vcpu = kvm->vcpus[i];
>> +		if (!vcpu)
>> +			continue;
>> +
>> +		if (!test_and_set_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests))
>> +			continue;
>> +
>> +		if (vcpu->cpu == -1)
>> +			continue;
>>     
>
> What happens if the check for cpu == -1 races with kvm_arch_vcpu_put?
> This context will wait until the vcpu_put context is scheduled back in
> to clear the bit? Is that OK?
>   
It either comes back to clear the bit or it is consumed on deletion of 
the vcpu. Both ways are ok. The question we have to answer is if it 
might stall the mem update ioctl for too long.
Because eventually the check for vcpu->cpu == -1 is just an optimization 
if we would completely ignore remove it we would have the same problem 
-> could it stall the set mem operation too much. That means the "race" 
is not an issue it might just be sub-optimal, but the chance for a long 
stall could become an issue. Unfortunately I have no better approach to 
that (yet), until then this I like this implementation more than what we 
would have without all the corner case fixes in that patch series.

>> +
>> +		kvm_s390_inject_sigp_stop(vcpu, ACTION_VCPUREQUEST_ON_STOP);
>> +		wait_on_bit(&vcpu->requests, KVM_REQ_MMU_RELOAD,
>> +			    wait_bit_schedule, TASK_UNINTERRUPTIBLE);
>>  	}
>>     
>
>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  {
> +       vcpu->cpu = -1;
>         save_fp_regs(&vcpu->arch.guest_fpregs);
>   
[...]
>> +++ kvm/arch/s390/kvm/kvm-s390.h
>> @@ -92,6 +92,13 @@ static inline unsigned long kvm_s390_han
>>  	if (!vcpu->requests)
>>  		return 0;
>>  
>> +	/* requests that can be handled at all levels */
>> +	if (test_and_clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests)) {
>> +		smp_mb__after_clear_bit();
>>     
>
> Really need that smp_mb__after_clear_bit ? AFAIK test_and_clear_bit
> implies a barrier?
>   

Well I agree that practically test_and_clear_bit has a barrier on s390, 
but as far as I read Documentation/atomic_ops.txt at line 339-360 I 
think the interface does not imply it so I wanted to add it explicitly. 
I would be happy if someone really knows the in depth details here and 
corrects me :-)

>> +		wake_up_bit(&vcpu->requests, KVM_REQ_MMU_RELOAD);
>> +		kvm_s390_vcpu_set_mem(vcpu);
>> +	}
>> +
>>  	return vcpu->requests;
>>  }
>>  
>> Index: kvm/virt/kvm/kvm_main.c
>> ===================================================================
>> --- kvm.orig/virt/kvm/kvm_main.c
>> +++ kvm/virt/kvm/kvm_main.c
>> @@ -1682,6 +1682,10 @@ static int kvm_vcpu_release(struct inode
>>  {
>>  	struct kvm_vcpu *vcpu = filp->private_data;
>>  
>> +	clear_bit(KVM_REQ_MMU_RELOAD, &vcpu->requests);
>> +	smp_mb__after_clear_bit();
>> +	wake_up_bit(&vcpu->requests, KVM_REQ_MMU_RELOAD);
>> +
>>     
>
> And this should be generic? Say if other architectures want to make use 
> of a similar wait infrastructure. Talk is cheap.
>   
Clear bit and wake up on release doesn't hurt any architecture, but it 
is at a good place fine for those using the mechanism to ensure cleaning 
up outstanding things when closing a vcpu fd.
I thought its not worth to add kvm_ARCH_vcpu_release for it while I 
could do so if we want it separated.
(continued below)
> Anyway, yeah, the set request / wait mechanism you implement here is
> quite similar to the idea mentioned earlier that could be used for x86.
>
> Just get rid of this explicit KVM_REQ_MMU_RELOAD knowledge in
> arch-independent code please (if you want to see this merged).
>   
I agree to lift the wait part to other archs later if needed, but as 
mentioned above I could move this to arch code to the cost of one arch 
hook more. But as also mentioned it doesn't really hurt. I agree that it 
does not need to be KVM_REQ_MMU_RELOAD specific, we could just 
walk/clear/wake all bits on that vcpu->requests variable.
Would that be generic enough in your opinion ?
> Later it can all be lifted off to arch independent code.
>   
True for the wait part which can evolve in our arch code until it is 
ripe to get cross arch merged.

-- 

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