Re: [PATCH v7 10/14] xen: add cache coloring allocator for domains

[Jan Beulich <jbeulich@suse.com>]

On 21.03.2024 16:36, Carlo Nonato wrote:
> On Tue, Mar 19, 2024 at 5:43 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 15.03.2024 11:58, Carlo Nonato wrote:
>>> +static void __init init_color_heap_pages(struct page_info *pg,
>>> +                                         unsigned long nr_pages)
>>> +{
>>> +    unsigned int i;
>>> +    bool need_scrub = opt_bootscrub == BOOTSCRUB_IDLE;
>>> +
>>> +    if ( buddy_alloc_size )
>>> +    {
>>> +        unsigned long buddy_pages = min(PFN_DOWN(buddy_alloc_size), nr_pages);
>>> +
>>> +        init_heap_pages(pg, buddy_pages);
>>
>> There's a corner case where init_heap_pages() would break when passed 0
>> as 2nd argument.
> 
> I don't see it. There's just a for-loop that would be skipped in that case...

Look at the first comment in the function and the if() following it. I
don't think that code would work very well when nr_pages == 0.

>>> +static void dump_color_heap(void)
>>> +{
>>> +    unsigned int color;
>>> +
>>> +    printk("Dumping color heap info\n");
>>> +    for ( color = 0; color < get_max_nr_llc_colors(); color++ )
>>> +        if ( free_colored_pages[color] > 0 )
>>> +            printk("Color heap[%u]: %lu pages\n",
>>> +                   color, free_colored_pages[color]);
>>> +}
>>
>> While having all of the code above from here outside of any #ifdef is
>> helpful to prevent unintended breakage when changes are made and tested
>> only on non-Arm64 targets, I'd still like to ask: Halfway recent
>> compilers manage to eliminate everything? I'd like to avoid e.g. x86
>> being left with traces of coloring despite not being able at all to use
>> it.
> 
> I don't know the answer to this, sorry.

Yet it is important to have.

Jan