[PATCH 0 of 6] Xen: generalize and beautify scheduling related perfc and stats

[PATCH 0 of 6] Xen: generalize and beautify scheduling related perfc and stats

[Dario Faggioli]

Hello Everyone,

The main idea behind this series was to cleanup a bit the stats management
within the SEDF code in sched_sedf.c.  While looking into it, I thought the
best idea to achieve that was to generalize a little bit the stat and
perfcounter macros we have in credit, so that they could be used within the
other schedulers too.

That is then what these patches ended up doing.

It's definitely nothing urgent or particularly important now, but will help
if/when we decide to make a broader use of performance counters in the
scheduler code (and I really plan to do that for SEDF at some point! :-D).

Very few functional changes are introduced and no hunk is effective if the
hypervisor is not compiled with 'perfc=y'.

While at it, as I found out build is broken with 'perfc=y', the very first
patch in the series tries to fix that. Paul, as it seems it was you that
introduced those two counters, could you double check? (As it happens in
viridian.c and, for me, it could be like the first time or so I even hear the
word 'Viridian' ;-P.)

Thanks and Regards, Dario

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

[PATCH 1 of 6] xen: fix build when 'perfc=y'

[Dario Faggioli]

Which was failing with this:

 viridian.c: In function ‘wrmsr_viridian_regs’:
 viridian.c:254:1: error: ‘PERFC_mshv_wrmsr_apic_msr’ undeclared (first use in this function)
 viridian.c:254:1: note: each undeclared identifier is reported only once for each function it appears in
 viridian.c: In function ‘rdmsr_viridian_regs’:
 viridian.c:305:1: error: ‘PERFC_mshv_rdmsr_apic_msr’ undeclared (first use in this function)

