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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 21.05.17 at 15:09, <luwei.kang@xxxxxxxxx> wrote:
> v3:
>  1.add cpu_online() check in vpm_load() and vpmu_arch_destroy();
>  2.add vpmu_ prefix. rename cpu_callback() to vpmu_cpu_callback();

I had specifically objected to the latter.

> @@ -394,8 +395,11 @@ int vpmu_load(struct vcpu *v, bool_t from_guest)
>      if ( !vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
>          return 0;
>  
> -    /* First time this VCPU is running here */
> -    if ( vpmu->last_pcpu != pcpu )
> +    /*
> +     * The last pCPU is still online and this is the first time this vCPU
> +     * running here.
> +     */
> +    if ( cpu_online(vpmu->last_pcpu) && vpmu->last_pcpu != pcpu )

Adding a cpu_online() check here is unlikely to be helpful. Actually I
may have misguided you with prior comments (and if so, I'm sorry) -
the LOADED check following this one makes sure on_selected_cpus()
won't be called with an offline CPU here. IOW I think the code can
be left untouched, but the reason why should be spelled out in the
commit message (matching the reasoning why adding the LOADED
check to vpmu_arch_destroy() is sufficient for the second use of
last_pcpu there).

> @@ -575,15 +579,21 @@ static void vpmu_arch_destroy(struct vcpu *v)
>       * We will test it again in vpmu_clear_last() with interrupts
>       * disabled to make sure we don't clear someone else.
>       */
> -    if ( per_cpu(last_vcpu, vpmu->last_pcpu) == v )
> +    if ( cpu_online(vpmu->last_pcpu) &&
> +            per_cpu(last_vcpu, vpmu->last_pcpu) == v )

Indentation.

Jan


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