Re: [PATCH v10 09/12] xen: add cache coloring allocator for domains

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.11.2024 15:13, Carlo Nonato wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -270,6 +270,20 @@ and not running softirqs. Reduce this if softirqs are 
> not being run frequently
>  enough. Setting this to a high value may cause boot failure, particularly if
>  the NMI watchdog is also enabled.
>  
> +### buddy-alloc-size (arm64)
> +> `= <size>`
> +
> +> Default: `64M`
> +
> +Amount of memory reserved for the buddy allocator when colored allocator is
> +active. This option is available only when `CONFIG_LLC_COLORING` is enabled.
> +The colored allocator is meant as an alternative to the buddy allocator,
> +because its allocation policy is by definition incompatible with the generic
> +one. Since the Xen heap systems is not colored yet, we need to support the
> +coexistence of the two allocators for now. This parameter, which is optional
> +and for expert only, it's used to set the amount of memory reserved to the
> +buddy allocator.

Every time I see this, and I further see the title of patch 12, I'm confused,
expecting that what's being said here needs adjusting (or even undoing) there.
The issue is that patch 12's title says just "Xen" when, if I'm not mistaken,
it really only means the Xen image.

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -537,4 +537,12 @@ config LLC_COLORS_ORDER
>         The default value corresponds to an 8 MiB 16-ways LLC, which should be
>         more than what's needed in the general case.
>  
> +config BUDDY_ALLOCATOR_SIZE
> +     int "Buddy allocator reserved memory size (MiB)" if LLC_COLORING
> +     default "0" if !LLC_COLORING
> +     default "64"
> +     help
> +       Amount of memory reserved for the buddy allocator to serve Xen heap,
> +       working alongside the colored one.

As the description has nothing in this regard: Why again is it that this
can't simply have "depends on LLC_COLORING"? Even if it ends up with a
value of 0, in an LLC_COLORING=n (or LLC_COLORING entirely absent) .config
I'd find it at least irritating to see this setting to be there.

> @@ -1945,6 +1960,155 @@ 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;
> +#define color_heap(color) (&_color_heap[color])
> +
> +static unsigned long *__ro_after_init free_colored_pages;
> +
> +/* Memory required for buddy allocator to work with colored one */
> +static unsigned long __initdata buddy_alloc_size =
> +    MB(CONFIG_BUDDY_ALLOCATOR_SIZE);
> +size_param("buddy-alloc-size", buddy_alloc_size);
> +
> +#ifdef CONFIG_LLC_COLORING

According to the (updated) command line doc, this #ifdef needs to move up
so the command line option indeed is unrecognized when !LLC_COLORING.
Question then is whether the variable is actually needed. With the variable
also moved into the #ifdef, the need for BUDDY_ALLOCATOR_SIZE also goes
away when !LLC_COLORING (see the comment on the common/Kconfig change). Of
course you'll then need "#ifndef buddy_alloc_size" in
init_color_heap_pages(), around the respective if() there.

> +#define domain_num_llc_colors(d) ((d)->num_llc_colors)
> +#define domain_llc_color(d, i)   ((d)->llc_colors[i])
> +#else
> +#define domain_num_llc_colors(d) 0
> +#define domain_llc_color(d, i)   0
> +#endif
> +
> +static void free_color_heap_page(struct page_info *pg, bool need_scrub)
> +{
> +    unsigned int color;
> +
> +    color = page_to_llc_color(pg);
> +    free_colored_pages[color]++;
> +    /*
> +     * Head insertion allows re-using cache-hot pages in configurations 
> without
> +     * sharing of colors.
> +     */
> +    page_list_add(pg, color_heap(color));
> +}

Iirc it was me who had asked to keep this and further functions outside of
#ifdef-s, for the compiler's DCE to take care of. However, with all that
Misra work that has been going on since then, I now wonder what Misra
thinks of this: When PGC_colored is 0, functions like the above are
unreachable, and any code or data the compiler doesn't manage to eliminate
would be dead. Imo if the code is to remain as is, correctness Misra-wise
may want mentioning in the description (this isn't the only place we have
such, so an overall position towards this is going to be needed).

> +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 >= PAGE_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);

At this point, xvmalloc() and friends want using by all new code, unless
explicitly justified otherwise.

> +        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));

While for this loop i being unsigned int is okay, I fear that ...

> +    }
> +
> +    for ( i = 0; i < nr_pages; i++ )
> +    {
> +        pg[i].count_info = PGC_colored;
> +        free_color_heap_page(&pg[i], need_scrub);
> +    }

... for this loop it isn't.

Jan