Re: [PATCH 3/3] xen/common: Add NUMA node id bounds check to page_alloc.c/node_to_scrub

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2023 09:32, Jan Beulich wrote:
> On 26.09.2023 00:42, Shawn Anastasio wrote:
>> When building for Power with CONFIG_DEBUG unset,

Hmm, depending on what gcc versions are used in CI, the above may be the
reason why ...

>> a compiler error gets
>> raised inside page_alloc.c's node_to_scrub function, likely due to the
>> increased optimization level:
>>
>> common/page_alloc.c: In function 'node_to_scrub.part.0':
>> common/page_alloc.c:1217:29: error: array subscript 1 is above array
>>             bounds of 'long unsigned int[1]' [-Werror=array-bounds]
>>  1217 |         if ( node_need_scrub[node] )
> 
> That's gcc13?
> 
>> It appears that this is a false positive, given that in practice
>> cycle_node should never return a node ID >= MAX_NUMNODES as long as the
>> architecture's node_online_map is properly defined and initialized, so
>> this additional bounds check is only to satisfy GCC.
> 
> Looks very similar to the situation that c890499871cc ("timer: fix
> NR_CPUS=1 build with gcc13") was dealing with, just that here it's
> MAX_NUMNODES. I'd therefore prefer a solution similar to the one
> taken there, i.e. make code conditional rather than add yet more
> code.
> 
> Otherwise, ...
> 
>> --- a/xen/common/page_alloc.c
>> +++ b/xen/common/page_alloc.c
>> @@ -1211,6 +1211,9 @@ static unsigned int node_to_scrub(bool get_node)
>>          } while ( !cpumask_empty(&node_to_cpumask(node)) &&
>>                    (node != local_node) );
>>
>> +        if ( node >= MAX_NUMNODES )
>> +            break;
> 
> ... this clearly redundant check would need to gain a comment.
> 
> What I'm puzzled by is that on Arm we had no reports of a similar
> problem, despite NUMA also not getting selected there (yet).

... this wasn't observed, yet. As far as I'm concerned, all my Arm builds
are debug ones (which I may need to change).

Jan