Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660799 > unrolled thread

Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts

Started byPeter Xu <peterx@redhat.com>
First post2017-06-08 09:00 +0200
Last post2017-06-09 09:50 +0200
Articles 7 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Peter Xu <peterx@redhat.com> - 2017-06-08 09:00 +0200
    Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Paolo Bonzini <pbonzini@redhat.com> - 2017-06-08 09:10 +0200
      Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Peter Xu <peterx@redhat.com> - 2017-06-08 11:20 +0200
        Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Paolo Bonzini <pbonzini@redhat.com> - 2017-06-08 13:30 +0200
          Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Peter Xu <peterx@redhat.com> - 2017-06-09 05:00 +0200
            Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Paolo Bonzini <pbonzini@redhat.com> - 2017-06-09 09:40 +0200
              Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted  interrupts Peter Xu <peterx@redhat.com> - 2017-06-09 09:50 +0200

#1660799 — Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts

FromPeter Xu <peterx@redhat.com>
Date2017-06-08 09:00 +0200
SubjectRe: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts
Message-ID<tPZAe-Nh-7@gated-at.bofh.it>
On Tue, Jun 06, 2017 at 12:57:05PM +0200, Paolo Bonzini wrote:
> In some cases, for example involving hot-unplug of assigned
> devices, pi_post_block can forget to remove the vCPU from the
> blocked_vcpu_list.  When this happens, the next call to
> pi_pre_block corrupts the list.
> 
> Fix this in two ways.  First, check vcpu->pre_pcpu in pi_pre_block
> and WARN instead of adding the element twice in the list.  Second,
> always do the list removal in pi_post_block if vcpu->pre_pcpu is
> set (not -1).
> 
> The new code keeps interrupts disabled for the whole duration of
> pi_pre_block/pi_post_block.  This is not strictly necessary, but
> easier to follow.  For the same reason, PI.ON is checked only
> after the cmpxchg, and to handle it we just call the post-block
> code.  This removes duplication of the list removal code.
> 
> Cc: Longpeng (Mike) <longpeng2@huawei.com>
> Cc: Huangweidong <weidong.huang@huawei.com>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Cc: wangxin <wangxinxin.wang@huawei.com>
> Cc: Radim Krčmář <rkrcmar@redhat.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/vmx.c | 62 ++++++++++++++++++++++--------------------------------
>  1 file changed, 25 insertions(+), 37 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 747d16525b45..0f4714fe4908 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11236,10 +11236,11 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>  	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
>  	struct pi_desc old, new;
>  	unsigned int dest;
> -	unsigned long flags;
>  
>  	do {
>  		old.control = new.control = pi_desc->control;
> +		WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR,
> +		     "Wakeup handler not enabled while the VCPU is blocked\n");
>  
>  		dest = cpu_physical_id(vcpu->cpu);
>  
> @@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>  	} while (cmpxchg(&pi_desc->control, old.control,
>  			new.control) != old.control);
>  
> -	if(vcpu->pre_pcpu != -1) {
> -		spin_lock_irqsave(
> -			&per_cpu(blocked_vcpu_on_cpu_lock,
> -			vcpu->pre_pcpu), flags);
> +	if (!WARN_ON_ONCE(vcpu->pre_pcpu == -1)) {
> +		spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
>  		list_del(&vcpu->blocked_vcpu_list);
> -		spin_unlock_irqrestore(
> -			&per_cpu(blocked_vcpu_on_cpu_lock,
> -			vcpu->pre_pcpu), flags);
> +		spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
>  		vcpu->pre_pcpu = -1;
>  	}
>  }
> @@ -11283,7 +11280,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
>   */
>  static int pi_pre_block(struct kvm_vcpu *vcpu)
>  {
> -	unsigned long flags;
>  	unsigned int dest;
>  	struct pi_desc old, new;
>  	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> @@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>  		!kvm_vcpu_apicv_active(vcpu))
>  		return 0;
>  
> -	vcpu->pre_pcpu = vcpu->cpu;
> -	spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
> -			  vcpu->pre_pcpu), flags);
> -	list_add_tail(&vcpu->blocked_vcpu_list,
> -		      &per_cpu(blocked_vcpu_on_cpu,
> -		      vcpu->pre_pcpu));
> -	spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock,
> -			       vcpu->pre_pcpu), flags);
> +	WARN_ON(irqs_disabled());
> +	local_irq_disable();
> +	if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
> +		vcpu->pre_pcpu = vcpu->cpu;
> +		spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> +		list_add_tail(&vcpu->blocked_vcpu_list,
> +			      &per_cpu(blocked_vcpu_on_cpu,
> +				       vcpu->pre_pcpu));
> +		spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> +	}
>  
>  	do {
>  		old.control = new.control = pi_desc->control;
>  
> -		/*
> -		 * We should not block the vCPU if
> -		 * an interrupt is posted for it.
> -		 */
> -		if (pi_test_on(pi_desc) == 1) {
> -			spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
> -					  vcpu->pre_pcpu), flags);
> -			list_del(&vcpu->blocked_vcpu_list);
> -			spin_unlock_irqrestore(
> -					&per_cpu(blocked_vcpu_on_cpu_lock,
> -					vcpu->pre_pcpu), flags);
> -			vcpu->pre_pcpu = -1;
> -
> -			return 1;

