Re: [Xen-devel] [PATCH v3 3/6] VMX: Cleanup PI per-cpu blocking list when vcpu is destroyed

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Dario Faggioli [mailto:dario.faggioli@xxxxxxxxxx]
> Sent: Tuesday, September 6, 2016 5:22 PM
> To: Wu, Feng <feng.wu@xxxxxxxxx>; xen-devel@xxxxxxxxxxxxx
> Cc: Tian, Kevin <kevin.tian@xxxxxxxxx>; george.dunlap@xxxxxxxxxxxxx;
> andrew.cooper3@xxxxxxxxxx; jbeulich@xxxxxxxx
> Subject: Re: [Xen-devel] [PATCH v3 3/6] VMX: Cleanup PI per-cpu blocking list
> when vcpu is destroyed
> 
> On Wed, 2016-08-31 at 11:56 +0800, Feng Wu wrote:
> > We should remove the vCPU from the per-cpu blocking list
> > if it is going to be destroyed.
> >
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> > ---
> >  xen/arch/x86/hvm/vmx/vmx.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/xen/arch/x86/hvm/vmx/vmx.c b/xen/arch/x86/hvm/vmx/vmx.c
> > index b869728..37fa2f1 100644
> > --- a/xen/arch/x86/hvm/vmx/vmx.c
> > +++ b/xen/arch/x86/hvm/vmx/vmx.c
> > @@ -346,6 +346,7 @@ static void vmx_vcpu_destroy(struct vcpu *v)
> >      vmx_destroy_vmcs(v);
> >      vpmu_destroy(v);
> >      passive_domain_destroy(v);
> > +    vmx_pi_blocking_cleanup(v);
> >
> I'm not too much into VMX, so I may be wrong (in which case, sorry),
> but is it safe to call this after vmx_destroy_vmcs() ?

It is safe, since vmx_pi_blocking_cleanup(v) doesn't deal with any
real VMCS stuff, it just handle some the pi blocking queue. But I think
your doubt here is available, maybe it looks more reasonable to
move it before vmx_destory_vmcs().

> 
> Also (even if it is), we're basically calling and executing the
> following (called by vmx_pi_blocking_clanup()):
> 
> static void vmx_pi_remove_vcpu_from_blocking_list(struct vcpu *v)
> {
>     unsigned long flags;
>     spinlock_t *pi_blocking_list_lock;
>     struct pi_desc *pi_desc = &v->arch.hvm_vmx.pi_desc;
> 
>     /*
>      * Set 'NV' field back to posted_intr_vector, so the
>      * Posted-Interrupts can be delivered to the vCPU when
>      * it is running in non-root mode.
>      */
>     write_atomic(&pi_desc->nv, posted_intr_vector);
> 
>     /* The vCPU is not on any blocking list. */
>     pi_blocking_list_lock = v->arch.hvm_vmx.pi_blocking.lock;
> 
>     /* Prevent the compiler from eliminating the local variable.*/
>     smp_rmb();
> 
>     if ( pi_blocking_list_lock == NULL )
>         return;
> 
>     spin_lock_irqsave(pi_blocking_list_lock, flags);
> 
>     /*
>      * v->arch.hvm_vmx.pi_blocking.lock == NULL here means the vCPU
>      * was removed from the blocking list while we are acquiring the lock.
>      */
>     if ( v->arch.hvm_vmx.pi_blocking.lock != NULL )
>     {
>         ASSERT(v->arch.hvm_vmx.pi_blocking.lock == pi_blocking_list_lock);
>         list_del(&v->arch.hvm_vmx.pi_blocking.list);
>         v->arch.hvm_vmx.pi_blocking.lock = NULL;
>     }
> 
>     spin_unlock_irqrestore(pi_blocking_list_lock, flags);
> }
> 
> Considering that we're destroying, isn't this too much? Maybe it's not
> a big deal, but I'd have expected that all is needed here is something
> like:
> 
>  if ( v->arch.hvm_vmx.pi_blocking.lock )
>  {
>      spin_lock_irqsave(..);
>      list_del(..);
>      spin_unlock_irqrestore(..);
>  }
> 
> Maybe the resume, list_remove, and cleanup functions need to be broken
> up a bit more/better?

Actually, this function carefully handles some cases, since
' v->arch.hvm_vmx.pi_blocking.lock ' can be set to NULL in other places,
such as, in pi_wakeup_interrupt(), so even we are in the destroy path,
we still need to save it to a local variable and check its value, then delete
the vCPU from the list if needed. So if we really want to eliminate something
in this function for the destroy path, maybe we don't need to restore the NV
field. If that is the case, seems this is not a big deal :)

> 
> Also, as a side note (which I think would be more appropriate as a
> comment to patch 1, but bear with me, I'm just back from vacations, I
> have a lot of catch up to do, and I'm in hurry! :-P), now that the
> function is called vmx_pi_remove_vcpu_from_blocking_list(), this
> comment being part of its body sounds a bit weird:
> 
>     ...
>     /* The vCPU is not on any blocking list. */
>     pi_blocking_list_loc
> k = v->arch.hvm_vmx.pi_blocking.lock;
>     ...
> 
> I'd take the chance for rephrasing it.

Oh, yes, this needs some adjustment. Thanks for the comments!

Thanks,
Feng

> 
> Regards,
> Dario
> --
> <<This happens because I choose it to happen!>> (Raistlin Majere)
> -----------------------------------------------------------------
> Dario Faggioli, Ph.D, http://about.me/dario.faggioli
> Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel