From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Ehrhardt Date: Mon, 31 Mar 2008 13:33:40 +0000 Subject: Re: [kvm-ppc-devel] [PATCH 2 of 2] Add PowerPC KVM guest wait Message-Id: <47F0E834.3030504@linux.vnet.ibm.com> List-Id: References: <21179d0ab8a62ecc24d1.1206740047@thinkpadL> In-Reply-To: <21179d0ab8a62ecc24d1.1206740047@thinkpadL> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: kvm-ppc@vger.kernel.org Jerone Young wrote: > # HG changeset patch > # User Jerone Young > # Date 1206968186 18000 > # Node ID 529f5a653d1e86b81270eeb7598dafcdba9abc1d > # Parent 15675e59e019c4800a834ae79090d92b07f1d0ce > Add PowerPC KVM guest wait handling support >=20 > 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. >=20 > Signed-off-by: Jerone Young >=20 > 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 |=3D TSR_DIS; > break; > } > + > + /* clear guest wait state */ > + if (vcpu->arch.msr & MSR_WE) > + vcpu->arch.msr &=3D ~MSR_WE; >=20 > vcpu->arch.srr0 =3D vcpu->arch.pc; > vcpu->arch.srr1 =3D vcpu->arch.msr; > @@ -452,6 +456,20 @@ int kvmppc_handle_exit(struct kvm_run *r > local_irq_disable(); >=20 > 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 comple= te. The wait queue is useless until you have a caller for it. If that patch wou= ld be all you could just remove the lines I marked with "##". We might need the waitqueue if we implement an explicit think that calls wa= ke_up() e.g. from interrupt handling. In that case we will addtitionally ne= ed some locks to prevent a race between entering the sleep and these delive= ry/wake_up, but let's start without that waitqueue for now and remove it un= til 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 slee= p forever. I know we have that sigalarm handler qemu/kvm sets up in userspa= ce 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 d= ebug tool clears WE" =20 let me describe some scenarios while the userspace alarm timer would not ex= ist (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 =3D> I think we need to check more than signal_pending here in this case vc= pu->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 t= ools" and so we change MSR[WE] in memory -> no signal occurs we still hang in that loop =3D> 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 tha= t 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)=20 + && !pending_exceptions + && (vcpu->arch.msr & MSR_WE)) + { + set_current_state(TASK_INTERRUPTIBLE); + schedule(); + } + + set_current_state(TASK_RUNNING); + kvmppc_check_and_deliver_interrupts(vcpu); =20 > + } >=20 > /* Do some exit accounting. */ > vcpu->stat.exits++; >=20 > ------------------------------------------------------------------------- > 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/marketpl= ace > _______________________________________________ > kvm-ppc-devel mailing list > kvm-ppc-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/kvm-ppc-devel --=20 Gr=FCsse / regards,=20 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