[1]

> -		}
> -
>  		WARN((pi_desc->sn == 1),
>  		     "Warning: SN field of posted-interrupts "
>  		     "is set before blocking\n");
> @@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
>  	} while (cmpxchg(&pi_desc->control, old.control,
>  			new.control) != old.control);
>  
> -	return 0;
> +	/* We should not block the vCPU if an interrupt is posted for it.  */
> +	if (pi_test_on(pi_desc) == 1)
> +		__pi_post_block(vcpu);

A question on when pi_test_on() is set:

The old code will return 1 if detected (ses [1]), while the new code
does not. Would that matter? (IIUC that decides whether the vcpu will
continue to run?)

> +
> +	local_irq_enable();
> +	return (vcpu->pre_pcpu == -1);

Above we have:

	if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
		vcpu->pre_pcpu = vcpu->cpu;
                ...
	}

Then can here vcpu->pre_pcpu really be -1?

>  }
>  
>  static int vmx_pre_block(struct kvm_vcpu *vcpu)
> @@ -11361,12 +11348,13 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
>  
>  static void pi_post_block(struct kvm_vcpu *vcpu)
>  {
> -	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
> -		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
> -		!kvm_vcpu_apicv_active(vcpu))
> +	if (vcpu->pre_pcpu == -1)
>  		return;
>  
> +	WARN_ON(irqs_disabled());
> +	local_irq_disable();
>  	__pi_post_block(vcpu);
> +	local_irq_enable();
>  }
>  
>  static void vmx_post_block(struct kvm_vcpu *vcpu)
> -- 
> 2.13.0
> 
> 

A general question to pre_block/post_block handling for PI:

I see that we are handling PI logic mostly in four places:

vmx_vcpu_pi_{load|put}
pi_{pre_post}_block

But do we really need the pre_block/post_block handling? Here's how I
understand when vcpu blocked:

- vcpu_block
  - ->pre_block
  - kvm_vcpu_block [1]
    - schedule()
      - kvm_sched_out
        - vmx_vcpu_pi_put [3]
      - (another process working) ...
      - kvm_sched_in
        - vmx_vcpu_pi_load [4]
  - ->post_block [2]

If so, [1] & [2] will definitely be paired with [3] & [4], then why we
need [3] & [4] at all?

(Though [3] & [4] will also be used when preemption happens, so they
 are required)

Please kindly figure out if I missed anything important...

Thanks,

-- 
Peter Xu

[toc] | [next] | [standalone]

#1660809

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-06-08 09:10 +0200
Message-ID<tPZJU-16j-15@gated-at.bofh.it>
In reply to#1660799

