[PATCH 1/3] powerpc/kvm: simplify the entering logic for secondary thread

[PATCH 1/3] powerpc/kvm: simplify the entering logic for secondary thread

[Liu Ping Fan]

After the primary vcpu changes vcore_state to VCORE_RUNNING, there is
very little chance to schedule to secondary vcpu. So if we change the
code sequence around set vcore_state to VCORE_RUNNING and disable
preemption, we lost little. But we simplify the entering logi, based on
the fact that if primary vcpu runs, the secondary vcpu can not be scheduled.

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_hv.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 62a2b5a..38b1fc0 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1222,8 +1222,8 @@ static void kvmppc_run_core(struct kvmppc_vcore *vc)
 		kvmppc_create_dtl_entry(vcpu, vc);
 	}
 
-	vc->vcore_state = VCORE_RUNNING;
 	preempt_disable();
+	vc->vcore_state = VCORE_RUNNING;
 	spin_unlock(&vc->lock);
 
 	kvm_guest_enter();
@@ -1351,12 +1351,7 @@ static int kvmppc_run_vcpu(struct kvm_run *kvm_run, struct kvm_vcpu *vcpu)
 	 * this thread straight away and have it join in.
 	 */
 	if (!signal_pending(current)) {
-		if (vc->vcore_state == VCORE_RUNNING &&
-		    VCORE_EXIT_COUNT(vc) == 0) {
-			vcpu->arch.ptid = vc->n_runnable - 1;
-			kvmppc_create_dtl_entry(vcpu, vc);
-			kvmppc_start_thread(vcpu);
-		} else if (vc->vcore_state == VCORE_SLEEPING) {
+		if (vc->vcore_state == VCORE_SLEEPING) {
 			wake_up(&vc->wq);
 		}
 
-- 
1.8.1.4

[PATCH 2/3] powerpc/kvm: fix rare but potential deadlock scene

[Liu Ping Fan]

Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
realmode, so it can trigger the deadlock.

Suppose the following scene:

Two physical cpuM, cpuN, two VM instances A, B, each VM has a group of vcpus.

If on cpuM, vcpu_A_1 holds bitlock X (HPTE_V_HVLOCK), then is switched out,
and on cpuN, vcpu_A_2 try to lock X in realmode, then cpuN will be caught in
realmode for a long time.

What makes things even worse if the following happens,
  On cpuM, bitlockX is hold, on cpuN, Y is hold.
  vcpu_B_2 try to lock Y on cpuM in realmode
  vcpu_A_2 try to lock X on cpuN in realmode

Oops! deadlock happens

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_64_mmu_hv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 043eec8..28160ac 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -474,8 +474,10 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
 	}
 
 	/* Find the HPTE in the hash table */
+	preempt_disable();
 	index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
 					 HPTE_V_VALID | HPTE_V_ABSENT);
+	preempt_enable();
 	if (index < 0)
 		return -ENOENT;
 	hptep = (unsigned long *)(kvm->arch.hpt_virt + (index << 4));
-- 
1.8.1.4

[PATCH 3/3] powerpc/kvm: remove redundant assignment

[Liu Ping Fan]

Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
---
 arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
index 28160ac..7682837 100644
--- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
+++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
@@ -731,7 +731,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
 	lock_rmap(rmap);
 
 	/* Check if we might have been invalidated; let the guest retry if so */
-	ret = RESUME_GUEST;
 	if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) {
 		unlock_rmap(rmap);
 		goto out_unlock;
-- 
1.8.1.4

Re: [PATCH 1/3] powerpc/kvm: simplify the entering logic for secondary thread

[Paul Mackerras]

On Tue, Nov 05, 2013 at 03:42:42PM +0800, Liu Ping Fan wrote:
> After the primary vcpu changes vcore_state to VCORE_RUNNING, there is
> very little chance to schedule to secondary vcpu. So if we change the

Why do you say there is very little chance to run the secondary vcpu?

> code sequence around set vcore_state to VCORE_RUNNING and disable
> preemption, we lost little. But we simplify the entering logi, based on
> the fact that if primary vcpu runs, the secondary vcpu can not be scheduled.

If a vcpu does a hypercall or something else that requires the host
(kernel or userspace) to do something, that can happen in the context
of the vcpu task for that vcpu.  That vcpu task can run on another
core (unless it has been pinned).  When it is finished we would like
the vcpu to continue executing in the guest as soon as possible.  The
code that you remove in this patch enables that to happen without
having to wait until the other threads exit the guest.  So I don't
think it is a good idea to remove this code.

Regards,
Paul.

Re: [PATCH 2/3] powerpc/kvm: fix rare but potential deadlock scene

[Paul Mackerras]

On Tue, Nov 05, 2013 at 03:42:43PM +0800, Liu Ping Fan wrote:
> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
> realmode, so it can trigger the deadlock.

Good catch, we should have preemption disabled while ever we have a
HPTE locked.

> @@ -474,8 +474,10 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
>  	}
>  
>  	/* Find the HPTE in the hash table */
> +	preempt_disable();
>  	index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
>  					 HPTE_V_VALID | HPTE_V_ABSENT);
> +	preempt_enable();

