Re: Re: [PATCH] CPUIDLE: shorten hpet spin_lock holding time

[Keir Fraser <keir.fraser@eu.citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 955 bytes --]

On 22/04/2010 08:22, "Keir Fraser" <keir.fraser@eu.citrix.com> wrote:

>> If a CPU cleared itself from cpuidle_mwait_flags, then this CPU didn't need a
>> IPI to be waken. And one useless write to softirq_pending doesn't have any
>> side effect. So this case should be acceptable.
> 
> That's not totally convincing. The write to softirq_pending has one extra
> side effect: it is possible that the next time TIMER_SOFTIRQ really needs to
> be raised on that CPU, it will not receive notification via IPI, because the
> flag is already set in its softirq_pending mask.
> 
> Hm, let me see if I can come up with a patch for this and post it for you.

How about the attached patch? It MWAITs on a completely new flag field,
avoiding the IPI-avoidance semantics of softirq_pending. It also folds in
your patch. It also does wakeup-waiting checks on timer_deadline_start, that
being the field that initiates wakeup via the MONITORed memory region.

 -- Keir


[-- Attachment #2: 00-mwait --]
[-- Type: application/octet-stream, Size: 1941 bytes --]

diff -r b0562b298d73 xen/arch/x86/acpi/cpu_idle.c
--- a/xen/arch/x86/acpi/cpu_idle.c	Wed Apr 21 12:51:53 2010 +0100
+++ b/xen/arch/x86/acpi/cpu_idle.c	Thu Apr 22 09:17:36 2010 +0100
@@ -153,40 +153,45 @@
 /*
  * The bit is set iff cpu use monitor/mwait to enter C state
  * with this flag set, CPU can be waken up from C state
- * by writing to specific memory address, instead of sending IPI
+ * by writing to specific memory address, instead of sending an IPI.
  */
 static cpumask_t cpuidle_mwait_flags;
+static DEFINE_PER_CPU(bool_t, cpuidle_mwait_wakeup);
 
 void cpuidle_wakeup_mwait(cpumask_t *mask)
 {
     cpumask_t target;
-    int cpu;
+    unsigned int cpu;
 
     cpus_and(target, *mask, cpuidle_mwait_flags);
 
-    /* cpu is 'mwait'ing at softirq_pending,
-       writing to it will wake up CPU */
+    /* CPU is MWAITing on the cpuidle_mwait_wakeup flag. */
     for_each_cpu_mask(cpu, target)
-        set_bit(TIMER_SOFTIRQ, &softirq_pending(cpu));
+        per_cpu(cpuidle_mwait_wakeup, cpu) = 0;
 
-    cpus_andnot(*mask, *mask, cpuidle_mwait_flags);
+    cpus_andnot(*mask, *mask, target);
 }
 
 static void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
 {
     int cpu = smp_processor_id();
 
-    __monitor((void *)&softirq_pending(cpu), 0, 0);
+    __monitor((void *)&this_cpu(cpuidle_mwait_wakeup), 0, 0);
+    smp_mb();
 
-    smp_mb();
-    if (!softirq_pending(cpu))
+    /*
+     * Timer deadline passing is the event on which we will be woken via
+     * cpuidle_mwait_wakeup. So check it now that the location is armed.
+     */
+    if ( per_cpu(timer_deadline_start, cpu) > NOW() )
     {
         cpu_set(cpu, cpuidle_mwait_flags);
-
         __mwait(eax, ecx);
-
         cpu_clear(cpu, cpuidle_mwait_flags);
     }
+
+    if ( per_cpu(timer_deadline_start, cpu) <= NOW() )
+        raise_softirq(TIMER_SOFTIRQ);
 }
 
 static void acpi_processor_ffh_cstate_enter(struct acpi_processor_cx *cx)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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