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

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

> >>> On 16.05.17 at 19:29, <luwei.kang@xxxxxxxxx> wrote:
> > Currently, hot unplug a cpu with vpmu enabled may cause system hang
> > due to send IPI to a die physical cpu. This patch add a cpu hot unplug
> > notifer to save vpmu context before cpu offline.
> >
> > Consider one scene, hotplug physical cpu N with vpmu is enabled.
> 
> I think you mean "scenario" and "hot unplug".
> 
> > The vcpu which running on this physical cpu before will be switch to
> > other online cpu. Before load the vpmu context to new physical cpu, a
> > IPI will be send to cpu N to save the vpmu context.
> > System will hang in function on_select_cpus because of that physical
> > cpu is offline and can not do any response.
> 
> Doesn't this make clear that you would better also make sure
> ->last_pcpu doesn't hold to the then stale CPU anymore? For
> example, vpmu_load() compares it with smp_processor_id() (the subsequent use 
> is guarded by a VPMU_CONTEXT_LOADED flag
> check), allowing badness if the same or another CPU with the same number 
> comes up again quickly enough. Similarly
> vpmu_arch_destroy() uses it without checking VPMU_CONTEXT_LOADED.

Hi Jan,
    I think it may can't make sure  "->last_pcpu" doesn't hold to the then 
stale CPU. The purpose of this notifier is to save the vpmu context before cpu 
offline. Avoid save vpmu context by send IPI to that offline cpu. There is no 
reason to change the value except it saving (vpmu_save()) in another physical 
cpu.
    Regarding vpmu_arch_destroy(), it indeed will cause same issue. What about 
add " this_cpu(cpu) = NULL" in cpu_callback() to clean the last_vcpu pointer of 
this physical cpu. 
    In addition, add VPMU_CONTEXT_LOADED check before execute 
on_selected_cpus(cpumask_of(vcpu_vpmu(v)->last_pcpu), vpmu_save_force, v, 1) in 
vpmu_arch_destroy(). Because of force save operation has been finished in 
notifier function.

Thanks,
Luwei Kang

> 
> > --- a/xen/arch/x86/cpu/vpmu.c
> > +++ b/xen/arch/x86/cpu/vpmu.c
> > @@ -35,6 +35,7 @@
> >  #include <asm/apic.h>
> >  #include <public/pmu.h>
> >  #include <xsm/xsm.h>
> > +#include <xen/cpu.h>
> 
> Please place this in the group of other xen/ includes.
> 
> > +static int cpu_callback(struct notifier_block *nfb, unsigned long
> > +action, void *hcpu) {
> > +    unsigned int cpu = (unsigned long)hcpu;
> > +    struct vcpu *vcpu = per_cpu(last_vcpu, cpu);
> > +    struct vpmu_struct *vpmu;
> > +
> > +    if ( !vcpu )
> > +        return NOTIFY_DONE;
> > +
> > +    vpmu = vcpu_vpmu(vcpu);
> > +    if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
> > +        return NOTIFY_DONE;
> > +
> > +    switch ( action )
> > +    {
> > +    case CPU_DYING:
> > +        vpmu_save_force(vcpu);
> > +        vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
> > +        break;
> > +    default:
> > +        break;
> 
> Pointless default case.
> 
> > @@ -871,10 +902,11 @@ static int __init vpmu_init(void)
> >          break;
> >      }
> >
> > -    if ( vpmu_mode != XENPMU_MODE_OFF )
> > +    if ( vpmu_mode != XENPMU_MODE_OFF ) {
> > +        register_cpu_notifier(&vpmu_cpu_nfb);
> >          printk(XENLOG_INFO "VPMU: version " __stringify(XENPMU_VER_MAJ) "."
> >                 __stringify(XENPMU_VER_MIN) "\n");
> > -    else
> > +    } else
> 
> Coding style (brace placement).
> 
> Jan


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