----- Original Message -----
> From: "Peter Xu" <peterx@redhat.com>
> To: "Paolo Bonzini" <pbonzini@redhat.com>
> Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Longpeng" <longpeng2@huawei.com>, "Huangweidong"
> <weidong.huang@huawei.com>, "Gonglei" <arei.gonglei@huawei.com>, "wangxin" <wangxinxin.wang@huawei.com>, "Radim
> Krčmář" <rkrcmar@redhat.com>
> Sent: Thursday, June 8, 2017 8:50:57 AM
> Subject: Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts
> 
> On Tue, Jun 06, 2017 at 12:57:05PM +0200, Paolo Bonzini wrote:
> > In some cases, for example involving hot-unplug of assigned
> > devices, pi_post_block can forget to remove the vCPU from the
> > blocked_vcpu_list.  When this happens, the next call to
> > pi_pre_block corrupts the list.
> > 
> > Fix this in two ways.  First, check vcpu->pre_pcpu in pi_pre_block
> > and WARN instead of adding the element twice in the list.  Second,
> > always do the list removal in pi_post_block if vcpu->pre_pcpu is
> > set (not -1).
> > 
> > The new code keeps interrupts disabled for the whole duration of
> > pi_pre_block/pi_post_block.  This is not strictly necessary, but
> > easier to follow.  For the same reason, PI.ON is checked only
> > after the cmpxchg, and to handle it we just call the post-block
> > code.  This removes duplication of the list removal code.
> > 
> > Cc: Longpeng (Mike) <longpeng2@huawei.com>
> > Cc: Huangweidong <weidong.huang@huawei.com>
> > Cc: Gonglei <arei.gonglei@huawei.com>
> > Cc: wangxin <wangxinxin.wang@huawei.com>
> > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  arch/x86/kvm/vmx.c | 62
> >  ++++++++++++++++++++++--------------------------------
> >  1 file changed, 25 insertions(+), 37 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > index 747d16525b45..0f4714fe4908 100644
> > --- a/arch/x86/kvm/vmx.c
> > +++ b/arch/x86/kvm/vmx.c
> > @@ -11236,10 +11236,11 @@ static void __pi_post_block(struct kvm_vcpu
> > *vcpu)
> >  	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> >  	struct pi_desc old, new;
> >  	unsigned int dest;
> > -	unsigned long flags;
> >  
> >  	do {
> >  		old.control = new.control = pi_desc->control;
> > +		WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR,
> > +		     "Wakeup handler not enabled while the VCPU is blocked\n");
> >  
> >  		dest = cpu_physical_id(vcpu->cpu);
> >  
> > @@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu
> > *vcpu)
> >  	} while (cmpxchg(&pi_desc->control, old.control,
> >  			new.control) != old.control);
> >  
> > -	if(vcpu->pre_pcpu != -1) {
> > -		spin_lock_irqsave(
> > -			&per_cpu(blocked_vcpu_on_cpu_lock,
> > -			vcpu->pre_pcpu), flags);
> > +	if (!WARN_ON_ONCE(vcpu->pre_pcpu == -1)) {
> > +		spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> >  		list_del(&vcpu->blocked_vcpu_list);
> > -		spin_unlock_irqrestore(
> > -			&per_cpu(blocked_vcpu_on_cpu_lock,
> > -			vcpu->pre_pcpu), flags);
> > +		spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> >  		vcpu->pre_pcpu = -1;
> >  	}
> >  }
> > @@ -11283,7 +11280,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
> >   */
> >  static int pi_pre_block(struct kvm_vcpu *vcpu)
> >  {
> > -	unsigned long flags;
> >  	unsigned int dest;
> >  	struct pi_desc old, new;
> >  	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > @@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
> >  		!kvm_vcpu_apicv_active(vcpu))
> >  		return 0;
> >  
> > -	vcpu->pre_pcpu = vcpu->cpu;
> > -	spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
> > -			  vcpu->pre_pcpu), flags);
> > -	list_add_tail(&vcpu->blocked_vcpu_list,
> > -		      &per_cpu(blocked_vcpu_on_cpu,
> > -		      vcpu->pre_pcpu));
> > -	spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock,
> > -			       vcpu->pre_pcpu), flags);
> > +	WARN_ON(irqs_disabled());
> > +	local_irq_disable();
> > +	if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
> > +		vcpu->pre_pcpu = vcpu->cpu;
> > +		spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> > +		list_add_tail(&vcpu->blocked_vcpu_list,
> > +			      &per_cpu(blocked_vcpu_on_cpu,
> > +				       vcpu->pre_pcpu));
> > +		spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> > +	}
> >  
> >  	do {
> >  		old.control = new.control = pi_desc->control;
> >  
> > -		/*
> > -		 * We should not block the vCPU if
> > -		 * an interrupt is posted for it.
> > -		 */
> > -		if (pi_test_on(pi_desc) == 1) {
> > -			spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
> > -					  vcpu->pre_pcpu), flags);
> > -			list_del(&vcpu->blocked_vcpu_list);
> > -			spin_unlock_irqrestore(
> > -					&per_cpu(blocked_vcpu_on_cpu_lock,
> > -					vcpu->pre_pcpu), flags);
> > -			vcpu->pre_pcpu = -1;
> > -
> > -			return 1;
> 
> [1]
> 
> > -		}
> > -
> >  		WARN((pi_desc->sn == 1),
> >  		     "Warning: SN field of posted-interrupts "
> >  		     "is set before blocking\n");
> > @@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
> >  	} while (cmpxchg(&pi_desc->control, old.control,
> >  			new.control) != old.control);
> >  
> > -	return 0;
> > +	/* We should not block the vCPU if an interrupt is posted for it.  */
> > +	if (pi_test_on(pi_desc) == 1)
> > +		__pi_post_block(vcpu);
> 
> A question on when pi_test_on() is set:
> 
> The old code will return 1 if detected (ses [1]), while the new code
> does not. Would that matter? (IIUC that decides whether the vcpu will
> continue to run?)

