Re: [Xen-devel] [PATCH v2] x86/vpmu: add cpu hot unplug notifier for vpmu

["Kang, Luwei" <luwei.kang@xxxxxxxxx>]

> >>> On 17.05.17 at 17:57, <luwei.kang@xxxxxxxxx> wrote:
> > @@ -581,9 +582,14 @@ static void vpmu_arch_destroy(struct vcpu *v)
> >
> >      if ( vpmu->arch_vpmu_ops && vpmu->arch_vpmu_ops->arch_vpmu_destroy )
> >      {
> > -        /* Unload VPMU first. This will stop counters */
> > -        on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu),
> > -                         vpmu_save_force, v, 1);
> > +        /*
> > +         * Unload VPMU first if VPMU_CONTEXT_LOADED being set.
> > +         * This will stop counters.
> > +         */
> > +        if ( vpmu_is_set(vpmu, VPMU_CONTEXT_LOADED) )
> > +            on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu),
> > +                             vpmu_save_force, v, 1);
> > +
> >           vpmu->arch_vpmu_ops->arch_vpmu_destroy(v);
> >      }
> >  }
> 
> So this is a good step towards what was requested during v1 review, provided 
> it is correct (I'll let Boris comment). You didn't,
> however, do anything about the other unguarded last_pcpu uses (in vpmu_load() 
> and upwards from the code above in
> vpmu_arch_destroy()). These _may_ be implicitly fine, but if so please at 
> least add suitable ASSERT()s.
> 

Hi Jan,
    Thanks for your reply. I think I understand the issue you mentioned. But 
sorry,  I am not very clear what is your solution from your description.
    At first, I want to change like this:
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -859,6 +859,7 @@ static int cpu_callback(
     {
         vpmu_save_force(vcpu);
         vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
+        per_cpu(last_vcpu, cpu) = NULL;        // OR: this_cpu(last_vcpu) = 
NULL;
     }
    As you mentioned in before comments, it has been done in vpmu_save_force(). 
So this change is unnecessary.

    In summary, I think it is enough to solve the issue in vpmu_load() and 
vpmu_arch_destroy().
    After cpu_callback() function, per_cpu(last_vcpu, vpmu->last_pcpu) will be 
NULL and VPMU_CONTEXT_LOADED will be clear.
    In vpmu_arch_destroy(), there will not make remote call to clear last.
    In vpmu_load(), remote call will guarded by VPMU_CONTEXT_LOADED flag check. 
As for vpmu->last_pcpu, we can't use some random online one to produce false.
    What is your opinion?

Thanks,
Luwei Kang


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