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
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
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
>>> 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
>>> 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
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
>>> 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
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
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
© 2016 - 2024 Red Hat, Inc.