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

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

> > --- 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.
> 
> Indeed. But all I was talking is last_pcpu (whereas you once again talk about 
> last_vcpu).
> 
> >     In summary, I think it is enough to solve the issue in vpmu_load() and 
> > vpmu_arch_destroy().
> 
> That's what I alluded to in my reply.
> 
> >     After cpu_callback() function, per_cpu(last_vcpu, vpmu->last_pcpu)
> > will be NULL
> 
> No. per_cpu(..., <offlined-pcpu>) simply is invalid.
> 
> > and VPMU_CONTEXT_LOADED will be clear.
> >     In vpmu_arch_destroy(), there will not make remote call to clear last.
> 
> I don't understand this sentence.

I mean per_cpu(..., <offlined-pcpu>) will be NULL after cpu_callback(), so that 
"per_cpu(last_vcpu, vpmu->last_pcpu) == v" check in 
vpmu_arch_destroy() will be fail when last_pcpu is the offlined pCPU. Or, it 
may make a remote call to clear the last_vpcu (vpmu_clear_last()).
But I don't understand why simply is invalid, last_vcpu set to NULL is 
presented in source code.  How to comprehend it?

Thanks,
Luwei Kang

> 
> >     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?
> 
> I continue to think that it needs to be made sure last_pcpu is valid before 
> using it for anything. Agreed, my previous suggestion of
> simply storing an invalid value was not very useful, as the questionable 
> comparison is != (when making the suggestion I did wrongly
> rememeber it to be == ), but that doesn't eliminate the need to sanity check 
> the value before use. Perhaps all that's needed are a
> couple of cpu_online() checks.
> 
> Jan


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