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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

> --- 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