[PATCH v2] x86/vpmu: add cpu hot unplug notifier for vpmu

[PATCH v2] x86/vpmu: add cpu hot unplug notifier for vpmu

[Luwei Kang]

Currently, Hot unplug a physical CPU with vpmu enabled may cause
system hang due to send a remote call to an offlined pCPU. This
patch add a cpu hot unplug notifer to save vpmu context before
cpu offline.

Consider one scenario, hot unplug pCPU N with vpmu enabled.
The vcpu which running on this pCPU will be switch to other
online cpu. A remote call will be send to pCPU N to save the 
vpmu context before loading the vpmu context on this pCPU.
System will hang in function on_select_cpus() because of that pCPU
is offlined and can not do any respond.

Signed-off-by: Luwei Kang <luwei.kang@intel.com>
---
v2:
 1.fix some typo and coding style;
 2.change "swith" to "if" in cpu_callback() because of there just have one case;
 3.add VPMU_CONTEX_LOADED check before send remote call in vpmu_arch_destroy();
---
 xen/arch/x86/cpu/vpmu.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/cpu/vpmu.c b/xen/arch/x86/cpu/vpmu.c
index 03401fd..57a0e9d 100644
--- a/xen/arch/x86/cpu/vpmu.c
+++ b/xen/arch/x86/cpu/vpmu.c
@@ -21,6 +21,7 @@
 #include <xen/xenoprof.h>
 #include <xen/event.h>
 #include <xen/guest_access.h>
+#include <xen/cpu.h>
 #include <asm/regs.h>
 #include <asm/types.h>
 #include <asm/msr.h>
@@ -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);
     }
 }
@@ -835,6 +841,33 @@ long do_xenpmu_op(unsigned int op, XEN_GUEST_HANDLE_PARAM(xen_pmu_params_t) arg)
     return ret;
 }
 
+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;
+
+    if ( action == CPU_DYING )
+    {
+        vpmu_save_force(vcpu);
+        vpmu_reset(vpmu, VPMU_CONTEXT_LOADED);
+    }
+
+    return NOTIFY_DONE;
+}
+
+static struct notifier_block vpmu_cpu_nfb = {
+    .notifier_call = cpu_callback
+};
+
 static int __init vpmu_init(void)
 {
     int vendor = current_cpu_data.x86_vendor;
@@ -872,8 +905,11 @@ static int __init vpmu_init(void)
     }
 
     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
         opt_vpmu_enabled = 0;
 
-- 
1.8.3.1


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

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

[Jan Beulich]

>>> On 17.05.17 at 17:57, <luwei.kang@intel.com> 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.

Jan


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

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

[Kang, Luwei]

> >>> On 17.05.17 at 17:57, <luwei.kang@intel.com> 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Boris Ostrovsky]

On 05/18/2017 05:07 AM, Jan Beulich wrote:
>>>> On 17.05.17 at 17:57, <luwei.kang@intel.com> 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). 

From correctness perspective I don't see any problems.

As I said last time, I'd rename cpu_callback() to something less
generic, like vpmu_cpu_callback() (or vpmu_cpuhp_callback()).

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

I wonder whether we should have such an ASSERT() in on_selected_cpus()
instead.

-boris



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

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

[Jan Beulich]

>>> On 18.05.17 at 13:51, <luwei.kang@intel.com> wrote:
>> >>> On 17.05.17 at 17:57, <luwei.kang@intel.com> 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.

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.

>     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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Jan Beulich]

>>> On 18.05.17 at 15:03, <boris.ostrovsky@oracle.com> wrote:
> On 05/18/2017 05:07 AM, Jan Beulich wrote:
>>>>> On 17.05.17 at 17:57, <luwei.kang@intel.com> 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). 
> 
> From correctness perspective I don't see any problems.
> 
> As I said last time, I'd rename cpu_callback() to something less
> generic, like vpmu_cpu_callback() (or vpmu_cpuhp_callback()).

The vpmu_ prefix is clearly pointless for a static function.

>> 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.
> 
> I wonder whether we should have such an ASSERT() in on_selected_cpus()
> instead.

That's a good idea (and I'll queue a patch to that effect for
post-4.9), but it won't deal with all issues here. Namely the use
of last_pcpu in vpmu_arch_destroy() which the v2 patch didn't
touch is a problem already before calling on_selected_cpus():
per_cpu(last_vcpu, vpmu->last_pcpu) is simply invalid if
vpmu->last_pcpu is offline.

Jan


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

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

[Jan Beulich]

>>> On 18.05.17 at 15:06, <JBeulich@suse.com> wrote:
>>>> On 18.05.17 at 13:51, <luwei.kang@intel.com> wrote:
>>     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 == ),

And I was wrong here and right originally:

int vpmu_load(struct vcpu *v, bool_t from_guest)
{
    ...
    /* First time this VCPU is running here */
    if ( vpmu->last_pcpu != pcpu )
    {

If last_pcpu was NR_CPUS or nr_cpu_ids or any other always
invalid value, this condition would be true, which is exactly what
we want. Whereas if you leave the field alone, and another (or
the same) CPU comes (back) up with that number, we may
wrongly not enter the if() body.

Jan


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

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

[Kang, Luwei]

> > --- 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@lists.xen.org
https://lists.xen.org/xen-devel

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

[Dario Faggioli]

[-- Attachment #1.1: Type: text/plain, Size: 1407 bytes --]

On Thu, 2017-05-18 at 07:16 -0600, Jan Beulich wrote:
> > > > On 18.05.17 at 15:03, <boris.ostrovsky@oracle.com> wrote:
> > 
> > As I said last time, I'd rename cpu_callback() to something less
> > generic, like vpmu_cpu_callback() (or vpmu_cpuhp_callback()).
> 
> The vpmu_ prefix is clearly pointless for a static function.
> 
And "just" using cpu_callback is what we do in a lot of (although, not
everywhere :-( ) other places:

xen/common/timer.c:    .notifier_call = cpu_callback,
xen/common/kexec.c:    .notifier_call = cpu_callback
xen/common/cpupool.c:    .notifier_call = cpu_callback
xen/arch/x86/hvm/hvm.c:    .notifier_call = cpu_callback
xen/arch/x86/cpu/mcheck/mce_intel.c:    .notifier_call = cpu_callback
xen/common/stop_machine.c:    .notifier_call = cpu_callback
xen/common/tmem_xen.c:    .notifier_call = cpu_callback
xen/common/tasklet.c:    .notifier_call = cpu_callback,
xen/common/trace.c:    .notifier_call = cpu_callback
xen/drivers/passthrough/io.c:    .notifier_call = cpu_callback,
xen/drivers/cpufreq/cpufreq.c:    .notifier_call = cpu_callback

Dario
-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://about.me/dario.faggioli
Senior Software Engineer, Citrix Systems R&D Ltd., Cambridge (UK)

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 127 bytes --]

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

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

[Jan Beulich]

>>> On 18.05.17 at 16:23, <luwei.kang@intel.com> wrote:
>> > --- 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?

per_cpu(..., <offlined-pcpu>) will fault once the CPU is actually
offline. See free_percpu_area().

Jan


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

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

[Kang, Luwei]

> >>> On 18.05.17 at 16:23, <luwei.kang@intel.com> wrote:
> >> > --- 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?
> 
> per_cpu(..., <offlined-pcpu>) will fault once the CPU is actually offline. See free_percpu_area().

Oh, yes. Thanks for your clarification.

Thanks,
Luwei Kang

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