Which means we need to add the preempt_enable after unlocking the
HPTE, not here.

Regards,
Paul.

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Paul Mackerras]

On Tue, Nov 05, 2013 at 03:42:44PM +0800, Liu Ping Fan wrote:
> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 28160ac..7682837 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -731,7 +731,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
>  	lock_rmap(rmap);
>  
>  	/* Check if we might have been invalidated; let the guest retry if so */
> -	ret = RESUME_GUEST;
>  	if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) {
>  		unlock_rmap(rmap);
>  		goto out_unlock;

Acked-by: Paul Mackerras <paulus@samba.org>

Re: [PATCH 2/3] powerpc/kvm: fix rare but potential deadlock scene

[Liu ping fan]

On Wed, Nov 6, 2013 at 1:04 PM, Paul Mackerras <paulus@samba.org> wrote:
> On Tue, Nov 05, 2013 at 03:42:43PM +0800, Liu Ping Fan wrote:
>> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
>> realmode, so it can trigger the deadlock.
>
> Good catch, we should have preemption disabled while ever we have a
> HPTE locked.
>
>> @@ -474,8 +474,10 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
>>       }
>>
>>       /* Find the HPTE in the hash table */
>> +     preempt_disable();
>>       index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
>>                                        HPTE_V_VALID | HPTE_V_ABSENT);
>> +     preempt_enable();
>
> Which means we need to add the preempt_enable after unlocking the
> HPTE, not here.
>
Yes. Sorry, but I am not sure about whether we can call
preempt_disable/enable() in realmode. I think since thread_info is
allocated with linear address, so we can use preempt_disable/enable()
inside kvmppc_hv_find_lock_hpte(), right?

Thanks and regards,
Pingfan

> Regards,
> Paul.

Re: [PATCH 1/3] powerpc/kvm: simplify the entering logic for secondary thread

[Liu ping fan]

On Wed, Nov 6, 2013 at 1:01 PM, Paul Mackerras <paulus@samba.org> wrote:
> On Tue, Nov 05, 2013 at 03:42:42PM +0800, Liu Ping Fan wrote:
>> After the primary vcpu changes vcore_state to VCORE_RUNNING, there is
>> very little chance to schedule to secondary vcpu. So if we change the
>
> Why do you say there is very little chance to run the secondary vcpu?
>
>> code sequence around set vcore_state to VCORE_RUNNING and disable
>> preemption, we lost little. But we simplify the entering logi, based on
>> the fact that if primary vcpu runs, the secondary vcpu can not be scheduled.
>
> If a vcpu does a hypercall or something else that requires the host
> (kernel or userspace) to do something, that can happen in the context
> of the vcpu task for that vcpu.  That vcpu task can run on another
> core (unless it has been pinned).  When it is finished we would like
> the vcpu to continue executing in the guest as soon as possible.  The
> code that you remove in this patch enables that to happen without
> having to wait until the other threads exit the guest.  So I don't
> think it is a good idea to remove this code.
>
Yes, you are right.

Thanks and regards,
Pingfan


> Regards,
> Paul.

Re: [PATCH 2/3] powerpc/kvm: fix rare but potential deadlock scene

[Paul Mackerras]

On Wed, Nov 06, 2013 at 02:02:07PM +0800, Liu ping fan wrote:
> On Wed, Nov 6, 2013 at 1:04 PM, Paul Mackerras <paulus@samba.org> wrote:
> > On Tue, Nov 05, 2013 at 03:42:43PM +0800, Liu Ping Fan wrote:
> >> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
> >> realmode, so it can trigger the deadlock.
> >
> > Good catch, we should have preemption disabled while ever we have a
> > HPTE locked.
> >
> >> @@ -474,8 +474,10 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
> >>       }
> >>
> >>       /* Find the HPTE in the hash table */
> >> +     preempt_disable();
> >>       index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
> >>                                        HPTE_V_VALID | HPTE_V_ABSENT);
> >> +     preempt_enable();
> >
> > Which means we need to add the preempt_enable after unlocking the
> > HPTE, not here.
> >
> Yes. Sorry, but I am not sure about whether we can call
> preempt_disable/enable() in realmode. I think since thread_info is
> allocated with linear address, so we can use preempt_disable/enable()
> inside kvmppc_hv_find_lock_hpte(), right?

