Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait

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

Jerone Young wrote:
> # HG changeset patch
> # User Jerone Young <jyoung5@us.ibm.com>
> # Date 1206968186 18000
> # Node ID 529f5a653d1e86b81270eeb7598dafcdba9abc1d
> # Parent  15675e59e019c4800a834ae79090d92b07f1d0ce
> Add PowerPC KVM guest wait handling support
> 
> This patch handles a guest that is in a wait state. This ensures that the guest is not allways eating up 100% cpu when it is idle.
> 
> Signed-off-by: Jerone Young <jyoung5@us.ibm.com>
> 
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -214,6 +214,10 @@ static void kvmppc_deliver_interrupt(str
>  		vcpu->arch.tsr |= TSR_DIS;
>  		break;
>  	}
> +
> +	/* clear guest wait state */
> +	if (vcpu->arch.msr & MSR_WE)
> +		vcpu->arch.msr &= ~MSR_WE;
> 
>  	vcpu->arch.srr0 = vcpu->arch.pc;
>  	vcpu->arch.srr1 = vcpu->arch.msr;
> @@ -452,6 +456,20 @@ int kvmppc_handle_exit(struct kvm_run *r
>  	local_irq_disable();
> 
>  	kvmppc_check_and_deliver_interrupts(vcpu);
> +
> +	/* handle guest vcpu that is in wait state */
> +	if (vcpu->arch.msr & MSR_WE) {
##> +		DECLARE_WAITQUEUE(wait, current);
##> +		add_wait_queue(&vcpu->wq, &wait);
> +
> +		while (!signal_pending(current)) {
> +			set_current_state(TASK_INTERRUPTIBLE);
> +			schedule();
> +		}
> +
> +		set_current_state(TASK_RUNNING);
##> +		remove_wait_queue(&vcpu->wq, &wait);

I might missing something but I tihnk this implementation is not yet complete.
The wait queue is useless until you have a caller for it. If that patch would be all you could just remove the lines I marked with "##".

We might need the waitqueue if we implement an explicit think that calls wake_up() e.g. from interrupt handling. In that case we will addtitionally need some locks to prevent a race between entering the sleep and these delivery/wake_up, but let's start without that waitqueue for now and remove it until we see the need for it - I have enough comments without racy wakeups ;-)

Atm only signal_pending is checked to exit that sleep loop, that might sleep forever. I know we have that sigalarm handler qemu/kvm sets up in userspace but by design the MSR[WE] sleep should not sleep until that signal right?
At the moment we have here in code:
 - exit MSRWE] sleep on a timer base (userspace SIG_ALRM)
In the specs I found the definition:
 - "... sleeps until an interrupt is taken, a reset occurs or an external debug tool clears WE"
 
let me describe some scenarios while the userspace alarm timer would not exist (we ignore it because that is not the defined exit of the MSR[WE] pause):
1. MSR[WE] is set to 1 by the guest and we enter this sleep loop here
2. an interrupt e.g. DEC occurs sets the pending exception via kvmppc_queue_exception and exits
-> the dec or any other interrupt occurs, but this is not causing a signal_pending we still sleep and never wake up
=> I think we need to check more than signal_pending here in this case vcpu->arch.pending_exceptions

1. MSR[WE] is set to 1 by the guest and we enter this sleep loop here
2. some day we might provide an interface that works like "external debug tools" and so we change MSR[WE] in memory
-> no signal occurs we still hang in that loop
=> so I guess we should check the condition of MSR[WE] again in the loop

When we are going to implement checks to pending_exceptions and MSR[WE] we will need to check kvmppc_check_and_deliver_interrupts(vcpu) on exit of that sleep e.g. where you set the task running atm.

So what I suggest (this will still need discussions and e.g. extension with waitqueue, a wake_up caller and locks if we need to wake it from somewhere else) would be something like:

+	/* handle guest vcpu that is in wait state */
+	if (vcpu->arch.msr & MSR_WE) {
+
+		while (!signal_pending(current) 
+			&& !pending_exceptions
+			&& (vcpu->arch.msr & MSR_WE))
+		{
+			set_current_state(TASK_INTERRUPTIBLE);
+			schedule();
+		}
+
+		set_current_state(TASK_RUNNING);
+		kvmppc_check_and_deliver_interrupts(vcpu);

 
> +	}
> 
>  	/* Do some exit accounting. */
>  	vcpu->stat.exits++;
> 
> -------------------------------------------------------------------------
> Check out the new SourceForge.net Marketplace.
> It's the best place to buy or sell services for
> just about anything Open Source.
> http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
> _______________________________________________
> kvm-ppc-devel mailing list
> kvm-ppc-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel


-- 

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

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
kvm-ppc-devel mailing list
kvm-ppc-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel