* [PATCH 0/2][4.15?] fix build when NR_CPUS == 1 @ 2021-03-01 8:27 Jan Beulich 2021-03-01 8:30 ` [PATCH 1/2][4.15?] sched: " Jan Beulich ` (2 more replies) 0 siblings, 3 replies; 16+ messages in thread From: Jan Beulich @ 2021-03-01 8:27 UTC (permalink / raw) To: xen-devel Cc: Connor Davis, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Dario Faggioli, Ian Jackson While we've long done away with CONFIG_SMP, we still allow CONFIG_NR_CPUS to be set to 1. Hence at least randconfig builds may fail, and the first of the two issues addressed was actually observed in the RISC-V bring-up work. I didn't check whether Arm would also have issues like these. 1: sched: fix build when NR_CPUS == 1 2: x86: fix build when NR_CPUS == 1 Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1 2021-03-01 8:27 [PATCH 0/2][4.15?] fix build when NR_CPUS == 1 Jan Beulich @ 2021-03-01 8:30 ` Jan Beulich 2021-03-01 15:57 ` Ian Jackson 2021-03-01 8:31 ` [PATCH 2/2][4.15?] x86: " Jan Beulich 2021-03-01 8:33 ` [PATCH 0/2][4.15?] " Jan Beulich 2 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2021-03-01 8:30 UTC (permalink / raw) To: xen-devel; +Cc: Connor Davis, George Dunlap, Dario Faggioli, Ian Jackson In this case the compiler is recognizing that no valid array indexes remain, and hence e.g. reports: core.c: In function 'cpu_schedule_up': core.c:2769:19: error: array subscript 1 is above array bounds of 'struct vcpu *[1]' [-Werror=array-bounds] 2769 | if ( idle_vcpu[cpu] == NULL ) | ~~~~~~~~~^~~~~ Reported-by: Connor Davis <connojdavis@gmail.com> Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/common/sched/core.c +++ b/xen/common/sched/core.c @@ -2768,6 +2768,12 @@ static int cpu_schedule_up(unsigned int if ( cpu == 0 ) return 0; + /* + * Guard in particular against the compiler suspecting out-of-bounds + * array accesses below when NR_CPUS=1. + */ + BUG_ON(cpu >= NR_CPUS); + if ( idle_vcpu[cpu] == NULL ) vcpu_create(idle_vcpu[0]->domain, cpu); else ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1 2021-03-01 8:30 ` [PATCH 1/2][4.15?] sched: " Jan Beulich @ 2021-03-01 15:57 ` Ian Jackson 2021-03-01 17:50 ` Dario Faggioli 0 siblings, 1 reply; 16+ messages in thread From: Ian Jackson @ 2021-03-01 15:57 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Connor Davis, George Dunlap, Dario Faggioli Jan Beulich writes ("[PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1"): > In this case the compiler is recognizing that no valid array indexes > remain, and hence e.g. reports: Release-Acked-by: Ian Jackson <iwj@xenproject.org> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2][4.15?] sched: fix build when NR_CPUS == 1 2021-03-01 15:57 ` Ian Jackson @ 2021-03-01 17:50 ` Dario Faggioli 0 siblings, 0 replies; 16+ messages in thread From: Dario Faggioli @ 2021-03-01 17:50 UTC (permalink / raw) To: Ian Jackson, Jan Beulich; +Cc: xen-devel, Connor Davis, George Dunlap [-- Attachment #1: Type: text/plain, Size: 647 bytes --] On Mon, 2021-03-01 at 15:57 +0000, Ian Jackson wrote: > Jan Beulich writes ("[PATCH 1/2][4.15?] sched: fix build when NR_CPUS > == 1"): > > In this case the compiler is recognizing that no valid array > > indexes > > remain, and hence e.g. reports: > > Release-Acked-by: Ian Jackson <iwj@xenproject.org> > Reviewed-by: Dario Faggioli <dfaggioli@suse.com> Regards -- Dario Faggioli, Ph.D http://about.me/dario.faggioli Virtualization Software Engineer SUSE Labs, SUSE https://www.suse.com/ ------------------------------------------------------------------- <<This happens because _I_ choose it to happen!>> (Raistlin Majere) [-- Attachment #2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 8:27 [PATCH 0/2][4.15?] fix build when NR_CPUS == 1 Jan Beulich 2021-03-01 8:30 ` [PATCH 1/2][4.15?] sched: " Jan Beulich @ 2021-03-01 8:31 ` Jan Beulich 2021-03-01 14:01 ` Roger Pau Monné 2021-03-01 16:03 ` Ian Jackson 2021-03-01 8:33 ` [PATCH 0/2][4.15?] " Jan Beulich 2 siblings, 2 replies; 16+ messages in thread From: Jan Beulich @ 2021-03-01 8:31 UTC (permalink / raw) To: xen-devel Cc: Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Ian Jackson In this case the compiler is recognizing that no valid array indexes remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, ...)), but oddly enough isn't really consistent about the checking it does (see the code comment). Signed-off-by: Jan Beulich <jbeulich@suse.com> --- a/xen/arch/x86/genapic/x2apic.c +++ b/xen/arch/x86/genapic/x2apic.c @@ -54,7 +54,17 @@ static void init_apic_ldr_x2apic_cluster per_cpu(cluster_cpus, this_cpu) = cluster_cpus_spare; for_each_online_cpu ( cpu ) { - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) + if ( this_cpu == cpu ) + continue; + /* + * Guard in particular against the compiler suspecting out-of-bounds + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it + * is the 1st of these alone which actually helps, not the 2nd, nor + * are both required together there). + */ + BUG_ON(this_cpu >= NR_CPUS); + BUG_ON(cpu >= NR_CPUS); + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) continue; per_cpu(cluster_cpus, this_cpu) = per_cpu(cluster_cpus, cpu); break; ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 8:31 ` [PATCH 2/2][4.15?] x86: " Jan Beulich @ 2021-03-01 14:01 ` Roger Pau Monné 2021-03-01 15:14 ` Jan Beulich 2021-03-01 16:03 ` Ian Jackson 1 sibling, 1 reply; 16+ messages in thread From: Roger Pau Monné @ 2021-03-01 14:01 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: > In this case the compiler is recognizing that no valid array indexes > remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > ...)), but oddly enough isn't really consistent about the checking it > does (see the code comment). I assume this is because of the underlying per_cpu access to __per_cpu_offset using cpu as the index, in which case wouldn't it be better to place the BUG_ON there? Also I wonder why the accesses the same function does to the per_cpu area before the modified chunk using this_cpu as index don't also trigger such warnings. Thanks, Roger. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 14:01 ` Roger Pau Monné @ 2021-03-01 15:14 ` Jan Beulich 2021-03-01 18:00 ` Roger Pau Monné 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2021-03-01 15:14 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson On 01.03.2021 15:01, Roger Pau Monné wrote: > On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: >> In this case the compiler is recognizing that no valid array indexes >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >> ...)), but oddly enough isn't really consistent about the checking it >> does (see the code comment). > > I assume this is because of the underlying per_cpu access to > __per_cpu_offset using cpu as the index, in which case wouldn't it be > better to place the BUG_ON there? Not sure, to be honest. It seemed more logical to me to place it next to where the issue is. > Also I wonder why the accesses the same function does to the per_cpu > area before the modified chunk using this_cpu as index don't also > trigger such warnings. The compiler appears to be issuing the warning when it can prove that no legitimate index can make it to a respective use. in this case it means that is is if ( this_cpu == cpu ) continue; which makes it possible for the compiler to know that what gets past this would be an out of bounds access, since for NR_CPUS=1 both this_cpu and cpu can only validly both be zero. (This also plays into my choice of placement, because it is not x2apic_cluster() on its own which has this issue.) Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 15:14 ` Jan Beulich @ 2021-03-01 18:00 ` Roger Pau Monné 2021-03-02 9:59 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Roger Pau Monné @ 2021-03-01 18:00 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: > On 01.03.2021 15:01, Roger Pau Monné wrote: > > On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: > >> In this case the compiler is recognizing that no valid array indexes > >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > >> ...)), but oddly enough isn't really consistent about the checking it > >> does (see the code comment). > > > > I assume this is because of the underlying per_cpu access to > > __per_cpu_offset using cpu as the index, in which case wouldn't it be > > better to place the BUG_ON there? > > Not sure, to be honest. It seemed more logical to me to place it > next to where the issue is. I'm not sure whether I would prefer to place it in per_cpu directly to avoid similar issues popping up in other parts of the code in the future? Maybe that's not likely. TBH it seems kind of random to be placing this BUG_ON conditionals to make the compilers happy, but maybe there's no other option. > > Also I wonder why the accesses the same function does to the per_cpu > > area before the modified chunk using this_cpu as index don't also > > trigger such warnings. > > The compiler appears to be issuing the warning when it can prove > that no legitimate index can make it to a respective use. in this > case it means that is is > > if ( this_cpu == cpu ) > continue; > > which makes it possible for the compiler to know that what gets > past this would be an out of bounds access, since for NR_CPUS=1 > both this_cpu and cpu can only validly both be zero. (This also > plays into my choice of placement, because it is not > x2apic_cluster() on its own which has this issue.) I see, thanks for the explanation. It makes me wonder whether other random issues like this will pop up in other parts of the code. We should have a gitlab build with NR_CPUS=1 in order to assert we don't regress it. Speaking for myself I certainly won't be able to detect whether something broke this support in the future. Thanks, Roger. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 18:00 ` Roger Pau Monné @ 2021-03-02 9:59 ` Jan Beulich 2021-03-02 10:57 ` Roger Pau Monné 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2021-03-02 9:59 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson On 01.03.2021 19:00, Roger Pau Monné wrote: > On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: >> On 01.03.2021 15:01, Roger Pau Monné wrote: >>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: >>>> In this case the compiler is recognizing that no valid array indexes >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >>>> ...)), but oddly enough isn't really consistent about the checking it >>>> does (see the code comment). >>> >>> I assume this is because of the underlying per_cpu access to >>> __per_cpu_offset using cpu as the index, in which case wouldn't it be >>> better to place the BUG_ON there? >> >> Not sure, to be honest. It seemed more logical to me to place it >> next to where the issue is. > > I'm not sure whether I would prefer to place it in per_cpu directly to > avoid similar issues popping up in other parts of the code in the > future? That's going to be a lot of BUG_ON(), and hence a lot of "ud2" instances. Not something I'm keen on having. The more that it's not the per_cpu() which triggers the warning, but separate conditionals allowing the compiler to deduce value ranges of variables. > Maybe that's not likely. TBH it seems kind of random to be placing > this BUG_ON conditionals to make the compilers happy, but maybe > there's no other option. In principle I agree - hence the longish comment. >>> Also I wonder why the accesses the same function does to the per_cpu >>> area before the modified chunk using this_cpu as index don't also >>> trigger such warnings. >> >> The compiler appears to be issuing the warning when it can prove >> that no legitimate index can make it to a respective use. in this >> case it means that is is >> >> if ( this_cpu == cpu ) >> continue; >> >> which makes it possible for the compiler to know that what gets >> past this would be an out of bounds access, since for NR_CPUS=1 >> both this_cpu and cpu can only validly both be zero. (This also >> plays into my choice of placement, because it is not >> x2apic_cluster() on its own which has this issue.) > > I see, thanks for the explanation. It makes me wonder whether other > random issues like this will pop up in other parts of the code. We > should have a gitlab build with NR_CPUS=1 in order to assert we don't > regress it. Speaking for myself I certainly won't be able to detect > whether something broke this support in the future. I guess I'll carry on having such a build test locally. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-02 9:59 ` Jan Beulich @ 2021-03-02 10:57 ` Roger Pau Monné 2021-03-02 11:18 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Roger Pau Monné @ 2021-03-02 10:57 UTC (permalink / raw) To: Jan Beulich; +Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson On Tue, Mar 02, 2021 at 10:59:37AM +0100, Jan Beulich wrote: > On 01.03.2021 19:00, Roger Pau Monné wrote: > > On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: > >> On 01.03.2021 15:01, Roger Pau Monné wrote: > >>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: > >>>> In this case the compiler is recognizing that no valid array indexes > >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > >>>> ...)), but oddly enough isn't really consistent about the checking it > >>>> does (see the code comment). > >>> > >>> I assume this is because of the underlying per_cpu access to > >>> __per_cpu_offset using cpu as the index, in which case wouldn't it be > >>> better to place the BUG_ON there? > >> > >> Not sure, to be honest. It seemed more logical to me to place it > >> next to where the issue is. > > > > I'm not sure whether I would prefer to place it in per_cpu directly to > > avoid similar issues popping up in other parts of the code in the > > future? > > That's going to be a lot of BUG_ON(), and hence a lot of "ud2" > instances. Not something I'm keen on having. The more that it's > not the per_cpu() which triggers the warning, but separate > conditionals allowing the compiler to deduce value ranges of > variables. Right. I don't see much other way around this then. Did you check with clang also? Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-02 10:57 ` Roger Pau Monné @ 2021-03-02 11:18 ` Jan Beulich 0 siblings, 0 replies; 16+ messages in thread From: Jan Beulich @ 2021-03-02 11:18 UTC (permalink / raw) To: Roger Pau Monné Cc: xen-devel, Andrew Cooper, Wei Liu, George Dunlap, Ian Jackson On 02.03.2021 11:57, Roger Pau Monné wrote: > On Tue, Mar 02, 2021 at 10:59:37AM +0100, Jan Beulich wrote: >> On 01.03.2021 19:00, Roger Pau Monné wrote: >>> On Mon, Mar 01, 2021 at 04:14:26PM +0100, Jan Beulich wrote: >>>> On 01.03.2021 15:01, Roger Pau Monné wrote: >>>>> On Mon, Mar 01, 2021 at 09:31:07AM +0100, Jan Beulich wrote: >>>>>> In this case the compiler is recognizing that no valid array indexes >>>>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >>>>>> ...)), but oddly enough isn't really consistent about the checking it >>>>>> does (see the code comment). >>>>> >>>>> I assume this is because of the underlying per_cpu access to >>>>> __per_cpu_offset using cpu as the index, in which case wouldn't it be >>>>> better to place the BUG_ON there? >>>> >>>> Not sure, to be honest. It seemed more logical to me to place it >>>> next to where the issue is. >>> >>> I'm not sure whether I would prefer to place it in per_cpu directly to >>> avoid similar issues popping up in other parts of the code in the >>> future? >> >> That's going to be a lot of BUG_ON(), and hence a lot of "ud2" >> instances. Not something I'm keen on having. The more that it's >> not the per_cpu() which triggers the warning, but separate >> conditionals allowing the compiler to deduce value ranges of >> variables. > > Right. I don't see much other way around this then. Did you check with > clang also? No, I didn't. But if anything it would require further additions, surely no less. > Acked-by: Roger Pau Monné <roger.pau@citrix.com> Thanks. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 8:31 ` [PATCH 2/2][4.15?] x86: " Jan Beulich 2021-03-01 14:01 ` Roger Pau Monné @ 2021-03-01 16:03 ` Ian Jackson 2021-03-02 10:02 ` Jan Beulich 1 sibling, 1 reply; 16+ messages in thread From: Ian Jackson @ 2021-03-01 16:03 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): > In this case the compiler is recognizing that no valid array indexes > remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > ...)), but oddly enough isn't really consistent about the checking it > does (see the code comment). ... > - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) > + if ( this_cpu == cpu ) > + continue; > + /* > + * Guard in particular against the compiler suspecting out-of-bounds > + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it > + * is the 1st of these alone which actually helps, not the 2nd, nor > + * are both required together there). > + */ > + BUG_ON(this_cpu >= NR_CPUS); > + BUG_ON(cpu >= NR_CPUS); > + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) > continue; Is there some particular reason for not putting the BUG_ON before the if test ? That would avoid the refactoring. Of course putting it in next to the array indexing would address that too. Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-01 16:03 ` Ian Jackson @ 2021-03-02 10:02 ` Jan Beulich 2021-03-02 12:28 ` Ian Jackson 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2021-03-02 10:02 UTC (permalink / raw) To: Ian Jackson Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap On 01.03.2021 17:03, Ian Jackson wrote: > Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): >> In this case the compiler is recognizing that no valid array indexes >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >> ...)), but oddly enough isn't really consistent about the checking it >> does (see the code comment). > ... >> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) >> + if ( this_cpu == cpu ) >> + continue; >> + /* >> + * Guard in particular against the compiler suspecting out-of-bounds >> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it >> + * is the 1st of these alone which actually helps, not the 2nd, nor >> + * are both required together there). >> + */ >> + BUG_ON(this_cpu >= NR_CPUS); >> + BUG_ON(cpu >= NR_CPUS); >> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) >> continue; > > Is there some particular reason for not putting the BUG_ON before the > if test ? That would avoid the refactoring. I want it to be as close as possible to the place where the issue is. I also view the refactoring as a good thing, since it allows a style correction as a side effect. > Of course putting it in next to the array indexing would address that > too. See my earlier reply to Roger's similar remark (which still was along the lines of what I've said above). Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-02 10:02 ` Jan Beulich @ 2021-03-02 12:28 ` Ian Jackson 2021-03-02 13:37 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Ian Jackson @ 2021-03-02 12:28 UTC (permalink / raw) To: Jan Beulich Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap Jan Beulich writes ("Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): > On 01.03.2021 17:03, Ian Jackson wrote: > > Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): > >> In this case the compiler is recognizing that no valid array indexes > >> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, > >> ...)), but oddly enough isn't really consistent about the checking it > >> does (see the code comment). > > ... > >> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) > >> + if ( this_cpu == cpu ) > >> + continue; > >> + /* > >> + * Guard in particular against the compiler suspecting out-of-bounds > >> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it > >> + * is the 1st of these alone which actually helps, not the 2nd, nor > >> + * are both required together there). > >> + */ > >> + BUG_ON(this_cpu >= NR_CPUS); > >> + BUG_ON(cpu >= NR_CPUS); > >> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) > >> continue; > > > > Is there some particular reason for not putting the BUG_ON before the > > if test ? That would avoid the refactoring. > > I want it to be as close as possible to the place where the issue > is. I also view the refactoring as a good thing, since it allows > a style correction as a side effect. I'm afraid that at this stage of the release I would prefer changes to be as small as reasonably sensible. So unless there is some reason, other than taste, style or formatting, could we please just introduce the new BUG_ON and not also do other refactoring. Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1 2021-03-02 12:28 ` Ian Jackson @ 2021-03-02 13:37 ` Jan Beulich 0 siblings, 0 replies; 16+ messages in thread From: Jan Beulich @ 2021-03-02 13:37 UTC (permalink / raw) To: Ian Jackson Cc: xen-devel, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap On 02.03.2021 13:28, Ian Jackson wrote: > Jan Beulich writes ("Re: [PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): >> On 01.03.2021 17:03, Ian Jackson wrote: >>> Jan Beulich writes ("[PATCH 2/2][4.15?] x86: fix build when NR_CPUS == 1"): >>>> In this case the compiler is recognizing that no valid array indexes >>>> remain (in x2apic_cluster()'s access to per_cpu(cpu_2_logical_apicid, >>>> ...)), but oddly enough isn't really consistent about the checking it >>>> does (see the code comment). >>> ... >>>> - if (this_cpu == cpu || x2apic_cluster(this_cpu) != x2apic_cluster(cpu)) >>>> + if ( this_cpu == cpu ) >>>> + continue; >>>> + /* >>>> + * Guard in particular against the compiler suspecting out-of-bounds >>>> + * array accesses below when NR_CPUS=1 (oddly enough with gcc 10 it >>>> + * is the 1st of these alone which actually helps, not the 2nd, nor >>>> + * are both required together there). >>>> + */ >>>> + BUG_ON(this_cpu >= NR_CPUS); >>>> + BUG_ON(cpu >= NR_CPUS); >>>> + if ( x2apic_cluster(this_cpu) != x2apic_cluster(cpu) ) >>>> continue; >>> >>> Is there some particular reason for not putting the BUG_ON before the >>> if test ? That would avoid the refactoring. >> >> I want it to be as close as possible to the place where the issue >> is. I also view the refactoring as a good thing, since it allows >> a style correction as a side effect. > > I'm afraid that at this stage of the release I would prefer changes to > be as small as reasonably sensible. So unless there is some > reason, other than taste, style or formatting, could we please just > introduce the new BUG_ON and not also do other refactoring. FAOD: That's fine - I'll keep this queued for 4.16 then. I did put a question mark behind the version anyway. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/2][4.15?] fix build when NR_CPUS == 1 2021-03-01 8:27 [PATCH 0/2][4.15?] fix build when NR_CPUS == 1 Jan Beulich 2021-03-01 8:30 ` [PATCH 1/2][4.15?] sched: " Jan Beulich 2021-03-01 8:31 ` [PATCH 2/2][4.15?] x86: " Jan Beulich @ 2021-03-01 8:33 ` Jan Beulich 2 siblings, 0 replies; 16+ messages in thread From: Jan Beulich @ 2021-03-01 8:33 UTC (permalink / raw) To: Ian Jackson Cc: Connor Davis, Andrew Cooper, Wei Liu, Roger Pau Monné, George Dunlap, Dario Faggioli, xen-devel On 01.03.2021 09:27, Jan Beulich wrote: > While we've long done away with CONFIG_SMP, we still allow > CONFIG_NR_CPUS to be set to 1. Hence at least randconfig builds > may fail, and the first of the two issues addressed was actually > observed in the RISC-V bring-up work. I didn't check whether Arm > would also have issues like these. > > 1: sched: fix build when NR_CPUS == 1 > 2: x86: fix build when NR_CPUS == 1 I've tagged this with a question mark because on one hand such configurations are unusual and hence unlikely to be overly relevant for the release. Otoh randconfig failures would better be avoided. Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2021-03-02 13:37 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-01 8:27 [PATCH 0/2][4.15?] fix build when NR_CPUS == 1 Jan Beulich 2021-03-01 8:30 ` [PATCH 1/2][4.15?] sched: " Jan Beulich 2021-03-01 15:57 ` Ian Jackson 2021-03-01 17:50 ` Dario Faggioli 2021-03-01 8:31 ` [PATCH 2/2][4.15?] x86: " Jan Beulich 2021-03-01 14:01 ` Roger Pau Monné 2021-03-01 15:14 ` Jan Beulich 2021-03-01 18:00 ` Roger Pau Monné 2021-03-02 9:59 ` Jan Beulich 2021-03-02 10:57 ` Roger Pau Monné 2021-03-02 11:18 ` Jan Beulich 2021-03-01 16:03 ` Ian Jackson 2021-03-02 10:02 ` Jan Beulich 2021-03-02 12:28 ` Ian Jackson 2021-03-02 13:37 ` Jan Beulich 2021-03-01 8:33 ` [PATCH 0/2][4.15?] " Jan Beulich
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.