as a consequence of 17b754cab7b0 using but not defining
the counters.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/include/asm-x86/perfc_defn.h b/xen/include/asm-x86/perfc_defn.h
--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -121,6 +121,7 @@ PERFCOUNTER(mshv_rdmsr_vp_index,        
 PERFCOUNTER(mshv_rdmsr_icr,             "MS Hv rdmsr icr")
 PERFCOUNTER(mshv_rdmsr_tpr,             "MS Hv rdmsr tpr")
 PERFCOUNTER(mshv_rdmsr_apic_assist,     "MS Hv rdmsr APIC assist")
+PERFCOUNTER(mshv_rdmsr_apic_msr,        "MS Hv rdmsr APIC msr")
 PERFCOUNTER(mshv_wrmsr_osid,            "MS Hv wrmsr Guest OS ID")
 PERFCOUNTER(mshv_wrmsr_hc_page,         "MS Hv wrmsr hypercall page")
 PERFCOUNTER(mshv_wrmsr_vp_index,        "MS Hv wrmsr vp index")
@@ -128,6 +129,7 @@ PERFCOUNTER(mshv_wrmsr_icr,             
 PERFCOUNTER(mshv_wrmsr_tpr,             "MS Hv wrmsr tpr")
 PERFCOUNTER(mshv_wrmsr_eoi,             "MS Hv wrmsr eoi")
 PERFCOUNTER(mshv_wrmsr_apic_assist,     "MS Hv wrmsr APIC assist")
+PERFCOUNTER(mshv_wrmsr_apic_msr,        "MS Hv wrmsr APIC msr")
 
 PERFCOUNTER(realmode_emulations, "realmode instructions emulated")
 PERFCOUNTER(realmode_exits,      "vmexits from realmode")
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[PATCH 2 of 6] xen: move `printk("Initializing domain")` from credit to generic scheduling code

[Dario Faggioli]

As, if it makes sense to let people know about it, it probably always does.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -1454,8 +1454,6 @@ csched_dom_init(const struct scheduler *
 {
     struct csched_dom *sdom;
 
-    printk("%s: Initializing domain %d\n", __func__, dom->domain_id);
-
     if ( is_idle_domain(dom) )
         return 0;
 
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -312,6 +312,8 @@ void sched_destroy_vcpu(struct vcpu *v)
 
 int sched_init_domain(struct domain *d)
 {
+    printk("%s: Initializing domain %d\n", __func__, d->domain_id);
+
     return SCHED_OP(DOM2OP(d), init_domain, d);
 }

[PATCH 3 of 6] xen: sched: generalize scheduling related perfcounter macros

[Dario Faggioli]

Moving some of them from sched_credit.c to generic scheduler code.
This also allows the other schedulers to use perf counters equally
easy.

This change is mainly preparatory work for what stated above. In fact,
it mostly does s/CSCHED_STAT/SCHED_STAT/, and, in general, it implies no
functional changes.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
--- a/xen/common/sched_credit.c
+++ b/xen/common/sched_credit.c
@@ -16,25 +16,12 @@
 #include <xen/delay.h>
 #include <xen/event.h>
 #include <xen/time.h>
-#include <xen/perfc.h>
 #include <xen/sched-if.h>
 #include <xen/softirq.h>
 #include <asm/atomic.h>
 #include <xen/errno.h>
 #include <xen/keyhandler.h>
 
-/*
- * CSCHED_STATS
- *
- * Manage very basic per-vCPU counters and stats.
- *
- * Useful for debugging live systems. The stats are displayed
- * with runq dumps ('r' on the Xen console).
- */
-#ifdef PERF_COUNTERS
-#define CSCHED_STATS
-#endif
-
 
 /*
  * Basic constants
@@ -75,29 +62,36 @@
 
 
 /*
- * Stats
+ * CSCHED_STATS
+ *
+ * Manage very basic per-vCPU counters and stats.
+ *
+ * Useful for debugging live systems. The stats are displayed
+ * with runq dumps ('r' on the Xen console).
  */
-#define CSCHED_STAT_CRANK(_X)               (perfc_incr(_X))
+#ifdef SCHED_STATS
 
-#ifdef CSCHED_STATS
+#define CSCHED_STATS
 
-#define CSCHED_VCPU_STATS_RESET(_V)                     \
+#define SCHED_VCPU_STATS_RESET(_V)                      \
     do                                                  \
     {                                                   \
         memset(&(_V)->stats, 0, sizeof((_V)->stats));   \
     } while ( 0 )
 
-#define CSCHED_VCPU_STAT_CRANK(_V, _X)      (((_V)->stats._X)++)
+#define SCHED_VCPU_STAT_CRANK(_V, _X)       (((_V)->stats._X)++)
 
-#define CSCHED_VCPU_STAT_SET(_V, _X, _Y)    (((_V)->stats._X) = (_Y))
+#define SCHED_VCPU_STAT_SET(_V, _X, _Y)     (((_V)->stats._X) = (_Y))
 
-#else /* CSCHED_STATS */
+#else /* !SCHED_STATS */
 
-#define CSCHED_VCPU_STATS_RESET(_V)         do {} while ( 0 )
-#define CSCHED_VCPU_STAT_CRANK(_V, _X)      do {} while ( 0 )
-#define CSCHED_VCPU_STAT_SET(_V, _X, _Y)    do {} while ( 0 )
+#undef CSCHED_STATS
 
-#endif /* CSCHED_STATS */
+#define SCHED_VCPU_STATS_RESET(_V)         do {} while ( 0 )
+#define SCHED_VCPU_STAT_CRANK(_V, _X)      do {} while ( 0 )
+#define SCHED_VCPU_STAT_SET(_V, _X, _Y)    do {} while ( 0 )
+
+#endif /* SCHED_STATS */
 
 
 /*
@@ -264,13 +258,13 @@ static inline void
     if ( new->pri > cur->pri )
     {
         if ( cur->pri == CSCHED_PRI_IDLE )
-            CSCHED_STAT_CRANK(tickle_local_idler);
+            SCHED_STAT_CRANK(tickle_local_idler);
         else if ( cur->pri == CSCHED_PRI_TS_OVER )
-            CSCHED_STAT_CRANK(tickle_local_over);
+            SCHED_STAT_CRANK(tickle_local_over);
         else if ( cur->pri == CSCHED_PRI_TS_UNDER )
-            CSCHED_STAT_CRANK(tickle_local_under);
+            SCHED_STAT_CRANK(tickle_local_under);
         else
-            CSCHED_STAT_CRANK(tickle_local_other);
+            SCHED_STAT_CRANK(tickle_local_other);
 
         cpumask_set_cpu(cpu, &mask);
     }
@@ -283,7 +277,7 @@ static inline void
     {
         if ( cpumask_empty(prv->idlers) )
         {
-            CSCHED_STAT_CRANK(tickle_idlers_none);
+            SCHED_STAT_CRANK(tickle_idlers_none);
         }
         else
         {
@@ -292,7 +286,7 @@ static inline void
             cpumask_and(&idle_mask, prv->idlers, new->vcpu->cpu_affinity);
             if ( !cpumask_empty(&idle_mask) )
             {
-                CSCHED_STAT_CRANK(tickle_idlers_some);
+                SCHED_STAT_CRANK(tickle_idlers_some);
                 if ( opt_tickle_one_idle )
                 {
                     this_cpu(last_tickle_cpu) = 
@@ -404,7 +398,7 @@ static inline void
         BUG_ON( !is_idle_vcpu(vc) );
     }
 
-    CSCHED_STAT_CRANK(vcpu_check);
+    SCHED_STAT_CRANK(vcpu_check);
 }
 #define CSCHED_VCPU_CHECK(_vc)  (__csched_vcpu_check(_vc))
 #else
@@ -437,7 +431,7 @@ static inline int
                ((uint64_t)vcpu_migration_delay * 1000u));
 
     if ( hot )
-        CSCHED_STAT_CRANK(vcpu_hot);
+        SCHED_STAT_CRANK(vcpu_hot);
 
     return hot;
 }
@@ -559,8 +553,8 @@ static inline void
 
     if ( list_empty(&svc->active_vcpu_elem) )
     {
-        CSCHED_VCPU_STAT_CRANK(svc, state_active);
-        CSCHED_STAT_CRANK(acct_vcpu_active);
+        SCHED_VCPU_STAT_CRANK(svc, state_active);
+        SCHED_STAT_CRANK(acct_vcpu_active);
 
         sdom->active_vcpu_count++;
         list_add(&svc->active_vcpu_elem, &sdom->active_vcpu);
@@ -583,8 +577,8 @@ static inline void
 
     BUG_ON( list_empty(&svc->active_vcpu_elem) );
 
-    CSCHED_VCPU_STAT_CRANK(svc, state_idle);
-    CSCHED_STAT_CRANK(acct_vcpu_idle);
+    SCHED_VCPU_STAT_CRANK(svc, state_idle);
+    SCHED_STAT_CRANK(acct_vcpu_idle);
 
     BUG_ON( prv->weight < sdom->weight );
     sdom->active_vcpu_count--;
@@ -633,8 +627,8 @@ csched_vcpu_acct(struct csched_private *
     }
     else if ( _csched_cpu_pick(ops, current, 0) != cpu )
     {
-        CSCHED_VCPU_STAT_CRANK(svc, migrate_r);
-        CSCHED_STAT_CRANK(migrate_running);
+        SCHED_VCPU_STAT_CRANK(svc, migrate_r);
+        SCHED_STAT_CRANK(migrate_running);
         set_bit(_VPF_migrating, &current->pause_flags);
         cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
     }
@@ -658,8 +652,8 @@ csched_alloc_vdata(const struct schedule
     svc->flags = 0U;
     svc->pri = is_idle_domain(vc->domain) ?
         CSCHED_PRI_IDLE : CSCHED_PRI_TS_UNDER;
-    CSCHED_VCPU_STATS_RESET(svc);
-    CSCHED_STAT_CRANK(vcpu_init);
+    SCHED_VCPU_STATS_RESET(svc);
+    SCHED_STAT_CRANK(vcpu_init);
     return svc;
 }
 
@@ -690,7 +684,7 @@ csched_vcpu_remove(const struct schedule
     struct csched_dom * const sdom = svc->sdom;
     unsigned long flags;
 
-    CSCHED_STAT_CRANK(vcpu_destroy);
+    SCHED_STAT_CRANK(vcpu_destroy);
 
     if ( __vcpu_on_runq(svc) )
         __runq_remove(svc);
@@ -711,7 +705,7 @@ csched_vcpu_sleep(const struct scheduler
 {
     struct csched_vcpu * const svc = CSCHED_VCPU(vc);
 
-    CSCHED_STAT_CRANK(vcpu_sleep);
+    SCHED_STAT_CRANK(vcpu_sleep);
 
     BUG_ON( is_idle_vcpu(vc) );
 
@@ -731,19 +725,19 @@ csched_vcpu_wake(const struct scheduler 
 
     if ( unlikely(per_cpu(schedule_data, cpu).curr == vc) )
     {
-        CSCHED_STAT_CRANK(vcpu_wake_running);
+        SCHED_STAT_CRANK(vcpu_wake_running);
         return;
     }
     if ( unlikely(__vcpu_on_runq(svc)) )
     {
-        CSCHED_STAT_CRANK(vcpu_wake_onrunq);
+        SCHED_STAT_CRANK(vcpu_wake_onrunq);
         return;
     }
 
     if ( likely(vcpu_runnable(vc)) )
-        CSCHED_STAT_CRANK(vcpu_wake_runnable);
+        SCHED_STAT_CRANK(vcpu_wake_runnable);
     else
-        CSCHED_STAT_CRANK(vcpu_wake_not_runnable);
+        SCHED_STAT_CRANK(vcpu_wake_not_runnable);
 
     /*
      * We temporarly boost the priority of awaking VCPUs!
@@ -883,8 +877,6 @@ csched_dom_init(const struct scheduler *
 {
     struct csched_dom *sdom;
 
-    CSCHED_STAT_CRANK(dom_init);
-
     if ( is_idle_domain(dom) )
         return 0;
 
@@ -906,7 +898,6 @@ csched_free_domdata(const struct schedul
 static void
 csched_dom_destroy(const struct scheduler *ops, struct domain *dom)
 {
-    CSCHED_STAT_CRANK(dom_destroy);
     csched_free_domdata(ops, CSCHED_DOM(dom));
 }
 
@@ -989,18 +980,18 @@ csched_acct(void* dummy)
     if ( prv->credit_balance < 0 )
     {
         credit_total -= prv->credit_balance;
-        CSCHED_STAT_CRANK(acct_balance);
+        SCHED_STAT_CRANK(acct_balance);
     }
 
     if ( unlikely(weight_total == 0) )
     {
         prv->credit_balance = 0;
         spin_unlock_irqrestore(&prv->lock, flags);
-        CSCHED_STAT_CRANK(acct_no_work);
+        SCHED_STAT_CRANK(acct_no_work);
         goto out;
     }
 
-    CSCHED_STAT_CRANK(acct_run);
+    SCHED_STAT_CRANK(acct_run);
 
     weight_left = weight_total;
     credit_balance = 0;
@@ -1075,7 +1066,7 @@ csched_acct(void* dummy)
                  * the queue to give others a chance at them in future
                  * accounting periods.
                  */
-                CSCHED_STAT_CRANK(acct_reorder);
+                SCHED_STAT_CRANK(acct_reorder);
                 list_del(&sdom->active_sdom_elem);
                 list_add(&sdom->active_sdom_elem, &prv->active_sdom);
             }
@@ -1110,7 +1101,7 @@ csched_acct(void* dummy)
                      credit < -credit_cap &&
                      !(svc->flags & CSCHED_FLAG_VCPU_PARKED) )
                 {
-                    CSCHED_STAT_CRANK(vcpu_park);
+                    SCHED_STAT_CRANK(vcpu_park);
                     vcpu_pause_nosync(svc->vcpu);
                     svc->flags |= CSCHED_FLAG_VCPU_PARKED;
                 }
@@ -1118,7 +1109,7 @@ csched_acct(void* dummy)
                 /* Lower bound on credits */
                 if ( credit < -prv->credits_per_tslice )
                 {
-                    CSCHED_STAT_CRANK(acct_min_credit);
+                    SCHED_STAT_CRANK(acct_min_credit);
                     credit = -prv->credits_per_tslice;
                     atomic_set(&svc->credit, credit);
                 }
@@ -1135,7 +1126,7 @@ csched_acct(void* dummy)
                      * call to make sure the VCPU's priority is not boosted
                      * if it is woken up here.
                      */
-                    CSCHED_STAT_CRANK(vcpu_unpark);
+                    SCHED_STAT_CRANK(vcpu_unpark);
                     vcpu_unpause(svc->vcpu);
                     svc->flags &= ~CSCHED_FLAG_VCPU_PARKED;
                 }
@@ -1151,8 +1142,8 @@ csched_acct(void* dummy)
                 }
             }
 
-            CSCHED_VCPU_STAT_SET(svc, credit_last, credit);
-            CSCHED_VCPU_STAT_SET(svc, credit_incr, credit_fair);
+            SCHED_VCPU_STAT_SET(svc, credit_last, credit);
+            SCHED_VCPU_STAT_SET(svc, credit_incr, credit_fair);
             credit_balance += credit;
         }
     }