The new code does, because __pi_post_block resets vcpu->pre_pcpu to -1.

> > +
> > +	local_irq_enable();
> > +	return (vcpu->pre_pcpu == -1);
> 
> Above we have:
> 
> 	if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
> 		vcpu->pre_pcpu = vcpu->cpu;
>                 ...
> 	}
> 
> Then can here vcpu->pre_pcpu really be -1?

See above. :)

> >  }
> >  
> >  static int vmx_pre_block(struct kvm_vcpu *vcpu)
> > @@ -11361,12 +11348,13 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
> >  
> >  static void pi_post_block(struct kvm_vcpu *vcpu)
> >  {
> > -	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
> > -		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
> > -		!kvm_vcpu_apicv_active(vcpu))
> > +	if (vcpu->pre_pcpu == -1)
> >  		return;
> >  
> > +	WARN_ON(irqs_disabled());
> > +	local_irq_disable();
> >  	__pi_post_block(vcpu);
> > +	local_irq_enable();
> >  }
> >  
> >  static void vmx_post_block(struct kvm_vcpu *vcpu)
> > --
> > 2.13.0
> > 
> > 
> 
> A general question to pre_block/post_block handling for PI:
> 
> I see that we are handling PI logic mostly in four places:
> 
> vmx_vcpu_pi_{load|put}
> pi_{pre_post}_block
> 
> But do we really need the pre_block/post_block handling? Here's how I
> understand when vcpu blocked:
> 
> - vcpu_block
>   - ->pre_block
>   - kvm_vcpu_block [1]
>     - schedule()
>       - kvm_sched_out
>         - vmx_vcpu_pi_put [3]
>       - (another process working) ...
>       - kvm_sched_in
>         - vmx_vcpu_pi_load [4]
>   - ->post_block [2]
> 
> If so, [1] & [2] will definitely be paired with [3] & [4], then why we
> need [3] & [4] at all?
> 
> (Though [3] & [4] will also be used when preemption happens, so they
>  are required)
> 
> Please kindly figure out if I missed anything important...

Oh, I see what you mean: set up the wakeup handler in vmx_vcpu_pi_put
and rely on PI.ON to wake up the sleeping process immediately.  That
should be feasible, but overall I like the current pre_block/post_block
structure, and I think it's simpler.  The only thing to be careful about
is leaving the IRTE unmodified when scheduling out a blocked VCPU, which
is cleaned up and simplified in patch 3.

So I understand that the state may seem a bit too complicated as
of this patch, but hopefully the next two make it clearer.

Paolo

[toc] | [prev] | [next] | [standalone]

#1660967

