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

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

[Jan Beulich]

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 0/2] core-parking: SMT-disable and section adjustments

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[Jan Beulich]

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

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

[Jan Beulich]

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: [PATCH 2/2] core-parking: adjust data/code placement

[Andrew Cooper]

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

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

[Andrew Cooper]

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

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

[Andrew Cooper]

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

[Andrew Cooper]

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: [PATCH 1/2] core-parking: interact with runtime SMT-disabling

[Jan Beulich]

>>> 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

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

[Jan Beulich]

>>> 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

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

[Julien Grall]

Hi Jan,

On 11/04/2019 13:45, Jan Beulich wrote:
> --- a/xen/common/core_parking.c
> +++ b/xen/common/core_parking.c


[...]

> +bool core_parking_remove(unsigned int cpu)

Something looks wrong. This function is implemented in common code but...

> +{
> +    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);

The prototype is declared in asm-x86/smp.h. Actually, it seems all the function 
exported in core_parking.c have their prototype declared in asm-x86/smp.h.

This raises the question of whether it makes sense to have core_parking.c in 
common. If it makes sense, then the prototype should be declared in include/xen.

Cheers,

-- 
Julien Grall

_______________________________________________
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

[Julien Grall]

Hi Jan,

On 11/04/2019 13:45, Jan Beulich wrote:
> --- a/xen/common/core_parking.c
> +++ b/xen/common/core_parking.c


[...]

> +bool core_parking_remove(unsigned int cpu)

Something looks wrong. This function is implemented in common code but...

> +{
> +    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);

The prototype is declared in asm-x86/smp.h. Actually, it seems all the function 
exported in core_parking.c have their prototype declared in asm-x86/smp.h.

This raises the question of whether it makes sense to have core_parking.c in 
common. If it makes sense, then the prototype should be declared in include/xen.

Cheers,

-- 
Julien Grall

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

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

[Jan Beulich]

>>> On 24.04.19 at 13:51, <julien.grall@arm.com> wrote:
> On 11/04/2019 13:45, Jan Beulich wrote:
>> --- a/xen/common/core_parking.c
>> +++ b/xen/common/core_parking.c
> 
> 
> [...]
> 
>> +bool core_parking_remove(unsigned int cpu)
> 
> Something looks wrong. This function is implemented in common code but...
> 
>> +{
>> +    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);
> 
> The prototype is declared in asm-x86/smp.h. Actually, it seems all the function 
> exported in core_parking.c have their prototype declared in asm-x86/smp.h.

I've noticed this too, but this is not an uncommon thing (hence
the CORE_PARKING Kconfig setting). Various such instance
have been eliminated since the introduction of Arm support,
but others remain. It's not the purpose of this series to clean
up this aspect, or ...

> This raises the question of whether it makes sense to have core_parking.c in 
> common. If it makes sense, then the prototype should be declared in 
> include/xen.

... to decide whether the .c file or the declarations should move.
Personally I think this is generic enough a concept that the .c
should remain where it is, in which case we'd likely want to gain
a xen/core-parking.h at some point.

Jan



_______________________________________________
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

[Jan Beulich]

>>> On 24.04.19 at 13:51, <julien.grall@arm.com> wrote:
> On 11/04/2019 13:45, Jan Beulich wrote:
>> --- a/xen/common/core_parking.c
>> +++ b/xen/common/core_parking.c
> 
> 
> [...]
> 
>> +bool core_parking_remove(unsigned int cpu)
> 
> Something looks wrong. This function is implemented in common code but...
> 
>> +{
>> +    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);
> 
> The prototype is declared in asm-x86/smp.h. Actually, it seems all the function 
> exported in core_parking.c have their prototype declared in asm-x86/smp.h.

I've noticed this too, but this is not an uncommon thing (hence
the CORE_PARKING Kconfig setting). Various such instance
have been eliminated since the introduction of Arm support,
but others remain. It's not the purpose of this series to clean
up this aspect, or ...

> This raises the question of whether it makes sense to have core_parking.c in 
> common. If it makes sense, then the prototype should be declared in 
> include/xen.

... to decide whether the .c file or the declarations should move.
Personally I think this is generic enough a concept that the .c
should remain where it is, in which case we'd likely want to gain
a xen/core-parking.h at some point.

Jan



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

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

[Jan Beulich]

>>> On 12.04.19 at 13:41,  wrote:
>>>> 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.

Did my responses address your concerns?

Jan



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

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

[Jan Beulich]

>>> On 12.04.19 at 13:41,  wrote:
>>>> 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.

Did my responses address your concerns?

Jan



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