[RESEND/PING] core-parking: interact with runtime SMT-disabling

Commit Message

Jan Beulich Aug. 30, 2019, 1:33 p.m. UTC

When disabling SMT at runtime, secondary threads should no longer be
candidates for bringing back up in response to _PUD ACPI events. Purge
them from the tracking array.

Doing so involves adding locking to guard accounting data in the core
parking code. While adding the declaration for the lock, take the
liberty to drop two unnecessary forward function declarations.

Signed-off-by: Jan Beulich <[e-mail]>

Comments

Andrew Cooper Sept. 2, 2019, 10:37 a.m. UTC | #1

On 30/08/2019 14:33, Jan Beulich wrote:
> When disabling SMT at runtime, secondary threads should no longer be
> candidates for bringing back up in response to _PUD ACPI events. Purge
> them from the tracking array.

I think I agree in principle, but what are _PUD events?  I can't find
any reference to them at all in the ACPI spec.

~Andrew

Jan Beulich Sept. 2, 2019, 10:46 a.m. UTC | #2

On 02.09.2019 12:37, Andrew Cooper wrote:
> On 30/08/2019 14:33, Jan Beulich wrote:
>> When disabling SMT at runtime, secondary threads should no longer be
>> candidates for bringing back up in response to _PUD ACPI events. Purge
>> them from the tracking array.
> 
> I think I agree in principle, but what are _PUD events?  I can't find
> any reference to them at all in the ACPI spec.

Oops - typo: _PUR is the correct name. I'm sorry.

Jan

Andrew Cooper Sept. 17, 2019, 6:37 p.m. UTC | #3

On 02/09/2019 11:46, Jan Beulich wrote:
> On 02.09.2019 12:37, Andrew Cooper wrote:
>> On 30/08/2019 14:33, Jan Beulich wrote:
>>> When disabling SMT at runtime, secondary threads should no longer be
>>> candidates for bringing back up in response to _PUD ACPI events. Purge
>>> them from the tracking array.
>> I think I agree in principle, but what are _PUD events?  I can't find
>> any reference to them at all in the ACPI spec.
> Oops - typo: _PUR is the correct name. I'm sorry.

With this fixed, Acked-by: Andrew Cooper <[e-mail]>

Patch

--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -128,6 +128,9 @@  static long smt_up_down_helper(void *dat
         if ( !(x86_cpu_to_apicid[cpu] & sibling_mask) )
             continue;
 
+        if ( !up && core_parking_remove(cpu) )
+            continue;
+
         ret = up ? cpu_up_helper(_p(cpu))
                  : cpu_down_helper(_p(cpu));
 
--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -25,9 +25,7 @@ 
 #define CORE_PARKING_INCREMENT 1
 #define CORE_PARKING_DECREMENT 2
 
-static unsigned int core_parking_power(unsigned int event);
-static unsigned int core_parking_performance(unsigned int event);
-
+static DEFINE_SPINLOCK(accounting_lock);
 static uint32_t cur_idle_nums;
 static unsigned int core_parking_cpunum[NR_CPUS] = {[0 ... NR_CPUS-1] = -1};
 
@@ -100,10 +98,10 @@  static unsigned int core_parking_perform
     break;
 
     case CORE_PARKING_DECREMENT:
-    {
-        cpu = core_parking_cpunum[cur_idle_nums -1];
-    }
-    break;
+        spin_lock(&accounting_lock);
+        cpu = core_parking_cpunum[cur_idle_nums - 1];
+        spin_unlock(&accounting_lock);
+        break;
 
     default:
         break;
@@ -158,10 +156,10 @@  static unsigned int core_parking_power(u
     break;
 
     case CORE_PARKING_DECREMENT:
-    {
-        cpu = core_parking_cpunum[cur_idle_nums -1];
-    }
-    break;
+        spin_lock(&accounting_lock);
+        cpu = core_parking_cpunum[cur_idle_nums - 1];
+        spin_unlock(&accounting_lock);
+        break;
 
     default:
         break;
@@ -185,7 +183,11 @@  long core_parking_helper(void *data)
         ret = cpu_down(cpu);
         if ( ret )
             return ret;
+
+        spin_lock(&accounting_lock);
+        BUG_ON(cur_idle_nums >= ARRAY_SIZE(core_parking_cpunum));
         core_parking_cpunum[cur_idle_nums++] = cpu;
+        spin_unlock(&accounting_lock);
     }
 
     while ( cur_idle_nums > idle_nums )
@@ -194,12 +196,43 @@  long core_parking_helper(void *data)
         ret = cpu_up(cpu);
         if ( ret )
             return ret;
-        core_parking_cpunum[--cur_idle_nums] = -1;
+
+        if ( !core_parking_remove(cpu) )
+        {
+            ret = cpu_down(cpu);
+            if ( ret == -EEXIST )
+                ret = 0;
+            if ( ret )
+                break;
+        }
     }
 
     return ret;
 }
 
+bool core_parking_remove(unsigned int cpu)
+{
+    unsigned int i;
+    bool found = false;
+
+    spin_lock(&accounting_lock);
+
+    for ( i = 0; i < cur_idle_nums; ++i )
+        if ( core_parking_cpunum[i] == cpu )
+        {
+            found = true;
+            --cur_idle_nums;
+            break;
+        }
+
+    for ( ; i < cur_idle_nums; ++i )
+        core_parking_cpunum[i] = core_parking_cpunum[i + 1];
+
+    spin_unlock(&accounting_lock);
+
+    return found;
+}
+
 uint32_t get_cur_idle_nums(void)
 {
     return cur_idle_nums;
--- a/xen/include/asm-x86/smp.h
+++ b/xen/include/asm-x86/smp.h
@@ -63,6 +63,7 @@  long cpu_up_helper(void *data);
 long cpu_down_helper(void *data);
 
 long core_parking_helper(void *data);
+bool core_parking_remove(unsigned int cpu);
 uint32_t get_cur_idle_nums(void);
 
 /*