Patchwork kvm/powerpc: Handle errors in secondary thread grabbing

login
register
mail settings
Submitter Michael Ellerman
Date Oct. 16, 2012, 12:15 a.m.
Message ID <1350346550-32539-1-git-send-email-michael@ellerman.id.au>
Download mbox | patch
Permalink /patch/191684/
State New
Headers show

Comments

Michael Ellerman - Oct. 16, 2012, 12:15 a.m.
In the Book3s HV code, kvmppc_run_core() has logic to grab the secondary
threads of the physical core.

If for some reason a thread is stuck, kvmppc_grab_hwthread() can fail,
but currently we ignore the failure and continue into the guest. If the
stuck thread is in the kernel badness ensues.

Instead we should check for failure and bail out.

I've moved the grabbing prior to the startup of runnable threads, to simplify
the error case. AFAICS this is harmless, but I could be missing something
subtle.

Signed-off-by: Michael Ellerman <michael@ellerman.id.au>
---

Or we could just BUG_ON() ?
---
 arch/powerpc/kvm/book3s_hv.c |   22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)
Paul Mackerras - Oct. 16, 2012, 3:13 a.m.
Michael,

On Tue, Oct 16, 2012 at 11:15:50AM +1100, Michael Ellerman wrote:
> In the Book3s HV code, kvmppc_run_core() has logic to grab the secondary
> threads of the physical core.
> 
> If for some reason a thread is stuck, kvmppc_grab_hwthread() can fail,
> but currently we ignore the failure and continue into the guest. If the
> stuck thread is in the kernel badness ensues.
> 
> Instead we should check for failure and bail out.
> 
> I've moved the grabbing prior to the startup of runnable threads, to simplify
> the error case. AFAICS this is harmless, but I could be missing something
> subtle.

Thanks for looking at this - but in fact this is fixed by my patch
entitled "KVM: PPC: Book3S HV: Fix some races in starting secondary
threads" submitted back on August 28.

Regards,
Paul.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michael Ellerman - Oct. 16, 2012, 6 a.m.
On Tue, 2012-10-16 at 14:13 +1100, Paul Mackerras wrote:
> Michael,
> 
> On Tue, Oct 16, 2012 at 11:15:50AM +1100, Michael Ellerman wrote:
> > In the Book3s HV code, kvmppc_run_core() has logic to grab the secondary
> > threads of the physical core.
> > 
> > If for some reason a thread is stuck, kvmppc_grab_hwthread() can fail,
> > but currently we ignore the failure and continue into the guest. If the
> > stuck thread is in the kernel badness ensues.
> > 
> > Instead we should check for failure and bail out.
> > 
> > I've moved the grabbing prior to the startup of runnable threads, to simplify
> > the error case. AFAICS this is harmless, but I could be missing something
> > subtle.
> 
> Thanks for looking at this - but in fact this is fixed by my patch
> entitled "KVM: PPC: Book3S HV: Fix some races in starting secondary
> threads" submitted back on August 28.

OK thanks. It seems that patch didn't make 3.7 ?

I don't see it in kvm-ppc-next either.

cheers

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Benjamin Herrenschmidt - Oct. 16, 2012, 7:33 p.m.
On Tue, 2012-10-16 at 17:00 +1100, Michael Ellerman wrote:
> > Thanks for looking at this - but in fact this is fixed by my patch
> > entitled "KVM: PPC: Book3S HV: Fix some races in starting secondary
> > threads" submitted back on August 28.
> 
> OK thanks. It seems that patch didn't make 3.7 ?
> 
> I don't see it in kvm-ppc-next either.

Alex, WTF ?

Ben.


--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alexander Graf - Oct. 17, 2012, 6:25 a.m.
On 16.10.2012, at 21:33, Benjamin Herrenschmidt wrote:

> On Tue, 2012-10-16 at 17:00 +1100, Michael Ellerman wrote:
>>> Thanks for looking at this - but in fact this is fixed by my patch
>>> entitled "KVM: PPC: Book3S HV: Fix some races in starting secondary
>>> threads" submitted back on August 28.
>> 
>> OK thanks. It seems that patch didn't make 3.7 ?
>> 
>> I don't see it in kvm-ppc-next either.
> 
> Alex, WTF ?

Hrm. Not sure what happened there. I think I waited for your ack, but never actually applied things when it came. My bad :)


Alex

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 721d460..55925cd 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -884,16 +884,30 @@  static int kvmppc_run_core(struct kvmppc_vcore *vc)
 		if (vcpu->arch.ceded)
 			vcpu->arch.ptid = ptid++;
 
+	/*
+	 * Grab any remaining hw threads so they can't go into the kernel.
+	 * Do this early to simplify the cleanup path if it fails.
+	 */
+	for (i = ptid; i < threads_per_core; ++i) {
+		int j, rc = kvmppc_grab_hwthread(vc->pcpu + i);
+		if (rc) {
+			for (j = i - 1; j ; j--)
+				kvmppc_release_hwthread(vc->pcpu + j);
+
+			list_for_each_entry(vcpu, &vc->runnable_threads,
+					    arch.run_list)
+				vcpu->arch.ret = -EBUSY;
+
+			goto out;
+		}
+	}
+
 	vc->stolen_tb += mftb() - vc->preempt_tb;
 	vc->pcpu = smp_processor_id();
 	list_for_each_entry(vcpu, &vc->runnable_threads, arch.run_list) {
 		kvmppc_start_thread(vcpu);
 		kvmppc_create_dtl_entry(vcpu, vc);
 	}
-	/* Grab any remaining hw threads so they can't go into the kernel */
-	for (i = ptid; i < threads_per_core; ++i)
-		kvmppc_grab_hwthread(vc->pcpu + i);
-
 	preempt_disable();
 	spin_unlock(&vc->lock);