FromPeter Xu <peterx@redhat.com>
Date2017-06-08 11:20 +0200
Message-ID<tQ1LI-2jR-7@gated-at.bofh.it>
In reply to#1660809
On Thu, Jun 08, 2017 at 03:00:39AM -0400, Paolo Bonzini wrote:
> 
> 
> ----- Original Message -----
> > From: "Peter Xu" <peterx@redhat.com>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>
> > Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, "Longpeng" <longpeng2@huawei.com>, "Huangweidong"
> > <weidong.huang@huawei.com>, "Gonglei" <arei.gonglei@huawei.com>, "wangxin" <wangxinxin.wang@huawei.com>, "Radim
> > Krčmář" <rkrcmar@redhat.com>
> > Sent: Thursday, June 8, 2017 8:50:57 AM
> > Subject: Re: [PATCH 2/4] KVM: VMX: avoid double list add with VT-d posted interrupts
> > 
> > On Tue, Jun 06, 2017 at 12:57:05PM +0200, Paolo Bonzini wrote:
> > > In some cases, for example involving hot-unplug of assigned
> > > devices, pi_post_block can forget to remove the vCPU from the
> > > blocked_vcpu_list.  When this happens, the next call to
> > > pi_pre_block corrupts the list.
> > > 
> > > Fix this in two ways.  First, check vcpu->pre_pcpu in pi_pre_block
> > > and WARN instead of adding the element twice in the list.  Second,
> > > always do the list removal in pi_post_block if vcpu->pre_pcpu is
> > > set (not -1).
> > > 
> > > The new code keeps interrupts disabled for the whole duration of
> > > pi_pre_block/pi_post_block.  This is not strictly necessary, but
> > > easier to follow.  For the same reason, PI.ON is checked only
> > > after the cmpxchg, and to handle it we just call the post-block
> > > code.  This removes duplication of the list removal code.
> > > 
> > > Cc: Longpeng (Mike) <longpeng2@huawei.com>
> > > Cc: Huangweidong <weidong.huang@huawei.com>
> > > Cc: Gonglei <arei.gonglei@huawei.com>
> > > Cc: wangxin <wangxinxin.wang@huawei.com>
> > > Cc: Radim Krčmář <rkrcmar@redhat.com>
> > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > > ---
> > >  arch/x86/kvm/vmx.c | 62
> > >  ++++++++++++++++++++++--------------------------------
> > >  1 file changed, 25 insertions(+), 37 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > index 747d16525b45..0f4714fe4908 100644
> > > --- a/arch/x86/kvm/vmx.c
> > > +++ b/arch/x86/kvm/vmx.c
> > > @@ -11236,10 +11236,11 @@ static void __pi_post_block(struct kvm_vcpu
> > > *vcpu)
> > >  	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > >  	struct pi_desc old, new;
> > >  	unsigned int dest;
> > > -	unsigned long flags;
> > >  
> > >  	do {
> > >  		old.control = new.control = pi_desc->control;
> > > +		WARN(old.nv != POSTED_INTR_WAKEUP_VECTOR,
> > > +		     "Wakeup handler not enabled while the VCPU is blocked\n");
> > >  
> > >  		dest = cpu_physical_id(vcpu->cpu);
> > >  
> > > @@ -11256,14 +11257,10 @@ static void __pi_post_block(struct kvm_vcpu
> > > *vcpu)
> > >  	} while (cmpxchg(&pi_desc->control, old.control,
> > >  			new.control) != old.control);
> > >  
> > > -	if(vcpu->pre_pcpu != -1) {
> > > -		spin_lock_irqsave(
> > > -			&per_cpu(blocked_vcpu_on_cpu_lock,
> > > -			vcpu->pre_pcpu), flags);
> > > +	if (!WARN_ON_ONCE(vcpu->pre_pcpu == -1)) {
> > > +		spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> > >  		list_del(&vcpu->blocked_vcpu_list);
> > > -		spin_unlock_irqrestore(
> > > -			&per_cpu(blocked_vcpu_on_cpu_lock,
> > > -			vcpu->pre_pcpu), flags);
> > > +		spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> > >  		vcpu->pre_pcpu = -1;
> > >  	}
> > >  }
> > > @@ -11283,7 +11280,6 @@ static void __pi_post_block(struct kvm_vcpu *vcpu)
> > >   */
> > >  static int pi_pre_block(struct kvm_vcpu *vcpu)
> > >  {
> > > -	unsigned long flags;
> > >  	unsigned int dest;
> > >  	struct pi_desc old, new;
> > >  	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
> > > @@ -11293,34 +11289,20 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
> > >  		!kvm_vcpu_apicv_active(vcpu))
> > >  		return 0;
> > >  
> > > -	vcpu->pre_pcpu = vcpu->cpu;
> > > -	spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
> > > -			  vcpu->pre_pcpu), flags);
> > > -	list_add_tail(&vcpu->blocked_vcpu_list,
> > > -		      &per_cpu(blocked_vcpu_on_cpu,
> > > -		      vcpu->pre_pcpu));
> > > -	spin_unlock_irqrestore(&per_cpu(blocked_vcpu_on_cpu_lock,
> > > -			       vcpu->pre_pcpu), flags);
> > > +	WARN_ON(irqs_disabled());
> > > +	local_irq_disable();
> > > +	if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
> > > +		vcpu->pre_pcpu = vcpu->cpu;
> > > +		spin_lock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> > > +		list_add_tail(&vcpu->blocked_vcpu_list,
> > > +			      &per_cpu(blocked_vcpu_on_cpu,
> > > +				       vcpu->pre_pcpu));
> > > +		spin_unlock(&per_cpu(blocked_vcpu_on_cpu_lock, vcpu->pre_pcpu));
> > > +	}
> > >  
> > >  	do {
> > >  		old.control = new.control = pi_desc->control;
> > >  
> > > -		/*
> > > -		 * We should not block the vCPU if
> > > -		 * an interrupt is posted for it.
> > > -		 */
> > > -		if (pi_test_on(pi_desc) == 1) {
> > > -			spin_lock_irqsave(&per_cpu(blocked_vcpu_on_cpu_lock,
> > > -					  vcpu->pre_pcpu), flags);
> > > -			list_del(&vcpu->blocked_vcpu_list);
> > > -			spin_unlock_irqrestore(
> > > -					&per_cpu(blocked_vcpu_on_cpu_lock,
> > > -					vcpu->pre_pcpu), flags);
> > > -			vcpu->pre_pcpu = -1;
> > > -
> > > -			return 1;
> > 
> > [1]
> > 
> > > -		}
> > > -
> > >  		WARN((pi_desc->sn == 1),
> > >  		     "Warning: SN field of posted-interrupts "
> > >  		     "is set before blocking\n");
> > > @@ -11345,7 +11327,12 @@ static int pi_pre_block(struct kvm_vcpu *vcpu)
> > >  	} while (cmpxchg(&pi_desc->control, old.control,
> > >  			new.control) != old.control);
> > >  
> > > -	return 0;
> > > +	/* We should not block the vCPU if an interrupt is posted for it.  */
> > > +	if (pi_test_on(pi_desc) == 1)
> > > +		__pi_post_block(vcpu);
> > 
> > A question on when pi_test_on() is set:
> > 
> > The old code will return 1 if detected (ses [1]), while the new code
> > does not. Would that matter? (IIUC that decides whether the vcpu will
> > continue to run?)
> 
> The new code does, because __pi_post_block resets vcpu->pre_pcpu to -1.

