Re: [PATCH v3 6/9] xen/common: add cache coloring allocator for domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.11.2022 16:04, Carlo Nonato wrote:
> On Thu, Nov 10, 2022 at 5:47 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>> On 22.10.2022 17:51, Carlo Nonato wrote:
>>> --- a/xen/arch/arm/p2m.c
>>> +++ b/xen/arch/arm/p2m.c
>>> @@ -661,7 +661,12 @@ static int p2m_create_table(struct p2m_domain *p2m, 
>>> lpae_t *entry)
>>>
>>>      ASSERT(!p2m_is_valid(*entry));
>>>
>>> -    page = alloc_domheap_page(NULL, 0);
>>> +    /* If cache coloring is enabled, p2m tables are allocated using the 
>>> domain
>>> +     * coloring configuration to prevent cache interference. */
>>> +    if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
>>> +        page = alloc_domheap_page(p2m->domain, MEMF_no_refcount);
>>
>> Are you sure you don't mean MEMF_no_owner (which implies MEMF_no_refcount)
>> here? And then ...
> 
> Yes. I've already fixed it in the v4 that I'm working on right now.
> 
>>> +    else
>>> +        page = alloc_domheap_page(NULL, 0);
>>
>> ... is it really necessary to keep the two cases separate?
> 
> Not sure. I don't know the reason behind the original code.

The difference becomes noticable in the NUMA case, which is only being
worked on for Arm. Yet that means that converting the original call in
a prereq patch, stating the NUMA effect as the justification, might be
the way to go. (See commit a7596378e454, which introduces the flag.)

>>> @@ -1926,24 +2106,49 @@ static unsigned long avail_heap_pages(
>>>  void __init end_boot_allocator(void)
>>>  {
>>>      unsigned int i;
>>> +    unsigned long buddy_pages;
>>>
>>> -    /* Pages that are free now go to the domain sub-allocator. */
>>> -    for ( i = 0; i < nr_bootmem_regions; i++ )
>>> +    buddy_pages = PFN_DOWN(buddy_alloc_size);
>>
>> Any reason this can't be the initializer of the variable?
> 
> Nope. The end_boot_allocator() changes are a bit messy. In v4 I'm doing
> things more nicely, moving everything in init_color_heap_pages().
> 
>>> +    if ( !IS_ENABLED(CONFIG_CACHE_COLORING) )
>>>      {
>>> -        struct bootmem_region *r = &bootmem_region_list[i];
>>> -        if ( (r->s < r->e) &&
>>> -             (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
>>> +        /* Pages that are free now go to the domain sub-allocator. */
>>> +        for ( i = 0; i < nr_bootmem_regions; i++ )
>>>          {
>>> -            init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
>>> -            r->e = r->s;
>>> -            break;
>>> +            struct bootmem_region *r = &bootmem_region_list[i];
>>> +            if ( (r->s < r->e) &&
>>
>> Even if you're only re-indenting the original code (which personally I'd
>> prefer if it was avoided), please add the missing blank line between
>> declaration and statement here.
>>
>>> +                (phys_to_nid(pfn_to_paddr(r->s)) == cpu_to_node(0)) )
>>> +            {
>>> +                init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
>>> +                r->e = r->s;
>>> +                break;
>>> +            }
>>>          }
>>>      }
>>> +
>>>      for ( i = nr_bootmem_regions; i-- > 0; )
>>>      {
>>> -        struct bootmem_region *r = &bootmem_region_list[i];
>>> +        struct bootmem_region *r;
>>> +
>>> +        if ( IS_ENABLED(CONFIG_CACHE_COLORING) )
>>> +            r = &bootmem_region_list[nr_bootmem_regions - i - 1];
>>
>> If you want to handle things low-to-high, why don't you alter the
>> earlier loop rather than skipping (and re-indenting) it?
> 
> Yes, you're right.
> 
>> However,
>> considering that in alloc_color_heap_page() you prefer pages at
>> higher addresses, I continue to find it odd that here you want to
>> process low address pages first.
> 
> It doesn't matter if alloc_color_heap_page() returns higher or lower
> addresses. The important thing is to create a sorted list so that min or
> max are easily found. Having a sorted list means that it's easier to insert
> pages if their addresses are always increasing or always decreasing, so that
> starting either from the head or from the tail, the position where to insert
> is found in O(1). If regions are processed high-to-low but pages of each
> region are instead low-to-high, the always-decreasing/always-increasing
> property doesn't hold anymore and the linear search needs to be repeated
> multiple times. This problem can be solved in many ways and doing
> everything low-to-high is one solution.

Of course. But please also put code churn in the set of a criteria to
apply. Page lists are doubly linked, so it shouldn't matter whether
you insert forwards or backwards.

Jan