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

Re: [Xen-devel] [PATCH v13 for-xen-4.5 11/21] x86/VPMU: Interface for setting PMU mode and flags



On 10/13/2014 09:51 AM, Jan Beulich wrote:
On 03.10.14 at 23:40, <boris.ostrovsky@xxxxxxxxxx> wrote:
@@ -274,3 +292,176 @@ void vpmu_dump(struct vcpu *v)
          vpmu->arch_vpmu_ops->arch_vpmu_dump(v);
  }
+static atomic_t vpmu_sched_counter;
+
+static void vpmu_sched_checkin(unsigned long unused)
+{
+    atomic_inc(&vpmu_sched_counter);
+}
+
+static int vpmu_force_context_switch(void)
+{
+    unsigned i, numcpus, mycpu;
+    s_time_t start;
+    static DEFINE_PER_CPU(struct tasklet *, sync_task);
+    int ret = 0;
+
+    numcpus = num_online_cpus();
+    mycpu = smp_processor_id();
+
+    for ( i = 0; i < numcpus; i++ )
for_each_online_cpu() would have saved you from running into ...

+    {
+        if ( i == mycpu )
+            continue;
+
+        per_cpu(sync_task, i) = xmalloc(struct tasklet);
... a crash here when some CPU other than the highest numbered
one got offlined.

+        if ( per_cpu(sync_task, i) == NULL )
+        {
+            printk(XENLOG_WARNING "vpmu_force_context_switch: out of 
memory\n");
+            ret = -ENOMEM;
+            goto out;
+        }
+        tasklet_init(per_cpu(sync_task, i), vpmu_sched_checkin, 0);
+    }
+
+    /* First count is for self */
+    atomic_set(&vpmu_sched_counter, 1);
+
+    for_each_online_cpu( i )
And oddly enough you use it here (albeit with malformed style -
either you drop the two blanks inside the parentheses or you add
another before the opening one)...

+    {
+        if ( i != mycpu )
+            tasklet_schedule_on_cpu(per_cpu(sync_task, i), i);
+    }
+
+    vpmu_save(current);
+
+    start = NOW();
+
+    /*
+     * Note that we may fail here if a CPU is hot-plugged while we are
+     * waiting. We will then time out.
+     */
+    while ( atomic_read(&vpmu_sched_counter) != numcpus )
+    {
+        s_time_t now;
+
+        cpu_relax();
+
+        now = NOW();
+
+        /* Give up after 5 seconds */
+        if ( now > start + SECONDS(5) )
+        {
+            printk(XENLOG_WARNING
+                   "vpmu_force_context_switch: failed to sync\n");
+            ret = -EBUSY;
+            break;
+        }
+
+        /* Or after (arbitrarily chosen) 2ms if need to be preempted */
+        if ( (now > start + MILLISECS(2)) && hypercall_preempt_check() )
+        {
+            ret = -EAGAIN;
+            break;
+        }
+    }
So this time round you don't retain any state between retries at all.
How is this process expected to ever complete on a large and loaded
enough system?

Since we now leave completion of the mode change request to user we can't guarantee that it will ever happen. So if we were to keep the state of who has passed through the context switch we'd have to prevent VPMUs on those CPUs from being loaded again. We don't have such mechanism now (unless we introduce a per-CPU VPMU mode which is not a good idea imo).

With continuations we could leave VPMU mode in new state and revert it to old if continuations failed but with things left to user we can't do this.

This has been the case for the last few versions btw.

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