Your analysis correctly pointed out that we can get a deadlock if we
can be preempted while holding a lock on a HPTE.  That means that we
have to disable preemption before taking an HPTE lock and keep it
disabled until after we unlock the HPTE.  Since the point of
kvmppc_hv_find_lock_hpte() is to lock the HPTE and return with it
locked, we can't have the preempt_enable() inside it.  The
preempt_enable() has to come after we have unlocked the HPTE.  That is
also why we can't have the preempt_enable() where your patch put it;
it needs to be about 9 lines further down, after the statement
hptep[0] = v.  (We also need to make sure to re-enable preemption in
the index < 0 case.)

Regards,
Paul.

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Alexander Graf]

On 05.11.2013, at 08:42, Liu Ping Fan <kernelfans@gmail.com> wrote:

> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>

Patch description missing.

Please add Paul's ack in the next revision of this patch :).


Alex

> ---
> arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 -
> 1 file changed, 1 deletion(-)
>=20
> diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c =
b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> index 28160ac..7682837 100644
> --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> @@ -731,7 +731,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run =
*run, struct kvm_vcpu *vcpu,
> 	lock_rmap(rmap);
>=20
> 	/* Check if we might have been invalidated; let the guest retry =
if so */
> -	ret =3D RESUME_GUEST;
> 	if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) {
> 		unlock_rmap(rmap);
> 		goto out_unlock;
> --=20
> 1.8.1.4
>=20
> --
> 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

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Benjamin Herrenschmidt]

On Wed, 2013-11-06 at 12:24 +0100, Alexander Graf wrote:
> On 05.11.2013, at 08:42, Liu Ping Fan <kernelfans@gmail.com> wrote:
> 
> > Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> 
> Patch description missing.

Do you really need a description for trivial one-lines whose subject
is a perfectly complete description already ?

> Please add Paul's ack in the next revision of this patch :).
> 
> 
> Alex
> 
> > ---
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 1 -
> > 1 file changed, 1 deletion(-)
> > 
> > diff --git a/arch/powerpc/kvm/book3s_64_mmu_hv.c b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > index 28160ac..7682837 100644
> > --- a/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > +++ b/arch/powerpc/kvm/book3s_64_mmu_hv.c
> > @@ -731,7 +731,6 @@ int kvmppc_book3s_hv_page_fault(struct kvm_run *run, struct kvm_vcpu *vcpu,
> > 	lock_rmap(rmap);
> > 
> > 	/* Check if we might have been invalidated; let the guest retry if so */
> > -	ret = RESUME_GUEST;
> > 	if (mmu_notifier_retry(vcpu->kvm, mmu_seq)) {
> > 		unlock_rmap(rmap);
> > 		goto out_unlock;
> > -- 
> > 1.8.1.4
> > 
> > --
> > 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

Re: [PATCH 2/3] powerpc/kvm: fix rare but potential deadlock scene

[Liu ping fan]

On Wed, Nov 6, 2013 at 7:18 PM, Paul Mackerras <paulus@samba.org> wrote:
> On Wed, Nov 06, 2013 at 02:02:07PM +0800, Liu ping fan wrote:
>> On Wed, Nov 6, 2013 at 1:04 PM, Paul Mackerras <paulus@samba.org> wrote:
>> > On Tue, Nov 05, 2013 at 03:42:43PM +0800, Liu Ping Fan wrote:
>> >> Since kvmppc_hv_find_lock_hpte() is called from both virtmode and
>> >> realmode, so it can trigger the deadlock.
>> >
>> > Good catch, we should have preemption disabled while ever we have a
>> > HPTE locked.
>> >
>> >> @@ -474,8 +474,10 @@ static int kvmppc_mmu_book3s_64_hv_xlate(struct kvm_vcpu *vcpu, gva_t eaddr,
>> >>       }
>> >>
>> >>       /* Find the HPTE in the hash table */
>> >> +     preempt_disable();
>> >>       index = kvmppc_hv_find_lock_hpte(kvm, eaddr, slb_v,
>> >>                                        HPTE_V_VALID | HPTE_V_ABSENT);
>> >> +     preempt_enable();
>> >
>> > Which means we need to add the preempt_enable after unlocking the
>> > HPTE, not here.
>> >
>> Yes. Sorry, but I am not sure about whether we can call
>> preempt_disable/enable() in realmode. I think since thread_info is
>> allocated with linear address, so we can use preempt_disable/enable()
>> inside kvmppc_hv_find_lock_hpte(), right?
>
> Your analysis correctly pointed out that we can get a deadlock if we
> can be preempted while holding a lock on a HPTE.  That means that we
> have to disable preemption before taking an HPTE lock and keep it
> disabled until after we unlock the HPTE.  Since the point of
> kvmppc_hv_find_lock_hpte() is to lock the HPTE and return with it
> locked, we can't have the preempt_enable() inside it.  The
> preempt_enable() has to come after we have unlocked the HPTE.  That is
> also why we can't have the preempt_enable() where your patch put it;
> it needs to be about 9 lines further down, after the statement
> hptep[0] = v.  (We also need to make sure to re-enable preemption in
> the index < 0 case.)
>
Oh, yes, will fix like what you said. My attention is attracted by the
trick of calling kernel func in realmode, and miss the exact point
where the lock is released.

Thanks and regards,
Pingfan

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Alexander Graf]

Am 06.11.2013 um 20:58 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.=
org>:

> On Wed, 2013-11-06 at 12:24 +0100, Alexander Graf wrote:
>> On 05.11.2013, at 08:42, Liu Ping Fan <kernelfans@gmail.com> wrote:
>>=20
>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>=20
>> Patch description missing.
>=20
> Do you really need a description for trivial one-lines whose subject
> is a perfectly complete description already ?

Would I ask for it otherwise? It's also not 100% obvious that the assignment=
 is redundant.


Alex

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Benjamin Herrenschmidt]

