| Message ID | 1c23930f-e809-d623-18ec-599a0e983b7a@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | core-parking: fix build with gcc12 and NR_CPUS=1 | expand |
Hi Jan, On 09/09/2022 11:18, Jan Beulich wrote: > Gcc12 takes issue with core_parking_remove()'s > > for ( ; i < cur_idle_nums; ++i ) > core_parking_cpunum[i] = core_parking_cpunum[i + 1]; > > complaining that the right hand side array access is past the bounds of > 1. Clearly the compiler can't know that cur_idle_nums can only ever be > zero in this case (as the sole CPU cannot be parked). > > Beyond addressing the immediate issue also adjust core_parking_init(): > There's no point registering any policy when there's no CPU to park. > Since this still doesn't result in the compiler spotting that > core_parking_policy is never written (and hence is continuously NULL), > also amend core_parking_helper() to avoid eventual similar issues there > (minimizing generated code at the same time). Given that CONFIG_NR_CPUS is a build time option. Wouldn't it be better to set CONFIG_CORE_PARKING=n and provide dummy helper for any function that may be called unconditionally? Cheers,
On 09.09.2022 13:00, Julien Grall wrote: > On 09/09/2022 11:18, Jan Beulich wrote: >> Gcc12 takes issue with core_parking_remove()'s >> >> for ( ; i < cur_idle_nums; ++i ) >> core_parking_cpunum[i] = core_parking_cpunum[i + 1]; >> >> complaining that the right hand side array access is past the bounds of >> 1. Clearly the compiler can't know that cur_idle_nums can only ever be >> zero in this case (as the sole CPU cannot be parked). >> >> Beyond addressing the immediate issue also adjust core_parking_init(): >> There's no point registering any policy when there's no CPU to park. >> Since this still doesn't result in the compiler spotting that >> core_parking_policy is never written (and hence is continuously NULL), >> also amend core_parking_helper() to avoid eventual similar issues there >> (minimizing generated code at the same time). > > Given that CONFIG_NR_CPUS is a build time option. Wouldn't it be better > to set CONFIG_CORE_PARKING=n and provide dummy helper for any function > that may be called unconditionally? That might be an option, yes; not sure whether that's really better. It's likely a more intrusive change ... Jan
Hi, On 09/09/2022 13:14, Jan Beulich wrote: > On 09.09.2022 13:00, Julien Grall wrote: >> On 09/09/2022 11:18, Jan Beulich wrote: >>> Gcc12 takes issue with core_parking_remove()'s >>> >>> for ( ; i < cur_idle_nums; ++i ) >>> core_parking_cpunum[i] = core_parking_cpunum[i + 1]; >>> >>> complaining that the right hand side array access is past the bounds of >>> 1. Clearly the compiler can't know that cur_idle_nums can only ever be >>> zero in this case (as the sole CPU cannot be parked). >>> >>> Beyond addressing the immediate issue also adjust core_parking_init(): >>> There's no point registering any policy when there's no CPU to park. >>> Since this still doesn't result in the compiler spotting that >>> core_parking_policy is never written (and hence is continuously NULL), >>> also amend core_parking_helper() to avoid eventual similar issues there >>> (minimizing generated code at the same time). >> >> Given that CONFIG_NR_CPUS is a build time option. Wouldn't it be better >> to set CONFIG_CORE_PARKING=n and provide dummy helper for any function >> that may be called unconditionally? > > That might be an option, yes; not sure whether that's really better. It's > likely a more intrusive change ... I quickly try to implement it (see below) and the result is IHMO a lot nicer and make clear the code is not necessary on uni-processor. This is only compile tested. diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 6a7825f4ba3c..f9a3daccdc92 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -10,7 +10,7 @@ config X86 select ALTERNATIVE_CALL select ARCH_MAP_DOMAIN_PAGE select ARCH_SUPPORTS_INT128 - select CORE_PARKING + select CORE_PARKING if NR_CPUS > 1 select HAS_ALTERNATIVE select HAS_COMPAT select HAS_CPUFREQ diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h index 41a3b6a0dadf..7baca00be182 100644 --- a/xen/arch/x86/include/asm/smp.h +++ b/xen/arch/x86/include/asm/smp.h @@ -61,7 +61,15 @@ long cf_check cpu_up_helper(void *data); long cf_check cpu_down_helper(void *data); long cf_check core_parking_helper(void *data); + +#ifdef CONFIG_CORE_PARKING bool core_parking_remove(unsigned int cpu); +#else +static inline bool core_parking_remove(unsigned int cpu) +{ + return false; +} +#endif uint32_t get_cur_idle_nums(void); /* diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c index a7341dc3d7d3..5d13fac41bd4 100644 --- a/xen/arch/x86/platform_hypercall.c +++ b/xen/arch/x86/platform_hypercall.c @@ -718,6 +718,7 @@ ret_t do_platform_op( op->u.mem_add.pxm); break; +#ifdef CONFIG_CORE_PARKING case XENPF_core_parking: { uint32_t idle_nums; @@ -743,6 +744,7 @@ ret_t do_platform_op( } } break; +#endif case XENPF_resource_op: { Cheers,
On 09.09.2022 14:27, Julien Grall wrote: > Hi, > > On 09/09/2022 13:14, Jan Beulich wrote: >> On 09.09.2022 13:00, Julien Grall wrote: >>> On 09/09/2022 11:18, Jan Beulich wrote: >>>> Gcc12 takes issue with core_parking_remove()'s >>>> >>>> for ( ; i < cur_idle_nums; ++i ) >>>> core_parking_cpunum[i] = core_parking_cpunum[i + 1]; >>>> >>>> complaining that the right hand side array access is past the bounds of >>>> 1. Clearly the compiler can't know that cur_idle_nums can only ever be >>>> zero in this case (as the sole CPU cannot be parked). >>>> >>>> Beyond addressing the immediate issue also adjust core_parking_init(): >>>> There's no point registering any policy when there's no CPU to park. >>>> Since this still doesn't result in the compiler spotting that >>>> core_parking_policy is never written (and hence is continuously NULL), >>>> also amend core_parking_helper() to avoid eventual similar issues there >>>> (minimizing generated code at the same time). >>> >>> Given that CONFIG_NR_CPUS is a build time option. Wouldn't it be better >>> to set CONFIG_CORE_PARKING=n and provide dummy helper for any function >>> that may be called unconditionally? >> >> That might be an option, yes; not sure whether that's really better. It's >> likely a more intrusive change ... > > I quickly try to implement it (see below) and the result is IHMO a lot > nicer and make clear the code is not necessary on uni-processor. Hmm, we can do something like this, but ... > This is only compile tested. > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 6a7825f4ba3c..f9a3daccdc92 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -10,7 +10,7 @@ config X86 > select ALTERNATIVE_CALL > select ARCH_MAP_DOMAIN_PAGE > select ARCH_SUPPORTS_INT128 > - select CORE_PARKING > + select CORE_PARKING if NR_CPUS > 1 > select HAS_ALTERNATIVE > select HAS_COMPAT > select HAS_CPUFREQ > diff --git a/xen/arch/x86/include/asm/smp.h b/xen/arch/x86/include/asm/smp.h > index 41a3b6a0dadf..7baca00be182 100644 > --- a/xen/arch/x86/include/asm/smp.h > +++ b/xen/arch/x86/include/asm/smp.h > @@ -61,7 +61,15 @@ long cf_check cpu_up_helper(void *data); > long cf_check cpu_down_helper(void *data); > > long cf_check core_parking_helper(void *data); > + > +#ifdef CONFIG_CORE_PARKING > bool core_parking_remove(unsigned int cpu); > +#else > +static inline bool core_parking_remove(unsigned int cpu) > +{ > + return false; > +} > +#endif > uint32_t get_cur_idle_nums(void); > > /* > diff --git a/xen/arch/x86/platform_hypercall.c > b/xen/arch/x86/platform_hypercall.c > index a7341dc3d7d3..5d13fac41bd4 100644 > --- a/xen/arch/x86/platform_hypercall.c > +++ b/xen/arch/x86/platform_hypercall.c > @@ -718,6 +718,7 @@ ret_t do_platform_op( > op->u.mem_add.pxm); > break; > > +#ifdef CONFIG_CORE_PARKING > case XENPF_core_parking: > { > uint32_t idle_nums; > @@ -743,6 +744,7 @@ ret_t do_platform_op( > } > } > break; > +#endif ... this needs doing differently to prevent the hypercall changing behavior. Will send a v2 in a minute. Jan
--- a/xen/common/core_parking.c +++ b/xen/common/core_parking.c @@ -175,7 +175,7 @@ long cf_check core_parking_helper(void * unsigned int cpu; int ret = 0; - if ( !core_parking_policy ) + if ( !core_parking_policy || CONFIG_NR_CPUS == 1 ) return -EINVAL; while ( cur_idle_nums < idle_nums ) @@ -213,8 +213,9 @@ long cf_check core_parking_helper(void * bool core_parking_remove(unsigned int cpu) { - unsigned int i; bool found = false; +#if CONFIG_NR_CPUS > 1 + unsigned int i; spin_lock(&accounting_lock); @@ -230,6 +231,7 @@ bool core_parking_remove(unsigned int cp core_parking_cpunum[i] = core_parking_cpunum[i + 1]; spin_unlock(&accounting_lock); +#endif /* CONFIG_NR_CPUS > 1 */ return found; } @@ -260,9 +262,11 @@ static int __init register_core_parking_ static int __init cf_check core_parking_init(void) { - int ret = 0; + int ret; - if ( core_parking_controller == PERFORMANCE_FIRST ) + if ( CONFIG_NR_CPUS == 1 ) + ret = 0; + else if ( core_parking_controller == PERFORMANCE_FIRST ) ret = register_core_parking_policy(&performance_first); else ret = register_core_parking_policy(&power_first);
Gcc12 takes issue with core_parking_remove()'s for ( ; i < cur_idle_nums; ++i ) core_parking_cpunum[i] = core_parking_cpunum[i + 1]; complaining that the right hand side array access is past the bounds of 1. Clearly the compiler can't know that cur_idle_nums can only ever be zero in this case (as the sole CPU cannot be parked). Beyond addressing the immediate issue also adjust core_parking_init(): There's no point registering any policy when there's no CPU to park. Since this still doesn't result in the compiler spotting that core_parking_policy is never written (and hence is continuously NULL), also amend core_parking_helper() to avoid eventual similar issues there (minimizing generated code at the same time). Signed-off-by: Jan Beulich <jbeulich@suse.com>