[Xen-devel] [PATCH 0/2] core-parking: SMT-disable and section adjustments

Jan Beulich posted 2 patches 1 week ago
Only 0 patches received!

[Xen-devel] [PATCH 0/2] core-parking: SMT-disable and section adjustments

Posted by Jan Beulich 1 week ago
1: interact with runtime SMT-disabling
2: adjust data/code placement

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 1/2] core-parking: interact with runtime SMT-disabling

Posted by Jan Beulich 1 week ago
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 <jbeulich@suse.com>

--- 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);
 
 /*





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] core-parking: interact with runtime SMT-disabling

Posted by Andrew Cooper 1 week ago
On 11/04/2019 13:45, 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.
>
> 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 <jbeulich@suse.com>

I can certainly appreciate these arguments, but surely the converse is
true.  When SMT-enable is used, the newly-onlined threads are now
eligible to be parked.

At the moment, this looks asymetric.

~Andrew

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 1/2] core-parking: interact with runtime SMT-disabling

Posted by Jan Beulich 1 week ago
>>> On 11.04.19 at 21:06, <andrew.cooper3@citrix.com> wrote:
> On 11/04/2019 13:45, 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.
>>
>> 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 <jbeulich@suse.com>
> 
> I can certainly appreciate these arguments, but surely the converse is
> true.  When SMT-enable is used, the newly-onlined threads are now
> eligible to be parked.

And nothing will keep them from getting parked.

> At the moment, this looks asymetric.

It does, but that's a result of core_parking.c only recording CPUs
it has parked, not ones it could park.

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH 2/2] core-parking: adjust data/code placement

Posted by Jan Beulich 1 week ago
Use __init{data,}, __read_mostly, and const as far as possible.

Take the liberty and also shorten the policy structure's tag name.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/common/core_parking.c
+++ b/xen/common/core_parking.c
@@ -29,15 +29,15 @@ static DEFINE_SPINLOCK(accounting_lock);
 static uint32_t cur_idle_nums;
 static unsigned int core_parking_cpunum[NR_CPUS] = {[0 ... NR_CPUS-1] = -1};
 
-static struct core_parking_policy {
+static const struct cp_policy {
     char name[30];
     unsigned int (*next)(unsigned int event);
-} *core_parking_policy;
+} *__read_mostly core_parking_policy;
 
 static enum core_parking_controller {
     POWER_FIRST,
     PERFORMANCE_FIRST
-} core_parking_controller = POWER_FIRST;
+} core_parking_controller __initdata = POWER_FIRST;
 
 static int __init setup_core_parking_option(const char *str)
 {
@@ -238,17 +238,17 @@ uint32_t get_cur_idle_nums(void)
     return cur_idle_nums;
 }
 
-static struct core_parking_policy power_first = {
+static const struct cp_policy power_first = {
     .name = "power",
     .next = core_parking_power,
 };
 
-static struct core_parking_policy performance_first = {
+static const struct cp_policy performance_first = {
     .name = "performance",
     .next = core_parking_performance,
 };
 
-static int register_core_parking_policy(struct core_parking_policy *policy)
+static int __init register_core_parking_policy(const struct cp_policy *policy)
 {
     if ( !policy || !policy->next )
         return -EINVAL;





_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH 2/2] core-parking: adjust data/code placement

Posted by Andrew Cooper 1 week ago
On 11/04/2019 13:45, Jan Beulich wrote:
> Use __init{data,}, __read_mostly, and const as far as possible.
>
> Take the liberty and also shorten the policy structure's tag name.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel