[Xen-devel] [PATCH v2] xen: idle_loop: either deal with tasklets or go idle

[Dario Faggioli <dario.faggioli@xxxxxxxxxx>]

In fact, there are two kinds of tasklets: vCPU and
softirq context. When we want to do vCPU context tasklet
work, we force the idle vCPU (of a particular pCPU) into
execution, and run it from there.

This means there are two possible reasons for choosing
to run the idle vCPU:
1) we want a pCPU to go idle,
2) we want to run some vCPU context tasklet work.

If we're in case 2), it does not make sense to even
try to go idle (as the check will _always_ fail).

This patch rearranges the code of the body of idle
vCPUs, so that we actually check whether we are in
case 1) or 2), and act accordingly.

As a matter of fact, this also means that we do not
check if there's any tasklet work to do after waking
up from idle. This is not a problem, because:
a) for softirq context tasklets, if any is queued
   "during" wakeup from idle, TASKLET_SOFTIRQ is
   raised, and the call to do_softirq() (which is still
   happening *after* the wakeup) will take care of it;
b) for vCPU context tasklets, if any is queued "during"
   wakeup from idle, SCHEDULE_SOFTIRQ is raised and
   do_softirq() (happening after the wakeup) calls
   the scheduler. The scheduler sees that there is
   tasklet work pending and confirms the idle vCPU
   in execution, which then will get to execute
   do_tasklet().

Signed-off-by: Dario Faggioli <dario.faggioli@xxxxxxxxxx>
---
Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
Cc: Julien Grall <julien.grall@xxxxxxx>
Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
Cc: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
Cc: Jan Beulich <jbeulich@xxxxxxxx>
---
Changes from v1:
* drop the pointless parentheses and indirection of pm_idle (in x86's idle
  loop);
* remove the 'cpu' input parameter to do_tasklet(), as suggested during review;
* in do_tasklet(), convert the check that there is tasklet work to do into an
  ASSERT() to the same effect, as suggested during review;
* add a comment in cpu_is_haltable() on why we check the per-cpu
  tasklet_work_to_do variable directly, rather than calling the new
  tasklet_work_to_do() helper.
---
 xen/arch/arm/domain.c     |   21 ++++++++++++++-------
 xen/arch/x86/domain.c     |   12 +++++++++---
 xen/common/tasklet.c      |   12 ++++++++----
 xen/include/xen/sched.h   |    5 +++++
 xen/include/xen/tasklet.h |   10 ++++++++++
 5 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 76310ed..2dc8b0a 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -41,20 +41,27 @@ DEFINE_PER_CPU(struct vcpu *, curr_vcpu);
 
 void idle_loop(void)
 {
+    unsigned int cpu = smp_processor_id();
+
     for ( ; ; )
     {
-        if ( cpu_is_offline(smp_processor_id()) )
+        if ( cpu_is_offline(cpu) )
             stop_cpu();
 
-        local_irq_disable();
-        if ( cpu_is_haltable(smp_processor_id()) )
+        /* Are we here for running vcpu context tasklets, or for idling? */
+        if ( unlikely(tasklet_work_to_do(cpu)) )
+            do_tasklet();
+        else
         {
-            dsb(sy);
-            wfi();
+            local_irq_disable();
+            if ( cpu_is_haltable(cpu) )
+            {
+                dsb(sy);
+                wfi();
+            }
+            local_irq_enable();
         }
-        local_irq_enable();
 
-        do_tasklet();
         do_softirq();
         /*
          * We MUST be last (or before dsb, wfi). Otherwise after we get the
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 49388f4..3a061a9 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -112,12 +112,18 @@ static void play_dead(void)
 
 static void idle_loop(void)
 {
+    unsigned int cpu = smp_processor_id();
+
     for ( ; ; )
     {
-        if ( cpu_is_offline(smp_processor_id()) )
+        if ( cpu_is_offline(cpu) )
             play_dead();
-        (*pm_idle)();
-        do_tasklet();
+
+        /* Are we here for running vcpu context tasklets, or for idling? */
+        if ( unlikely(tasklet_work_to_do(cpu)) )
+            do_tasklet();
+        else
+            pm_idle();
         do_softirq();
         /*
          * We MUST be last (or before pm_idle). Otherwise after we get the
diff --git a/xen/common/tasklet.c b/xen/common/tasklet.c
index 365a777..0f0a6f8 100644
--- a/xen/common/tasklet.c
+++ b/xen/common/tasklet.c
@@ -111,11 +111,15 @@ void do_tasklet(void)
     struct list_head *list = &per_cpu(tasklet_list, cpu);
 
     /*
-     * Work must be enqueued *and* scheduled. Otherwise there is no work to
-     * do, and/or scheduler needs to run to update idle vcpu priority.
+     * We want to be sure any caller has checked that a tasklet is both
+     * enqueued and scheduled, before calling this. And, if the caller has
+     * actually checked, it's not an issue that we are outside of the
+     * critical region, in fact:
+     * - TASKLET_enqueued is cleared only here,
+     * - TASKLET_scheduled is only cleared when schedule() find it set,
+     *   without TASKLET_enqueued being set as well.
      */
-    if ( likely(*work_to_do != (TASKLET_enqueued|TASKLET_scheduled)) )
-        return;
+    ASSERT(tasklet_work_to_do(cpu));
 
     spin_lock_irq(&tasklet_lock);
 
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
index 1127ca9..6673b27 100644
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -843,6 +843,11 @@ uint64_t get_cpu_idle_time(unsigned int cpu);
 /*
  * Used by idle loop to decide whether there is work to do:
  *  (1) Run softirqs; or (2) Play dead; or (3) Run tasklets.
+ *
+ * About (3), if a tasklet is enqueued, it will be scheduled
+ * really really soon, and hence it's pointless to try to
+ * sleep between these two events (that's why we don't call
+ * the tasklet_work_to_do() helper).
  */
 #define cpu_is_haltable(cpu)                    \
     (!softirq_pending(cpu) &&                   \
diff --git a/xen/include/xen/tasklet.h b/xen/include/xen/tasklet.h
index 8c3de7e..23d69c7 100644
--- a/xen/include/xen/tasklet.h
+++ b/xen/include/xen/tasklet.h
@@ -40,6 +40,16 @@ DECLARE_PER_CPU(unsigned long, tasklet_work_to_do);
 #define TASKLET_enqueued   (1ul << _TASKLET_enqueued)
 #define TASKLET_scheduled  (1ul << _TASKLET_scheduled)
 
+static inline bool tasklet_work_to_do(unsigned int cpu)
+{
+    /*
+     * Work must be enqueued *and* scheduled. Otherwise there is no work to
+     * do, and/or scheduler needs to run to update idle vcpu priority.
+     */
+    return per_cpu(tasklet_work_to_do, cpu) == (TASKLET_enqueued|
+                                                TASKLET_scheduled);
+}
+
 void tasklet_schedule_on_cpu(struct tasklet *t, unsigned int cpu);
 void tasklet_schedule(struct tasklet *t);
 void do_tasklet(void);


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