Sorry I overlook that. :-)

> 
> > > +
> > > +	local_irq_enable();
> > > +	return (vcpu->pre_pcpu == -1);
> > 
> > Above we have:
> > 
> > 	if (!WARN_ON_ONCE(vcpu->pre_pcpu != -1)) {
> > 		vcpu->pre_pcpu = vcpu->cpu;
> >                 ...
> > 	}
> > 
> > Then can here vcpu->pre_pcpu really be -1?
> 
> See above. :)

Yes. Then there's no problem.

> 
> > >  }
> > >  
> > >  static int vmx_pre_block(struct kvm_vcpu *vcpu)
> > > @@ -11361,12 +11348,13 @@ static int vmx_pre_block(struct kvm_vcpu *vcpu)
> > >  
> > >  static void pi_post_block(struct kvm_vcpu *vcpu)
> > >  {
> > > -	if (!kvm_arch_has_assigned_device(vcpu->kvm) ||
> > > -		!irq_remapping_cap(IRQ_POSTING_CAP)  ||
> > > -		!kvm_vcpu_apicv_active(vcpu))
> > > +	if (vcpu->pre_pcpu == -1)
> > >  		return;
> > >  
> > > +	WARN_ON(irqs_disabled());
> > > +	local_irq_disable();
> > >  	__pi_post_block(vcpu);
> > > +	local_irq_enable();
> > >  }
> > >  
> > >  static void vmx_post_block(struct kvm_vcpu *vcpu)
> > > --
> > > 2.13.0
> > > 
> > > 
> > 
> > A general question to pre_block/post_block handling for PI:
> > 
> > I see that we are handling PI logic mostly in four places:
> > 
> > vmx_vcpu_pi_{load|put}
> > pi_{pre_post}_block
> > 
> > But do we really need the pre_block/post_block handling? Here's how I
> > understand when vcpu blocked:
> > 
> > - vcpu_block
> >   - ->pre_block
> >   - kvm_vcpu_block [1]
> >     - schedule()
> >       - kvm_sched_out
> >         - vmx_vcpu_pi_put [3]
> >       - (another process working) ...
> >       - kvm_sched_in
> >         - vmx_vcpu_pi_load [4]
> >   - ->post_block [2]
> > 
> > If so, [1] & [2] will definitely be paired with [3] & [4], then why we
> > need [3] & [4] at all?
> > 
> > (Though [3] & [4] will also be used when preemption happens, so they
> >  are required)
> > 
> > Please kindly figure out if I missed anything important...
> 
> Oh, I see what you mean: set up the wakeup handler in vmx_vcpu_pi_put
> and rely on PI.ON to wake up the sleeping process immediately.  That
> should be feasible, but overall I like the current pre_block/post_block
> structure, and I think it's simpler.  The only thing to be careful about
> is leaving the IRTE unmodified when scheduling out a blocked VCPU, which
> is cleaned up and simplified in patch 3.
> 
> So I understand that the state may seem a bit too complicated as
> of this patch, but hopefully the next two make it clearer.

