Re: [PATCH v6 10/15] xen: add cache coloring allocator for domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.01.2024 18:18, Carlo Nonato wrote:
> @@ -157,7 +158,11 @@
>  #define PGC_static 0
>  #endif
>  
> -#define PGC_preserved (PGC_extra | PGC_static)
> +#ifndef PGC_colored
> +#define PGC_colored 0
> +#endif
> +
> +#define PGC_preserved (PGC_extra | PGC_static | PGC_colored)
>  
>  #ifndef PGT_TYPE_INFO_INITIALIZER
>  #define PGT_TYPE_INFO_INITIALIZER 0
> @@ -1504,6 +1509,7 @@ static void free_heap_pages(
>              if ( !mfn_valid(page_to_mfn(predecessor)) ||
>                   !page_state_is(predecessor, free) ||
>                   (predecessor->count_info & PGC_static) ||
> +                 (predecessor->count_info & PGC_colored) ||

How about a 2nd #define (e.g. PGC_no_buddy_merge) to use here and ...

>                   (PFN_ORDER(predecessor) != order) ||
>                   (page_to_nid(predecessor) != node) )
>                  break;
> @@ -1528,6 +1534,7 @@ static void free_heap_pages(
>              if ( !mfn_valid(page_to_mfn(successor)) ||
>                   !page_state_is(successor, free) ||
>                   (successor->count_info & PGC_static) ||
> +                 (successor->count_info & PGC_colored) ||

... here? That'll then also avoid me commenting that I don't see why
these two PGC_* checks aren't folded.

> @@ -1943,6 +1950,161 @@ static unsigned long avail_heap_pages(
>      return free_pages;
>  }
>  
> +/*************************
> + * COLORED SIDE-ALLOCATOR
> + *
> + * Pages are grouped by LLC color in lists which are globally referred to as 
> the
> + * color heap. Lists are populated in end_boot_allocator().
> + * After initialization there will be N lists where N is the number of
> + * available colors on the platform.
> + */
> +static struct page_list_head *__ro_after_init _color_heap;
> +static unsigned long *__ro_after_init free_colored_pages;
> +
> +/* Memory required for buddy allocator to work with colored one */
> +#ifdef CONFIG_LLC_COLORING
> +static unsigned long __initdata buddy_alloc_size =
> +    MB(CONFIG_BUDDY_ALLOCATOR_SIZE);
> +size_param("buddy-alloc-size", buddy_alloc_size);
> +
> +#define domain_num_llc_colors(d) (d)->num_llc_colors
> +#define domain_llc_color(d, i)   (d)->llc_colors[(i)]

Nit: Unnecessary parentheses inside the square brackets.

> +#else
> +static unsigned long __initdata buddy_alloc_size;
> +
> +#define domain_num_llc_colors(d) 0
> +#define domain_llc_color(d, i)   0
> +#endif
> +
> +#define color_heap(color) (&_color_heap[color])

Wouldn't this better live next to _color_heap()'s definition?

> +static void free_color_heap_page(struct page_info *pg, bool need_scrub)
> +{
> +    unsigned int color = page_to_llc_color(pg);
> +    struct page_list_head *head = color_heap(color);
> +
> +    spin_lock(&heap_lock);
> +
> +    mark_page_free(pg, page_to_mfn(pg));
> +
> +    if ( need_scrub )
> +    {
> +        pg->count_info |= PGC_need_scrub;
> +        poison_one_page(pg);
> +    }
> +
> +    pg->count_info |= PGC_colored;

The page transiently losing PGC_colored is not an issue then (presumably
because of holding the heap lock)? Did you consider having mark_page_free()
also use PGC_preserved?

> +    free_colored_pages[color]++;
> +    page_list_add(pg, head);
> +
> +    spin_unlock(&heap_lock);
> +}
> +
> +static struct page_info *alloc_color_heap_page(unsigned int memflags,
> +                                               const struct domain *d)
> +{
> +    struct page_info *pg = NULL;
> +    unsigned int i, color = 0;
> +    unsigned long max = 0;
> +    bool need_tlbflush = false;
> +    uint32_t tlbflush_timestamp = 0;
> +    bool scrub = !(memflags & MEMF_no_scrub);
> +
> +    spin_lock(&heap_lock);
> +
> +    for ( i = 0; i < domain_num_llc_colors(d); i++ )
> +    {
> +        unsigned long free = free_colored_pages[domain_llc_color(d, i)];
> +
> +        if ( free > max )
> +        {
> +            color = domain_llc_color(d, i);
> +            pg = page_list_first(color_heap(color));
> +            max = free;
> +        }
> +    }

The apporach is likely fine at least initially, but: By going from where
the most free pages are, you're not necessarily evenly distributing a
domain's pages over the colors it may use, unless the domain uses its
set of colors exclusively.

> +    if ( !pg )
> +    {
> +        spin_unlock(&heap_lock);
> +        return NULL;
> +    }
> +
> +    pg->count_info = PGC_state_inuse | PGC_colored |
> +                     (pg->count_info & PGC_need_scrub);

Isn't PGC_colored already set on the page? Together with ...

> +    free_colored_pages[color]--;
> +    page_list_del(pg, color_heap(color));
> +
> +    if ( !(memflags & MEMF_no_tlbflush) )
> +        accumulate_tlbflush(&need_tlbflush, pg, &tlbflush_timestamp);
> +
> +    init_free_page_fields(pg);
> +
> +    spin_unlock(&heap_lock);
> +
> +    if ( test_and_clear_bit(_PGC_need_scrub, &pg->count_info) && scrub )

... this, can't the above be simplified?

> +        scrub_one_page(pg);
> +    else if ( scrub )
> +        check_one_page(pg);
> +
> +    if ( need_tlbflush )
> +        filtered_flush_tlb_mask(tlbflush_timestamp);
> +
> +    flush_page_to_ram(mfn_x(page_to_mfn(pg)),
> +                      !(memflags & MEMF_no_icache_flush));
> +
> +    return pg;
> +}
> +
> +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);
> +        nr_pages -= buddy_pages;
> +        buddy_alloc_size -= buddy_pages << PAGE_SHIFT;
> +        pg += buddy_pages;
> +    }
> +
> +    if ( !_color_heap )
> +    {
> +        unsigned int max_nr_colors = get_max_nr_llc_colors();
> +
> +        _color_heap = xmalloc_array(struct page_list_head, max_nr_colors);
> +        free_colored_pages = xzalloc_array(unsigned long, max_nr_colors);
> +        if ( !_color_heap || !free_colored_pages )
> +            panic("Can't allocate colored heap. Buddy reserved size is too 
> low");
> +
> +        for ( i = 0; i < max_nr_colors; i++ )
> +            INIT_PAGE_LIST_HEAD(color_heap(i));
> +    }
> +
> +    if ( nr_pages )
> +        printk(XENLOG_DEBUG
> +               "Init color heap with %lu pages, start MFN: %"PRI_mfn"\n",
> +               nr_pages, mfn_x(page_to_mfn(pg)));

This message can be issued more than once. Is that really desirable /
necessary?

> @@ -2458,7 +2626,14 @@ struct page_info *alloc_domheap_pages(
>      if ( memflags & MEMF_no_owner )
>          memflags |= MEMF_no_refcount;
>  
> -    if ( !dma_bitsize )
> +    /* Only domains are supported for coloring */
> +    if ( d && llc_coloring_enabled )
> +    {
> +        /* Colored allocation must be done on 0 order */
> +        if ( order || (pg = alloc_color_heap_page(memflags, d)) == NULL )
> +            return NULL;
> +    }

I think I had asked before: What about MEMF_node() or MEMF_bits()
having been used by the caller?

Jan