@@ -1229,8 +1220,8 @@ csched_runq_steal(int peer_cpu, int cpu,
             if (__csched_vcpu_is_migrateable(vc, cpu))
             {
                 /* We got a candidate. Grab it! */
-                CSCHED_VCPU_STAT_CRANK(speer, migrate_q);
-                CSCHED_STAT_CRANK(migrate_queued);
+                SCHED_VCPU_STAT_CRANK(speer, migrate_q);
+                SCHED_STAT_CRANK(migrate_queued);
                 WARN_ON(vc->is_urgent);
                 __runq_remove(speer);
                 vc->processor = cpu;
@@ -1239,7 +1230,7 @@ csched_runq_steal(int peer_cpu, int cpu,
         }
     }
 
-    CSCHED_STAT_CRANK(steal_peer_idle);
+    SCHED_STAT_CRANK(steal_peer_idle);
     return NULL;
 }
 
@@ -1260,11 +1251,11 @@ csched_load_balance(struct csched_privat
         goto out;
 
     if ( snext->pri == CSCHED_PRI_IDLE )
-        CSCHED_STAT_CRANK(load_balance_idle);
+        SCHED_STAT_CRANK(load_balance_idle);
     else if ( snext->pri == CSCHED_PRI_TS_OVER )
-        CSCHED_STAT_CRANK(load_balance_over);
+        SCHED_STAT_CRANK(load_balance_over);
     else
-        CSCHED_STAT_CRANK(load_balance_other);
+        SCHED_STAT_CRANK(load_balance_other);
 
     /*
      * Peek at non-idling CPUs in the system, starting with our
@@ -1288,7 +1279,7 @@ csched_load_balance(struct csched_privat
          */
         if ( !pcpu_schedule_trylock(peer_cpu) )
         {
-            CSCHED_STAT_CRANK(steal_trylock_failed);
+            SCHED_STAT_CRANK(steal_trylock_failed);
             continue;
         }
 
@@ -1327,7 +1318,7 @@ csched_schedule(
     struct task_slice ret;
     s_time_t runtime, tslice;
 
-    CSCHED_STAT_CRANK(schedule);
+    SCHED_STAT_CRANK(schedule);
     CSCHED_VCPU_CHECK(current);
 
     runtime = now - current->runstate.state_entry_time;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -314,11 +314,14 @@ int sched_init_domain(struct domain *d)
 {
     printk("%s: Initializing domain %d\n", __func__, d->domain_id);
 
+    SCHED_STAT_CRANK(dom_init);
+
     return SCHED_OP(DOM2OP(d), init_domain, d);
 }
 
 void sched_destroy_domain(struct domain *d)
 {
+    SCHED_STAT_CRANK(dom_destroy);
     SCHED_OP(DOM2OP(d), destroy_domain, d);
 }
 
@@ -1086,7 +1089,7 @@ static void schedule(void)
 
     ASSERT(!in_atomic());
 
-    perfc_incr(sched_run);
+    SCHED_STAT_CRANK(sched_run);
 
     sd = &this_cpu(schedule_data);
 
@@ -1164,7 +1167,7 @@ static void schedule(void)
 
     pcpu_schedule_unlock_irq(cpu);
 
-    perfc_incr(sched_ctx);
+    SCHED_STAT_CRANK(sched_ctx);
 
     stop_timer(&prev->periodic_timer);
 
@@ -1198,7 +1201,7 @@ void context_saved(struct vcpu *prev)
 static void s_timer_fn(void *unused)
 {
     raise_softirq(SCHEDULE_SOFTIRQ);
-    perfc_incr(sched_irq);
+    SCHED_STAT_CRANK(sched_irq);
 }
 
 /* Per-VCPU periodic timer function: sends a virtual timer interrupt. */
diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
--- a/xen/include/xen/perfc_defn.h
+++ b/xen/include/xen/perfc_defn.h
@@ -12,13 +12,19 @@ PERFCOUNTER(calls_from_multicall,       
 PERFCOUNTER(irqs,                   "#interrupts")
 PERFCOUNTER(ipis,                   "#IPIs")
 
+/* Generic scheduler counters (applicable to all schedulers) */
 PERFCOUNTER(sched_irq,              "sched: timer")
 PERFCOUNTER(sched_run,              "sched: runs through scheduler")
 PERFCOUNTER(sched_ctx,              "sched: context switches")
+PERFCOUNTER(schedule,               "sched: specific scheduler")
+PERFCOUNTER(dom_init,               "sched: dom_init")
+PERFCOUNTER(dom_destroy,            "sched: dom_destroy")
+PERFCOUNTER(vcpu_init,              "sched: vcpu_init")
+PERFCOUNTER(vcpu_destroy,           "sched: vcpu_destroy")
 
+/* credit specific counters */
 PERFCOUNTER(delay_ms,               "csched: delay")
 PERFCOUNTER(vcpu_check,             "csched: vcpu_check")
-PERFCOUNTER(schedule,               "csched: schedule")
 PERFCOUNTER(acct_run,               "csched: acct_run")
 PERFCOUNTER(acct_no_work,           "csched: acct_no_work")
 PERFCOUNTER(acct_balance,           "csched: acct_balance")
@@ -46,10 +52,6 @@ PERFCOUNTER(steal_trylock_failed,   "csc
 PERFCOUNTER(steal_peer_idle,        "csched: steal_peer_idle")
 PERFCOUNTER(migrate_queued,         "csched: migrate_queued")
 PERFCOUNTER(migrate_running,        "csched: migrate_running")
-PERFCOUNTER(dom_init,               "csched: dom_init")
-PERFCOUNTER(dom_destroy,            "csched: dom_destroy")
-PERFCOUNTER(vcpu_init,              "csched: vcpu_init")
-PERFCOUNTER(vcpu_destroy,           "csched: vcpu_destroy")
 PERFCOUNTER(vcpu_hot,               "csched: vcpu_hot")
 
 PERFCOUNTER(need_flush_tlb_flush,   "PG_need_flush tlb flushes")
diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
--- a/xen/include/xen/sched.h
+++ b/xen/include/xen/sched.h
@@ -16,6 +16,7 @@
 #include <xen/tasklet.h>
 #include <xen/mm.h>
 #include <xen/smp.h>
+#include <xen/perfc.h>
 #include <asm/atomic.h>
 #include <xen/wait.h>
 #include <public/xen.h>
@@ -29,6 +30,18 @@
 DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t);
 #endif
 
+/*
+ * Stats
+ *
+ * Enable and ease the use of scheduling related performance counters.
+ *
+ */
+#ifdef PERF_COUNTERS
+#define SCHED_STATS
+#endif
+
+#define SCHED_STAT_CRANK(_X)                (perfc_incr(_X))
+
 /* A global pointer to the initial domain (DOM0). */
 extern struct domain *dom0;

[PATCH 4 of 6] xen: sched: introduce a couple of counters in credit2 and SEDF

[Dario Faggioli]

Mainly for consistency with credit, at least for the events that are
general enough, like vCPU initialization/destruction and calls
to the specific scheduling function.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_credit2.c b/xen/common/sched_credit2.c
--- a/xen/common/sched_credit2.c
+++ b/xen/common/sched_credit2.c
@@ -753,6 +753,8 @@ csched_alloc_vdata(const struct schedule
         svc->weight = 0;
     }
 
+    SCHED_STAT_CRANK(vcpu_init);
+
     return svc;
 }
 
@@ -870,6 +872,8 @@ csched_vcpu_remove(const struct schedule
 
     if ( ! is_idle_vcpu(vc) )
     {
+        SCHED_STAT_CRANK(vcpu_destroy);
+
         /* Remove from runqueue */
         vcpu_schedule_lock_irq(vc);
 
@@ -1585,6 +1589,7 @@ csched_schedule(
     struct csched_vcpu *snext = NULL;
     struct task_slice ret;
 
+    SCHED_STAT_CRANK(schedule);
     CSCHED_VCPU_CHECK(current);
 
     d2printk("sc p%d c d%dv%d now %"PRI_stime"\n",
diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c
+++ b/xen/common/sched_sedf.c
@@ -332,6 +332,8 @@ static void *sedf_alloc_vdata(const stru
     INIT_LIST_HEAD(&(inf->extralist[EXTRA_PEN_Q]));
     INIT_LIST_HEAD(&(inf->extralist[EXTRA_UTIL_Q]));
 
+    SCHED_STAT_CRANK(vcpu_init);
+
     return inf;
 }
 
@@ -763,6 +765,8 @@ static struct task_slice sedf_do_schedul
     struct sedf_vcpu_info *runinf, *waitinf;
     struct task_slice      ret;
 
+    SCHED_STAT_CRANK(schedule);
+
     /* Idle tasks don't need any of the following stuf */
     if ( is_idle_vcpu(current) )
         goto check_waitq;

[PATCH 5 of 6] xen: sched_sedf: beautify statisics in SEDF

[Dario Faggioli]

By gathering all the related fields in a struct (as it is being done
in credit) and using the macros we now have available. No functional
changes involved.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c
+++ b/xen/common/sched_sedf.c
@@ -14,7 +14,6 @@
 #include <xen/errno.h>
 
 #ifndef NDEBUG
-#define SEDF_STATS
 #define CHECK(_p)                                           \
     do {                                                    \
         if ( !(_p) )                                        \
@@ -45,6 +44,37 @@
 #define IMPLY(a, b) (!(a) || (b))
 #define EQ(a, b) ((!!(a)) == (!!(b)))
 
+/*
+ * SEDF_STATS
+ *
+ * Some statistics about vCPU execution.
+ *
+ * Some of them are displayed with runq dumps
+ * ('r' on the Xen console).
+ */
+#ifdef SCHED_STATS
+
+#define SEDF_STATS
+
+#define SCHED_VCPU_STATS_RESET(_V)                      \
+    do                                                  \
+    {                                                   \
+        memset(&(_V)->stats, 0, sizeof((_V)->stats));   \
+    } while ( 0 )
+
+#define SCHED_VCPU_STAT_CRANK(_V, _X)       (((_V)->stats._X)++)
+
+/*#define SCHED_VCPU_STAT_SET(_V, _X, _Y)     (((_V)->stats._X) = (_Y))*/
+
+#else /* !SCHED_STATS */
+
+#undef SEDF_STATS
+
+#define SCHED_VCPU_STATS_RESET(_V)         do {} while ( 0 )
+#define SCHED_VCPU_STAT_CRANK(_V, _X)      do {} while ( 0 )
+/*#define SCHED_VCPU_STAT_SET(_V, _X, _Y)    do {} while ( 0 )*/
+
+#endif /* SCHED_STATS */
 
 struct sedf_dom_info {
     struct domain  *domain;
@@ -92,14 +122,16 @@ struct sedf_vcpu_info {
     s_time_t  extra_time_tot;
 
 #ifdef SEDF_STATS
-    s_time_t  block_time_tot;
-    s_time_t  penalty_time_tot;
-    int   block_tot;
-    int   short_block_tot;
-    int   long_block_tot;
-    int   short_cont;
-    int   pen_extra_blocks;
-    int   pen_extra_slices;
+    struct {
+        s_time_t block_time_tot;
+        s_time_t penalty_time_tot;
+        int block_tot;
+        int short_block_tot;
+        int long_block_tot;
+        int short_cont;
+        int pen_extra_blocks;
+        int pen_extra_slices;
+    } stats;
 #endif
 };
 
@@ -332,6 +364,7 @@ static void *sedf_alloc_vdata(const stru
     INIT_LIST_HEAD(&(inf->extralist[EXTRA_PEN_Q]));
     INIT_LIST_HEAD(&(inf->extralist[EXTRA_UTIL_Q]));
 
+    SCHED_VCPU_STATS_RESET(inf);
     SCHED_STAT_CRANK(vcpu_init);
 
     return inf;
@@ -688,9 +721,7 @@ static struct task_slice sedf_do_extra_s
         runinf->status |= EXTRA_RUN_PEN;
         ret.task = runinf->vcpu;
         ret.time = EXTRA_QUANTUM;
-#ifdef SEDF_STATS
-        runinf->pen_extra_slices++;
-#endif
+        SCHED_VCPU_STAT_CRANK(runinf, pen_extra_slices);
     }
     else
     {
@@ -989,9 +1020,7 @@ static void unblock_short_extra_support(
         {
             inf->score[0] = (inf->period << 10) /
                 inf->short_block_lost_tot;
-#ifdef SEDF_STATS
-            inf->pen_extra_blocks++;
-#endif
+            SCHED_VCPU_STAT_CRANK(inf, pen_extra_blocks);
             if ( extraq_on(inf->vcpu, EXTRA_PEN_Q) )
                 /* Remove domain for possible resorting! */
                 extraq_del(inf->vcpu, EXTRA_PEN_Q);
@@ -1108,9 +1137,7 @@ static void sedf_wake(const struct sched
         inf->deadl_abs = now + inf->slice;
     }
   
-#ifdef SEDF_STATS 
-    inf->block_tot++;
-#endif
+    SCHED_VCPU_STAT_CRANK(inf, block_tot);
 
     if ( unlikely(now < PERIOD_BEGIN(inf)) )
     {
@@ -1132,9 +1159,7 @@ static void sedf_wake(const struct sched
         if ( now < inf->deadl_abs )
         {
             /* Short blocking */
-#ifdef SEDF_STATS
-            inf->short_block_tot++;
-#endif
+            SCHED_VCPU_STAT_CRANK(inf, short_block_tot);
             unblock_short_extra_support(inf, now);
 
             extraq_check_add_unblocked(d, 1);
@@ -1142,9 +1167,7 @@ static void sedf_wake(const struct sched
         else
         {
             /* Long unblocking */
-#ifdef SEDF_STATS
-            inf->long_block_tot++;
-#endif
+            SCHED_VCPU_STAT_CRANK(inf, long_block_tot);
             unblock_long_cons_b(inf, now);
 
             extraq_check_add_unblocked(d, 1);
@@ -1160,8 +1183,8 @@ static void sedf_wake(const struct sched
     /* Do some statistics here... */
     if ( inf->block_abs != 0 )
     {
-        inf->block_time_tot += now - inf->block_abs;
-        inf->penalty_time_tot +=
+        inf->stats.block_time_tot += now - inf->block_abs;
+        inf->stats.penalty_time_tot +=
             PERIOD_BEGIN(inf) + inf->cputime - inf->block_abs;
     }
 #endif
@@ -1197,22 +1220,22 @@ static void sedf_dump_domain(struct vcpu
            EDOM_INFO(d)->extra_time_tot, EDOM_INFO(d)->extraweight);
     
 #ifdef SEDF_STATS
-    if ( EDOM_INFO(d)->block_time_tot != 0 )
-        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->penalty_time_tot * 100) /
-               EDOM_INFO(d)->block_time_tot);
-    if ( EDOM_INFO(d)->block_tot != 0 )
+    if ( EDOM_INFO(d)->stats.block_time_tot != 0 )
+        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->stats.penalty_time_tot * 100) /
+               EDOM_INFO(d)->stats.block_time_tot);
+    if ( EDOM_INFO(d)->stats.block_tot != 0 )
         printk("\n   blks=%u sh=%u (%u%%) (shc=%u (%u%%) shex=%i "\
                "shexsl=%i) l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
-               EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_block_tot,
-               (EDOM_INFO(d)->short_block_tot * 100) 
-               / EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_cont,
-               (EDOM_INFO(d)->short_cont * 100) / EDOM_INFO(d)->block_tot,
-               EDOM_INFO(d)->pen_extra_blocks,
-               EDOM_INFO(d)->pen_extra_slices,
-               EDOM_INFO(d)->long_block_tot,
-               (EDOM_INFO(d)->long_block_tot * 100) / EDOM_INFO(d)->block_tot,
-               (EDOM_INFO(d)->block_time_tot) / EDOM_INFO(d)->block_tot,
-               (EDOM_INFO(d)->penalty_time_tot) / EDOM_INFO(d)->block_tot);
+               EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_block_tot,
+               (EDOM_INFO(d)->stats.short_block_tot * 100) 
+               / EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_cont,
+               (EDOM_INFO(d)->stats.short_cont * 100) / EDOM_INFO(d)->stats.block_tot,
+               EDOM_INFO(d)->stats.pen_extra_blocks,
+               EDOM_INFO(d)->stats.pen_extra_slices,
+               EDOM_INFO(d)->stats.long_block_tot,
+               (EDOM_INFO(d)->stats.long_block_tot * 100) / EDOM_INFO(d)->stats.block_tot,
+               (EDOM_INFO(d)->stats.block_time_tot) / EDOM_INFO(d)->stats.block_tot,
+               (EDOM_INFO(d)->stats.penalty_time_tot) / EDOM_INFO(d)->stats.block_tot);
 #endif
     printk("\n");
 }

[PATCH 6 of 6] xen: sched_sedf: remove an unused stat in SEDF

[Dario Faggioli]

Namely, `short_cont' which is not updated anywhere in the code.

Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

diff --git a/xen/common/sched_sedf.c b/xen/common/sched_sedf.c
--- a/xen/common/sched_sedf.c
+++ b/xen/common/sched_sedf.c
@@ -128,7 +128,6 @@ struct sedf_vcpu_info {
         int block_tot;
         int short_block_tot;
         int long_block_tot;
-        int short_cont;
         int pen_extra_blocks;
         int pen_extra_slices;
     } stats;
@@ -1224,12 +1223,11 @@ static void sedf_dump_domain(struct vcpu
         printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->stats.penalty_time_tot * 100) /
                EDOM_INFO(d)->stats.block_time_tot);
     if ( EDOM_INFO(d)->stats.block_tot != 0 )
-        printk("\n   blks=%u sh=%u (%u%%) (shc=%u (%u%%) shex=%i "\
-               "shexsl=%i) l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
+        printk("\n   blks=%u sh=%u (%u%%) shex=%i (shexsl=%i) "\
+               "l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
                EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_block_tot,
                (EDOM_INFO(d)->stats.short_block_tot * 100) 
-               / EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_cont,
-               (EDOM_INFO(d)->stats.short_cont * 100) / EDOM_INFO(d)->stats.block_tot,
+               / EDOM_INFO(d)->stats.block_tot,
                EDOM_INFO(d)->stats.pen_extra_blocks,
                EDOM_INFO(d)->stats.pen_extra_slices,
                EDOM_INFO(d)->stats.long_block_tot,

Re: [PATCH 1 of 6] xen: fix build when 'perfc=y'

[George Dunlap]

On 22/10/12 15:40, Dario Faggioli wrote:
> Which was failing with this:
>
>   viridian.c: In function ‘wrmsr_viridian_regs’:
>   viridian.c:254:1: error: ‘PERFC_mshv_wrmsr_apic_msr’ undeclared (first use in this function)
>   viridian.c:254:1: note: each undeclared identifier is reported only once for each function it appears in
>   viridian.c: In function ‘rdmsr_viridian_regs’:
>   viridian.c:305:1: error: ‘PERFC_mshv_rdmsr_apic_msr’ undeclared (first use in this function)
>
> as a consequence of 17b754cab7b0 using but not defining
> the counters.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>


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

Re: [PATCH 2 of 6] xen: move `printk("Initializing domain")` from credit to generic scheduling code

[George Dunlap]

On 22/10/12 15:40, Dario Faggioli wrote:
> As, if it makes sense to let people know about it, it probably always does.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

The difference is, credit2 is explicity "experimental"; it's not 
unexpected to have a bunch of chatty debug messages when random things 
happen.  I don't think we want to print this on production systems 
running credit1.

Maybe change it to "KERN_DEBUG"?

  -George

Re: [PATCH 2 of 6] xen: move `printk("Initializing domain")` from credit to generic scheduling code

[Dario Faggioli]

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

On Tue, 2012-10-23 at 17:04 +0100, George Dunlap wrote:
> On 22/10/12 15:40, Dario Faggioli wrote:
> > As, if it makes sense to let people know about it, it probably always does.
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> The difference is, credit2 is explicity "experimental"; it's not 
> unexpected to have a bunch of chatty debug messages when random things 
> happen.  
>
I see.

> I don't think we want to print this on production systems 
> running credit1.
> 
Ok, I think I agree.

> Maybe change it to "KERN_DEBUG"?
> 
Well, that, or maybe just forget this patch (for now). I'm fine with
both. :-)

Thanks for looking at this,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/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: 198 bytes --]

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

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

Re: [PATCH 2 of 6] xen: move `printk("Initializing domain")` from credit to generic scheduling code

[George Dunlap]

On 23/10/12 17:13, Dario Faggioli wrote:
>> Maybe change it to "KERN_DEBUG"?
>>
> Well, that, or maybe just forget this patch (for now). I'm fine with
> both. :-)

OK -- for now why don't we drop it. :-)

  -George

Re: [PATCH 5 of 6] xen: sched_sedf: beautify statisics in SEDF

[George Dunlap]

On 22/10/12 15:40, Dario Faggioli wrote:
> By gathering all the related fields in a struct (as it is being done
> in credit) and using the macros we now have available. No functional
> changes involved.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>

I'm OK with this as it is, but...

>   #ifdef SEDF_STATS
> -    if ( EDOM_INFO(d)->block_time_tot != 0 )
> -        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->penalty_time_tot * 100) /
> -               EDOM_INFO(d)->block_time_tot);
> -    if ( EDOM_INFO(d)->block_tot != 0 )
> +    if ( EDOM_INFO(d)->stats.block_time_tot != 0 )
> +        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->stats.penalty_time_tot * 100) /
> +               EDOM_INFO(d)->stats.block_time_tot);
> +    if ( EDOM_INFO(d)->stats.block_tot != 0 )
>           printk("\n   blks=%u sh=%u (%u%%) (shc=%u (%u%%) shex=%i "\
>                  "shexsl=%i) l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
> -               EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_block_tot,
> -               (EDOM_INFO(d)->short_block_tot * 100)
> -               / EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_cont,
> -               (EDOM_INFO(d)->short_cont * 100) / EDOM_INFO(d)->block_tot,
> -               EDOM_INFO(d)->pen_extra_blocks,
> -               EDOM_INFO(d)->pen_extra_slices,
> -               EDOM_INFO(d)->long_block_tot,
> -               (EDOM_INFO(d)->long_block_tot * 100) / EDOM_INFO(d)->block_tot,
> -               (EDOM_INFO(d)->block_time_tot) / EDOM_INFO(d)->block_tot,
> -               (EDOM_INFO(d)->penalty_time_tot) / EDOM_INFO(d)->block_tot);
> +               EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_block_tot,
> +               (EDOM_INFO(d)->stats.short_block_tot * 100)
> +               / EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_cont,
> +               (EDOM_INFO(d)->stats.short_cont * 100) / EDOM_INFO(d)->stats.block_tot,
> +               EDOM_INFO(d)->stats.pen_extra_blocks,
> +               EDOM_INFO(d)->stats.pen_extra_slices,
> +               EDOM_INFO(d)->stats.long_block_tot,
> +               (EDOM_INFO(d)->stats.long_block_tot * 100) / EDOM_INFO(d)->stats.block_tot,
> +               (EDOM_INFO(d)->stats.block_time_tot) / EDOM_INFO(d)->stats.block_tot,
> +               (EDOM_INFO(d)->stats.penalty_time_tot) / EDOM_INFO(d)->stats.block_tot);

...wouldn't it be even more beautiful to have a macro for reading stats 
as well?

Like I said, it's fine as it is, but since you're looking for beauty, I 
figured I'd point it out. :-)

  -George

Re: [PATCH 3 of 6] xen: sched: generalize scheduling related perfcounter macros

[George Dunlap]

On 22/10/12 15:40, Dario Faggioli wrote:
> Moving some of them from sched_credit.c to generic scheduler code.
> This also allows the other schedulers to use perf counters equally
> easy.
>
> This change is mainly preparatory work for what stated above. In fact,
> it mostly does s/CSCHED_STAT/SCHED_STAT/, and, in general, it implies no
> functional changes.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>
>
> diff --git a/xen/common/sched_credit.c b/xen/common/sched_credit.c
> --- a/xen/common/sched_credit.c
> +++ b/xen/common/sched_credit.c
> @@ -16,25 +16,12 @@
>   #include <xen/delay.h>
>   #include <xen/event.h>
>   #include <xen/time.h>
> -#include <xen/perfc.h>
>   #include <xen/sched-if.h>
>   #include <xen/softirq.h>
>   #include <asm/atomic.h>
>   #include <xen/errno.h>
>   #include <xen/keyhandler.h>
>
> -/*
> - * CSCHED_STATS
> - *
> - * Manage very basic per-vCPU counters and stats.
> - *
> - * Useful for debugging live systems. The stats are displayed
> - * with runq dumps ('r' on the Xen console).
> - */
> -#ifdef PERF_COUNTERS
> -#define CSCHED_STATS
> -#endif
> -
>
>   /*
>    * Basic constants
> @@ -75,29 +62,36 @@
>
>
>   /*
> - * Stats
> + * CSCHED_STATS
> + *
> + * Manage very basic per-vCPU counters and stats.
> + *
> + * Useful for debugging live systems. The stats are displayed
> + * with runq dumps ('r' on the Xen console).
>    */
> -#define CSCHED_STAT_CRANK(_X)               (perfc_incr(_X))
> +#ifdef SCHED_STATS
>
> -#ifdef CSCHED_STATS
> +#define CSCHED_STATS
>
> -#define CSCHED_VCPU_STATS_RESET(_V)                     \
> +#define SCHED_VCPU_STATS_RESET(_V)                      \
>       do                                                  \
>       {                                                   \
>           memset(&(_V)->stats, 0, sizeof((_V)->stats));   \
>       } while ( 0 )
>
> -#define CSCHED_VCPU_STAT_CRANK(_V, _X)      (((_V)->stats._X)++)
> +#define SCHED_VCPU_STAT_CRANK(_V, _X)       (((_V)->stats._X)++)
>
> -#define CSCHED_VCPU_STAT_SET(_V, _X, _Y)    (((_V)->stats._X) = (_Y))
> +#define SCHED_VCPU_STAT_SET(_V, _X, _Y)     (((_V)->stats._X) = (_Y))
>
> -#else /* CSCHED_STATS */
> +#else /* !SCHED_STATS */
>
> -#define CSCHED_VCPU_STATS_RESET(_V)         do {} while ( 0 )
> -#define CSCHED_VCPU_STAT_CRANK(_V, _X)      do {} while ( 0 )
> -#define CSCHED_VCPU_STAT_SET(_V, _X, _Y)    do {} while ( 0 )
> +#undef CSCHED_STATS
>
> -#endif /* CSCHED_STATS */
> +#define SCHED_VCPU_STATS_RESET(_V)         do {} while ( 0 )
> +#define SCHED_VCPU_STAT_CRANK(_V, _X)      do {} while ( 0 )
> +#define SCHED_VCPU_STAT_SET(_V, _X, _Y)    do {} while ( 0 )
> +
> +#endif /* SCHED_STATS */
>
>
>   /*
> @@ -264,13 +258,13 @@ static inline void
>       if ( new->pri > cur->pri )
>       {
>           if ( cur->pri == CSCHED_PRI_IDLE )
> -            CSCHED_STAT_CRANK(tickle_local_idler);
> +            SCHED_STAT_CRANK(tickle_local_idler);
>           else if ( cur->pri == CSCHED_PRI_TS_OVER )
> -            CSCHED_STAT_CRANK(tickle_local_over);
> +            SCHED_STAT_CRANK(tickle_local_over);
>           else if ( cur->pri == CSCHED_PRI_TS_UNDER )
> -            CSCHED_STAT_CRANK(tickle_local_under);
> +            SCHED_STAT_CRANK(tickle_local_under);
>           else
> -            CSCHED_STAT_CRANK(tickle_local_other);
> +            SCHED_STAT_CRANK(tickle_local_other);
>
>           cpumask_set_cpu(cpu, &mask);
>       }
> @@ -283,7 +277,7 @@ static inline void
>       {
>           if ( cpumask_empty(prv->idlers) )
>           {
> -            CSCHED_STAT_CRANK(tickle_idlers_none);
> +            SCHED_STAT_CRANK(tickle_idlers_none);
>           }
>           else
>           {
> @@ -292,7 +286,7 @@ static inline void
>               cpumask_and(&idle_mask, prv->idlers, new->vcpu->cpu_affinity);
>               if ( !cpumask_empty(&idle_mask) )
>               {
> -                CSCHED_STAT_CRANK(tickle_idlers_some);
> +                SCHED_STAT_CRANK(tickle_idlers_some);
>                   if ( opt_tickle_one_idle )
>                   {
>                       this_cpu(last_tickle_cpu) =
> @@ -404,7 +398,7 @@ static inline void
>           BUG_ON( !is_idle_vcpu(vc) );
>       }
>
> -    CSCHED_STAT_CRANK(vcpu_check);
> +    SCHED_STAT_CRANK(vcpu_check);
>   }
>   #define CSCHED_VCPU_CHECK(_vc)  (__csched_vcpu_check(_vc))
>   #else
> @@ -437,7 +431,7 @@ static inline int
>                  ((uint64_t)vcpu_migration_delay * 1000u));
>
>       if ( hot )
> -        CSCHED_STAT_CRANK(vcpu_hot);
> +        SCHED_STAT_CRANK(vcpu_hot);
>
>       return hot;
>   }
> @@ -559,8 +553,8 @@ static inline void
>
>       if ( list_empty(&svc->active_vcpu_elem) )
>       {
> -        CSCHED_VCPU_STAT_CRANK(svc, state_active);
> -        CSCHED_STAT_CRANK(acct_vcpu_active);
> +        SCHED_VCPU_STAT_CRANK(svc, state_active);
> +        SCHED_STAT_CRANK(acct_vcpu_active);
>
>           sdom->active_vcpu_count++;
>           list_add(&svc->active_vcpu_elem, &sdom->active_vcpu);
> @@ -583,8 +577,8 @@ static inline void
>
>       BUG_ON( list_empty(&svc->active_vcpu_elem) );
>
> -    CSCHED_VCPU_STAT_CRANK(svc, state_idle);
> -    CSCHED_STAT_CRANK(acct_vcpu_idle);
> +    SCHED_VCPU_STAT_CRANK(svc, state_idle);
> +    SCHED_STAT_CRANK(acct_vcpu_idle);
>
>       BUG_ON( prv->weight < sdom->weight );
>       sdom->active_vcpu_count--;
> @@ -633,8 +627,8 @@ csched_vcpu_acct(struct csched_private *
>       }
>       else if ( _csched_cpu_pick(ops, current, 0) != cpu )
>       {
> -        CSCHED_VCPU_STAT_CRANK(svc, migrate_r);
> -        CSCHED_STAT_CRANK(migrate_running);
> +        SCHED_VCPU_STAT_CRANK(svc, migrate_r);
> +        SCHED_STAT_CRANK(migrate_running);
>           set_bit(_VPF_migrating, &current->pause_flags);
>           cpu_raise_softirq(cpu, SCHEDULE_SOFTIRQ);
>       }
> @@ -658,8 +652,8 @@ csched_alloc_vdata(const struct schedule
>       svc->flags = 0U;
>       svc->pri = is_idle_domain(vc->domain) ?
>           CSCHED_PRI_IDLE : CSCHED_PRI_TS_UNDER;
> -    CSCHED_VCPU_STATS_RESET(svc);
> -    CSCHED_STAT_CRANK(vcpu_init);
> +    SCHED_VCPU_STATS_RESET(svc);
> +    SCHED_STAT_CRANK(vcpu_init);
>       return svc;
>   }
>
> @@ -690,7 +684,7 @@ csched_vcpu_remove(const struct schedule
>       struct csched_dom * const sdom = svc->sdom;
>       unsigned long flags;
>
> -    CSCHED_STAT_CRANK(vcpu_destroy);
> +    SCHED_STAT_CRANK(vcpu_destroy);
>
>       if ( __vcpu_on_runq(svc) )
>           __runq_remove(svc);
> @@ -711,7 +705,7 @@ csched_vcpu_sleep(const struct scheduler
>   {
>       struct csched_vcpu * const svc = CSCHED_VCPU(vc);
>
> -    CSCHED_STAT_CRANK(vcpu_sleep);
> +    SCHED_STAT_CRANK(vcpu_sleep);
>
>       BUG_ON( is_idle_vcpu(vc) );
>
> @@ -731,19 +725,19 @@ csched_vcpu_wake(const struct scheduler
>
>       if ( unlikely(per_cpu(schedule_data, cpu).curr == vc) )
>       {
> -        CSCHED_STAT_CRANK(vcpu_wake_running);
> +        SCHED_STAT_CRANK(vcpu_wake_running);
>           return;
>       }
>       if ( unlikely(__vcpu_on_runq(svc)) )
>       {
> -        CSCHED_STAT_CRANK(vcpu_wake_onrunq);
> +        SCHED_STAT_CRANK(vcpu_wake_onrunq);
>           return;
>       }
>
>       if ( likely(vcpu_runnable(vc)) )
> -        CSCHED_STAT_CRANK(vcpu_wake_runnable);
> +        SCHED_STAT_CRANK(vcpu_wake_runnable);
>       else
> -        CSCHED_STAT_CRANK(vcpu_wake_not_runnable);
> +        SCHED_STAT_CRANK(vcpu_wake_not_runnable);
>
>       /*
>        * We temporarly boost the priority of awaking VCPUs!
> @@ -883,8 +877,6 @@ csched_dom_init(const struct scheduler *
>   {
>       struct csched_dom *sdom;
>
> -    CSCHED_STAT_CRANK(dom_init);
> -
>       if ( is_idle_domain(dom) )
>           return 0;
>
> @@ -906,7 +898,6 @@ csched_free_domdata(const struct schedul
>   static void
>   csched_dom_destroy(const struct scheduler *ops, struct domain *dom)
>   {
> -    CSCHED_STAT_CRANK(dom_destroy);
>       csched_free_domdata(ops, CSCHED_DOM(dom));
>   }
>
> @@ -989,18 +980,18 @@ csched_acct(void* dummy)
>       if ( prv->credit_balance < 0 )
>       {
>           credit_total -= prv->credit_balance;
> -        CSCHED_STAT_CRANK(acct_balance);
> +        SCHED_STAT_CRANK(acct_balance);
>       }
>
>       if ( unlikely(weight_total == 0) )
>       {
>           prv->credit_balance = 0;
>           spin_unlock_irqrestore(&prv->lock, flags);
> -        CSCHED_STAT_CRANK(acct_no_work);
> +        SCHED_STAT_CRANK(acct_no_work);
>           goto out;
>       }
>
> -    CSCHED_STAT_CRANK(acct_run);
> +    SCHED_STAT_CRANK(acct_run);
>
>       weight_left = weight_total;
>       credit_balance = 0;
> @@ -1075,7 +1066,7 @@ csched_acct(void* dummy)
>                    * the queue to give others a chance at them in future
>                    * accounting periods.
>                    */
> -                CSCHED_STAT_CRANK(acct_reorder);
> +                SCHED_STAT_CRANK(acct_reorder);
>                   list_del(&sdom->active_sdom_elem);
>                   list_add(&sdom->active_sdom_elem, &prv->active_sdom);
>               }
> @@ -1110,7 +1101,7 @@ csched_acct(void* dummy)
>                        credit < -credit_cap &&
>                        !(svc->flags & CSCHED_FLAG_VCPU_PARKED) )
>                   {
> -                    CSCHED_STAT_CRANK(vcpu_park);
> +                    SCHED_STAT_CRANK(vcpu_park);
>                       vcpu_pause_nosync(svc->vcpu);
>                       svc->flags |= CSCHED_FLAG_VCPU_PARKED;
>                   }
> @@ -1118,7 +1109,7 @@ csched_acct(void* dummy)
>                   /* Lower bound on credits */
>                   if ( credit < -prv->credits_per_tslice )
>                   {
> -                    CSCHED_STAT_CRANK(acct_min_credit);
> +                    SCHED_STAT_CRANK(acct_min_credit);
>                       credit = -prv->credits_per_tslice;
>                       atomic_set(&svc->credit, credit);
>                   }
> @@ -1135,7 +1126,7 @@ csched_acct(void* dummy)
>                        * call to make sure the VCPU's priority is not boosted
>                        * if it is woken up here.
>                        */
> -                    CSCHED_STAT_CRANK(vcpu_unpark);
> +                    SCHED_STAT_CRANK(vcpu_unpark);
>                       vcpu_unpause(svc->vcpu);
>                       svc->flags &= ~CSCHED_FLAG_VCPU_PARKED;
>                   }
> @@ -1151,8 +1142,8 @@ csched_acct(void* dummy)
>                   }
>               }
>
> -            CSCHED_VCPU_STAT_SET(svc, credit_last, credit);
> -            CSCHED_VCPU_STAT_SET(svc, credit_incr, credit_fair);
> +            SCHED_VCPU_STAT_SET(svc, credit_last, credit);
> +            SCHED_VCPU_STAT_SET(svc, credit_incr, credit_fair);
>               credit_balance += credit;
>           }
>       }
> @@ -1229,8 +1220,8 @@ csched_runq_steal(int peer_cpu, int cpu,
>               if (__csched_vcpu_is_migrateable(vc, cpu))
>               {
>                   /* We got a candidate. Grab it! */
> -                CSCHED_VCPU_STAT_CRANK(speer, migrate_q);
> -                CSCHED_STAT_CRANK(migrate_queued);
> +                SCHED_VCPU_STAT_CRANK(speer, migrate_q);
> +                SCHED_STAT_CRANK(migrate_queued);
>                   WARN_ON(vc->is_urgent);
>                   __runq_remove(speer);
>                   vc->processor = cpu;
> @@ -1239,7 +1230,7 @@ csched_runq_steal(int peer_cpu, int cpu,
>           }
>       }
>
> -    CSCHED_STAT_CRANK(steal_peer_idle);
> +    SCHED_STAT_CRANK(steal_peer_idle);
>       return NULL;
>   }
>
> @@ -1260,11 +1251,11 @@ csched_load_balance(struct csched_privat
>           goto out;
>
>       if ( snext->pri == CSCHED_PRI_IDLE )
> -        CSCHED_STAT_CRANK(load_balance_idle);
> +        SCHED_STAT_CRANK(load_balance_idle);
>       else if ( snext->pri == CSCHED_PRI_TS_OVER )
> -        CSCHED_STAT_CRANK(load_balance_over);
> +        SCHED_STAT_CRANK(load_balance_over);
>       else
> -        CSCHED_STAT_CRANK(load_balance_other);
> +        SCHED_STAT_CRANK(load_balance_other);
>
>       /*
>        * Peek at non-idling CPUs in the system, starting with our
> @@ -1288,7 +1279,7 @@ csched_load_balance(struct csched_privat
>            */
>           if ( !pcpu_schedule_trylock(peer_cpu) )
>           {
> -            CSCHED_STAT_CRANK(steal_trylock_failed);
> +            SCHED_STAT_CRANK(steal_trylock_failed);
>               continue;
>           }
>
> @@ -1327,7 +1318,7 @@ csched_schedule(
>       struct task_slice ret;
>       s_time_t runtime, tslice;
>
> -    CSCHED_STAT_CRANK(schedule);
> +    SCHED_STAT_CRANK(schedule);
>       CSCHED_VCPU_CHECK(current);
>
>       runtime = now - current->runstate.state_entry_time;
> diff --git a/xen/common/schedule.c b/xen/common/schedule.c
> --- a/xen/common/schedule.c
> +++ b/xen/common/schedule.c
> @@ -314,11 +314,14 @@ int sched_init_domain(struct domain *d)
>   {
>       printk("%s: Initializing domain %d\n", __func__, d->domain_id);
>
> +    SCHED_STAT_CRANK(dom_init);
> +
>       return SCHED_OP(DOM2OP(d), init_domain, d);
>   }
>
>   void sched_destroy_domain(struct domain *d)
>   {
> +    SCHED_STAT_CRANK(dom_destroy);
>       SCHED_OP(DOM2OP(d), destroy_domain, d);
>   }
>
> @@ -1086,7 +1089,7 @@ static void schedule(void)
>
>       ASSERT(!in_atomic());
>
> -    perfc_incr(sched_run);
> +    SCHED_STAT_CRANK(sched_run);
>
>       sd = &this_cpu(schedule_data);
>
> @@ -1164,7 +1167,7 @@ static void schedule(void)
>
>       pcpu_schedule_unlock_irq(cpu);
>
> -    perfc_incr(sched_ctx);
> +    SCHED_STAT_CRANK(sched_ctx);
>
>       stop_timer(&prev->periodic_timer);
>
> @@ -1198,7 +1201,7 @@ void context_saved(struct vcpu *prev)
>   static void s_timer_fn(void *unused)
>   {
>       raise_softirq(SCHEDULE_SOFTIRQ);
> -    perfc_incr(sched_irq);
> +    SCHED_STAT_CRANK(sched_irq);
>   }
>
>   /* Per-VCPU periodic timer function: sends a virtual timer interrupt. */
> diff --git a/xen/include/xen/perfc_defn.h b/xen/include/xen/perfc_defn.h
> --- a/xen/include/xen/perfc_defn.h
> +++ b/xen/include/xen/perfc_defn.h
> @@ -12,13 +12,19 @@ PERFCOUNTER(calls_from_multicall,
>   PERFCOUNTER(irqs,                   "#interrupts")
>   PERFCOUNTER(ipis,                   "#IPIs")
>
> +/* Generic scheduler counters (applicable to all schedulers) */
>   PERFCOUNTER(sched_irq,              "sched: timer")
>   PERFCOUNTER(sched_run,              "sched: runs through scheduler")
>   PERFCOUNTER(sched_ctx,              "sched: context switches")
> +PERFCOUNTER(schedule,               "sched: specific scheduler")
> +PERFCOUNTER(dom_init,               "sched: dom_init")
> +PERFCOUNTER(dom_destroy,            "sched: dom_destroy")
> +PERFCOUNTER(vcpu_init,              "sched: vcpu_init")
> +PERFCOUNTER(vcpu_destroy,           "sched: vcpu_destroy")
>
> +/* credit specific counters */
>   PERFCOUNTER(delay_ms,               "csched: delay")
>   PERFCOUNTER(vcpu_check,             "csched: vcpu_check")
> -PERFCOUNTER(schedule,               "csched: schedule")
>   PERFCOUNTER(acct_run,               "csched: acct_run")
>   PERFCOUNTER(acct_no_work,           "csched: acct_no_work")
>   PERFCOUNTER(acct_balance,           "csched: acct_balance")
> @@ -46,10 +52,6 @@ PERFCOUNTER(steal_trylock_failed,   "csc
>   PERFCOUNTER(steal_peer_idle,        "csched: steal_peer_idle")
>   PERFCOUNTER(migrate_queued,         "csched: migrate_queued")
>   PERFCOUNTER(migrate_running,        "csched: migrate_running")
> -PERFCOUNTER(dom_init,               "csched: dom_init")
> -PERFCOUNTER(dom_destroy,            "csched: dom_destroy")
> -PERFCOUNTER(vcpu_init,              "csched: vcpu_init")
> -PERFCOUNTER(vcpu_destroy,           "csched: vcpu_destroy")
>   PERFCOUNTER(vcpu_hot,               "csched: vcpu_hot")
>
>   PERFCOUNTER(need_flush_tlb_flush,   "PG_need_flush tlb flushes")
> diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -16,6 +16,7 @@
>   #include <xen/tasklet.h>
>   #include <xen/mm.h>
>   #include <xen/smp.h>
> +#include <xen/perfc.h>
>   #include <asm/atomic.h>
>   #include <xen/wait.h>
>   #include <public/xen.h>
> @@ -29,6 +30,18 @@
>   DEFINE_XEN_GUEST_HANDLE(vcpu_runstate_info_compat_t);
>   #endif
>
> +/*
> + * Stats
> + *
> + * Enable and ease the use of scheduling related performance counters.
> + *
> + */
> +#ifdef PERF_COUNTERS
> +#define SCHED_STATS
> +#endif
> +
> +#define SCHED_STAT_CRANK(_X)                (perfc_incr(_X))
> +
>   /* A global pointer to the initial domain (DOM0). */
>   extern struct domain *dom0;
>

Re: [PATCH 4 of 6] xen: sched: introduce a couple of counters in credit2 and SEDF

[George Dunlap]

On 22/10/12 15:40, Dario Faggioli wrote:
> Mainly for consistency with credit, at least for the events that are
> general enough, like vCPU initialization/destruction and calls
> to the specific scheduling function.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH 6 of 6] xen: sched_sedf: remove an unused stat in SEDF

[George Dunlap]

On 22/10/12 15:40, Dario Faggioli wrote:
> Namely, `short_cont' which is not updated anywhere in the code.
>
> Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
Acked-by: George Dunlap <george.dunlap@eu.citrix.com>

Re: [PATCH 5 of 6] xen: sched_sedf: beautify statisics in SEDF

[Dario Faggioli]

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

On Tue, 2012-10-23 at 17:26 +0100, George Dunlap wrote:
> On 22/10/12 15:40, Dario Faggioli wrote:
> > By gathering all the related fields in a struct (as it is being done
> > in credit) and using the macros we now have available. No functional
> > changes involved.
> >
> > Signed-off-by: Dario Faggioli <dario.faggioli@citrix.com>
> 
> I'm OK with this as it is, but...
> 
Ok.

> >   #ifdef SEDF_STATS
> > -    if ( EDOM_INFO(d)->block_time_tot != 0 )
> > -        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->penalty_time_tot * 100) /
> > -               EDOM_INFO(d)->block_time_tot);
> > -    if ( EDOM_INFO(d)->block_tot != 0 )
> > +    if ( EDOM_INFO(d)->stats.block_time_tot != 0 )
> > +        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->stats.penalty_time_tot * 100) /
> > +               EDOM_INFO(d)->stats.block_time_tot);
> > +    if ( EDOM_INFO(d)->stats.block_tot != 0 )
> >           printk("\n   blks=%u sh=%u (%u%%) (shc=%u (%u%%) shex=%i "\
> >                  "shexsl=%i) l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
> > -               EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_block_tot,
> > -               (EDOM_INFO(d)->short_block_tot * 100)
> > -               / EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_cont,
> > -               (EDOM_INFO(d)->short_cont * 100) / EDOM_INFO(d)->block_tot,
> > -               EDOM_INFO(d)->pen_extra_blocks,
> > -               EDOM_INFO(d)->pen_extra_slices,
> > -               EDOM_INFO(d)->long_block_tot,
> > -               (EDOM_INFO(d)->long_block_tot * 100) / EDOM_INFO(d)->block_tot,
> > -               (EDOM_INFO(d)->block_time_tot) / EDOM_INFO(d)->block_tot,
> > -               (EDOM_INFO(d)->penalty_time_tot) / EDOM_INFO(d)->block_tot);
> > +               EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_block_tot,
> > +               (EDOM_INFO(d)->stats.short_block_tot * 100)
> > +               / EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_cont,
> > +               (EDOM_INFO(d)->stats.short_cont * 100) / EDOM_INFO(d)->stats.block_tot,
> > +               EDOM_INFO(d)->stats.pen_extra_blocks,
> > +               EDOM_INFO(d)->stats.pen_extra_slices,
> > +               EDOM_INFO(d)->stats.long_block_tot,
> > +               (EDOM_INFO(d)->stats.long_block_tot * 100) / EDOM_INFO(d)->stats.block_tot,
> > +               (EDOM_INFO(d)->stats.block_time_tot) / EDOM_INFO(d)->stats.block_tot,
> > +               (EDOM_INFO(d)->stats.penalty_time_tot) / EDOM_INFO(d)->stats.block_tot);
> 
> ...wouldn't it be even more beautiful to have a macro for reading stats 
> as well?
> 
> Like I said, it's fine as it is, but since you're looking for beauty, I 
> figured I'd point it out. :-)
> 
I see what you mean. Again, as you wish.

This code will need some (and quite a bit actually) of attention as soon
as I or someone else get the time to work on it. If you're fine about
taking this as is, I'll make a note to self about the macro (as I agree
it would be nice).

OTOH, if you prefer me to repost the patch, I think I can find 5 mins to
hack it up...

Thanks and Regards,
Dario

-- 
<<This happens because I choose it to happen!>> (Raistlin Majere)
-----------------------------------------------------------------
Dario Faggioli, Ph.D, http://retis.sssup.it/people/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: 198 bytes --]

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

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

Re: [PATCH 5 of 6] xen: sched_sedf: beautify statisics in SEDF

[George Dunlap]

On 23/10/12 17:33, Dario Faggioli wrote:
>>>    #ifdef SEDF_STATS
>>> -    if ( EDOM_INFO(d)->block_time_tot != 0 )
>>> -        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->penalty_time_tot * 100) /
>>> -               EDOM_INFO(d)->block_time_tot);
>>> -    if ( EDOM_INFO(d)->block_tot != 0 )
>>> +    if ( EDOM_INFO(d)->stats.block_time_tot != 0 )
>>> +        printk(" pen=%"PRIu64"%%", (EDOM_INFO(d)->stats.penalty_time_tot * 100) /
>>> +               EDOM_INFO(d)->stats.block_time_tot);
>>> +    if ( EDOM_INFO(d)->stats.block_tot != 0 )
>>>            printk("\n   blks=%u sh=%u (%u%%) (shc=%u (%u%%) shex=%i "\
>>>                   "shexsl=%i) l=%u (%u%%) avg: b=%"PRIu64" p=%"PRIu64"",
>>> -               EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_block_tot,
>>> -               (EDOM_INFO(d)->short_block_tot * 100)
>>> -               / EDOM_INFO(d)->block_tot, EDOM_INFO(d)->short_cont,
>>> -               (EDOM_INFO(d)->short_cont * 100) / EDOM_INFO(d)->block_tot,
>>> -               EDOM_INFO(d)->pen_extra_blocks,
>>> -               EDOM_INFO(d)->pen_extra_slices,
>>> -               EDOM_INFO(d)->long_block_tot,
>>> -               (EDOM_INFO(d)->long_block_tot * 100) / EDOM_INFO(d)->block_tot,
>>> -               (EDOM_INFO(d)->block_time_tot) / EDOM_INFO(d)->block_tot,
>>> -               (EDOM_INFO(d)->penalty_time_tot) / EDOM_INFO(d)->block_tot);
>>> +               EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_block_tot,
>>> +               (EDOM_INFO(d)->stats.short_block_tot * 100)
>>> +               / EDOM_INFO(d)->stats.block_tot, EDOM_INFO(d)->stats.short_cont,
>>> +               (EDOM_INFO(d)->stats.short_cont * 100) / EDOM_INFO(d)->stats.block_tot,
>>> +               EDOM_INFO(d)->stats.pen_extra_blocks,
>>> +               EDOM_INFO(d)->stats.pen_extra_slices,
>>> +               EDOM_INFO(d)->stats.long_block_tot,
>>> +               (EDOM_INFO(d)->stats.long_block_tot * 100) / EDOM_INFO(d)->stats.block_tot,
>>> +               (EDOM_INFO(d)->stats.block_time_tot) / EDOM_INFO(d)->stats.block_tot,
>>> +               (EDOM_INFO(d)->stats.penalty_time_tot) / EDOM_INFO(d)->stats.block_tot);
>> ...wouldn't it be even more beautiful to have a macro for reading stats
>> as well?
>>
>> Like I said, it's fine as it is, but since you're looking for beauty, I
>> figured I'd point it out. :-)
>>
> I see what you mean. Again, as you wish.
>
> This code will need some (and quite a bit actually) of attention as soon
> as I or someone else get the time to work on it. If you're fine about
> taking this as is, I'll make a note to self about the macro (as I agree
> it would be nice).

OK -- in that case:

Acked-by: George Dunlap <george.dunlap@eu.citrix.com>