After re-read the codes and patches I got the point. Indeed current
way should be clearer since pre/post_block are mostly handling NV/DST
while pi_load/put are for SN bit.  Thanks!

-- 
Peter Xu

[toc] | [prev] | [next] | [standalone]

#1661064

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-06-08 13:30 +0200
Message-ID<tQ3Nv-3Dr-1@gated-at.bofh.it>
In reply to#1660967

On 08/06/2017 11:16, Peter Xu wrote:
>> Oh, I see what you mean: set up the wakeup handler in vmx_vcpu_pi_put
>> and rely on PI.ON to wake up the sleeping process immediately.  That
>> should be feasible, but overall I like the current pre_block/post_block
>> structure, and I think it's simpler.  The only thing to be careful about
>> is leaving the IRTE unmodified when scheduling out a blocked VCPU, which
>> is cleaned up and simplified in patch 3.
>>
>> So I understand that the state may seem a bit too complicated as
>> of this patch, but hopefully the next two make it clearer.
> After re-read the codes and patches I got the point. Indeed current
> way should be clearer since pre/post_block are mostly handling NV/DST
> while pi_load/put are for SN bit.  Thanks!

Almost: pi_load handles NDST too.  However, I think with patch 3 it's
clearer how pi_load handles the nesting inside pre_block...post_block.

Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1661883

FromPeter Xu <peterx@redhat.com>
Date2017-06-09 05:00 +0200
Message-ID<tQijw-4dh-19@gated-at.bofh.it>
In reply to#1661064
On Thu, Jun 08, 2017 at 01:24:44PM +0200, Paolo Bonzini wrote:
> 
> 
> On 08/06/2017 11:16, Peter Xu wrote:
> >> Oh, I see what you mean: set up the wakeup handler in vmx_vcpu_pi_put
> >> and rely on PI.ON to wake up the sleeping process immediately.  That
> >> should be feasible, but overall I like the current pre_block/post_block
> >> structure, and I think it's simpler.  The only thing to be careful about
> >> is leaving the IRTE unmodified when scheduling out a blocked VCPU, which
> >> is cleaned up and simplified in patch 3.
> >>
> >> So I understand that the state may seem a bit too complicated as
> >> of this patch, but hopefully the next two make it clearer.
> > After re-read the codes and patches I got the point. Indeed current
> > way should be clearer since pre/post_block are mostly handling NV/DST
> > while pi_load/put are for SN bit.  Thanks!
> 
> Almost: pi_load handles NDST too.  However, I think with patch 3 it's
> clearer how pi_load handles the nesting inside pre_block...post_block.

