[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH for 4.5] x86/VPMU: Clear last_vcpu when destroying VPMU



On 12/15/2014 05:07 AM, Jan Beulich wrote:
On 12.12.14 at 22:20, <boris.ostrovsky@xxxxxxxxxx> wrote:
--- a/xen/arch/x86/hvm/vpmu.c
+++ b/xen/arch/x86/hvm/vpmu.c
@@ -247,10 +247,32 @@ void vpmu_initialise(struct vcpu *v)
      }
  }
+static void vpmu_clear_last(void *arg)
+{
+    struct vcpu *v = (struct vcpu *)arg;
Please drop this pointless cast, or perhaps the entire variable, as ...

+
+    if ( this_cpu(last_vcpu) == v )
... you don't really need it to be of "struct vcpu *" type - "void *"
is quite fine for a comparison.

+        this_cpu(last_vcpu) = NULL;
+}
+
  void vpmu_destroy(struct vcpu *v)
  {
      struct vpmu_struct *vpmu = vcpu_vpmu(v);
+ if ( vpmu_is_set(vpmu, VPMU_CONTEXT_ALLOCATED) )
+    {
+        /* Need to clear last_vcpu in case it points to v */
+        if ( vpmu->last_pcpu != smp_processor_id() )
+            on_selected_cpus(cpumask_of(vpmu->last_pcpu),
+                             vpmu_clear_last, (void *)v, 1);
The cast here is pointless too. But considering your subsequent
reply this code may go away altogether anyway; if it doesn't,
explaining (in the commit message) why you need to use an IPI
here would seem necessary.

If I do simply
    if (per_cpu(last_vcpu, vpmu->last_pcpu) == v)
        per_cpu(last_vcpu, vpmu->last_pcpu) = NULL

then there is a (rather theoretical) possibility that between the test and subsequent clearing the remote cpu (i.e. vpmu->last_pcpu) will do load_vpmu() and then save_vpmu() for another VCPU. The former will clear last_vcpu and the latter will set last_vcpu to something else. And then the destroy_vpmu() will set it again to NULL, which is bad.

Doing it in in IPI will guarantee that nothing can happen between test and setting it to NULL.

Again, this very much theoretical, but that's why I have it. (BTW, doing this via IPI also preserves assumption that last_vcpu is always updated on local CPU.)

My changes for next version would make the need to do the IPIs less frequent. But if last_cpu needs to be cleared it would still be via IPI.

-boris

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.