On Thu, 2013-11-07 at 08:52 +0100, Alexander Graf wrote:
> Am 06.11.2013 um 20:58 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:
> 
> > On Wed, 2013-11-06 at 12:24 +0100, Alexander Graf wrote:
> >> On 05.11.2013, at 08:42, Liu Ping Fan <kernelfans@gmail.com> wrote:
> >> 
> >>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
> >> 
> >> Patch description missing.
> > 
> > Do you really need a description for trivial one-lines whose subject
> > is a perfectly complete description already ?
> 
> Would I ask for it otherwise? It's also not 100% obvious that the assignment is redundant.

And ? An explanation isn't going to be clearer than the code in that
case ...

Ben.

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Alexander Graf]

Am 07.11.2013 um 08:55 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.=
org>:

> On Thu, 2013-11-07 at 08:52 +0100, Alexander Graf wrote:
>> Am 06.11.2013 um 20:58 schrieb Benjamin Herrenschmidt <benh@kernel.crashi=
ng.org>:
>>=20
>>> On Wed, 2013-11-06 at 12:24 +0100, Alexander Graf wrote:
>>>> On 05.11.2013, at 08:42, Liu Ping Fan <kernelfans@gmail.com> wrote:
>>>>=20
>>>>> Signed-off-by: Liu Ping Fan <pingfank@linux.vnet.ibm.com>
>>>>=20
>>>> Patch description missing.
>>>=20
>>> Do you really need a description for trivial one-lines whose subject
>>> is a perfectly complete description already ?
>>=20
>> Would I ask for it otherwise? It's also not 100% obvious that the assignm=
ent is redundant.
>=20
> And ? An explanation isn't going to be clearer than the code in that
> case ...

It's pretty non-obvious when you do a git show on that patch in 1 year from n=
ow, as the redundancy is out of scope of what the diff shows.


Alex

Re: [PATCH 3/3] powerpc/kvm: remove redundant assignment

[Benjamin Herrenschmidt]

On Thu, 2013-11-07 at 09:14 +0100, Alexander Graf wrote:
> > And ? An explanation isn't going to be clearer than the code in that
> > case ...
> 
> It's pretty non-obvious when you do a git show on that patch in 1 year
> from now, as the redundancy is out of scope of what the diff shows.

And ? How would an explanation help ?

Either it's redundant or it's not ... but only look at the code can
prove it. An explanation won't because if the patch is wrong, so will be
the explanation.

Cheers,
Ben.