Yes. The old codes & comments for vmx_vcpu_pi_load() are not very easy
to understand for me.

For patch 3, not sure whether moving clear_sn() upper would be
clearer:

pi_load()
{
  if (!pi_test_bit() && vcpu->cpu == cpu)
    return;

  /* Unconditionally clear SN */
  pi_clear_sn();

  /*
   * If cpu not changed, no need to switch PDST; if WAKEUP, post_block
   * will do it for us
   */
  if (vcpu->cpu == cpu || nv == WAKEUP)
    return;

  /*
   * Update PDST. Possibly the vcpu thread switched from one cpu to
   * another.
   */
  do {
    ...
  } while (...)
}

Even, I'm thinking whether we can unconditionally setup PDST only in
pi_load(), then post_block() only needs to handle the NV bit.

(PS. since I'm at here... could I ask why in pi_pre_block we need to
 udpate PDST as well? I guess that decides who will run the
 wakeup_handler code to kick the vcpu thread, but would that really
 matter?)

Thanks,

-- 
Peter Xu

[toc] | [prev] | [next] | [standalone]

#1662030

FromPaolo Bonzini <pbonzini@redhat.com>
Date2017-06-09 09:40 +0200
Message-ID<tQmGv-75y-31@gated-at.bofh.it>
In reply to#1661883

On 09/06/2017 04:50, Peter Xu wrote:
> Even, I'm thinking whether we can unconditionally setup PDST only in
> pi_load(), then post_block() only needs to handle the NV bit.

No, you can't do that without fiddling with the blocked_vcpu lists in
pi_load.

> (PS. since I'm at here... could I ask why in pi_pre_block we need to
>  udpate PDST as well? I guess that decides who will run the
>  wakeup_handler code to kick the vcpu thread, but would that really
>  matter?)

For this one it's a yes. :)  I think it's not needed anymore indeed
after these patches; see this comment:

                /*
                 * The wakeup_handler expects the VCPU to be on the
                 * blocked_vcpu_list that matches ndst.  Interrupts
                 * are disabled so no preemption should happen, but
                 * err on the side of safety.
                 */

So we could add a WARN.

Paolo

[toc] | [prev] | [next] | [standalone]

#1662035

FromPeter Xu <peterx@redhat.com>
Date2017-06-09 09:50 +0200
Message-ID<tQmQ9-78W-1@gated-at.bofh.it>
In reply to#1662030
On Fri, Jun 09, 2017 at 09:29:45AM +0200, Paolo Bonzini wrote:
> 
> 
> On 09/06/2017 04:50, Peter Xu wrote:
> > Even, I'm thinking whether we can unconditionally setup PDST only in
> > pi_load(), then post_block() only needs to handle the NV bit.
> 
> No, you can't do that without fiddling with the blocked_vcpu lists in
> pi_load.

Then how about we keep the blocked_vcpu list maintainance in
post_block(), but only let pi_load() handle the PDST?

(I really feel like they are two things - the blocked_vcpu list helps
 for the kick when wakeup happens; while PDST makes sure the PI is
 always pointing to the correct cpu)

> 
> > (PS. since I'm at here... could I ask why in pi_pre_block we need to
> >  udpate PDST as well? I guess that decides who will run the
> >  wakeup_handler code to kick the vcpu thread, but would that really
> >  matter?)
> 
> For this one it's a yes. :)  I think it's not needed anymore indeed
> after these patches; see this comment:
> 
>                 /*
>                  * The wakeup_handler expects the VCPU to be on the
>                  * blocked_vcpu_list that matches ndst.

Actually I was always unclear on what this sentense means: iiuc
blocked_vcpu_list is only a list of vcpus that may need a kick, so why
it has anything to do with PDST after all?

(or say, no matter what PDST is, we just kick the vcpu thread without
 doing anything else, do we?)

> Interrupts
>                  * are disabled so no preemption should happen, but
>                  * err on the side of safety.
>                  */
> 
> So we could add a WARN.

Thanks,

-- 
Peter Xu

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web