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

[Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>]

Hi Jan,

On Thu, Nov 28, 2024 at 12:43 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

I'll change the patch 12 title to reflect what you said.

> > --- 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.

Quoting you from v8 (6 months ago, a lot of time ago I know, link here
https://patchew.org/Xen/20240502165533.319988-1-carlo.nonato@xxxxxxxxxxxxxxx/20240502165533.319988-10-carlo.nonato@xxxxxxxxxxxxxxx/#5c16f53f-3bb0-49d6-b174-b0e8c6ceff4c@xxxxxxxx):
> > +/* 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);
>
> I think it would be quite nice if this and ...
>
> > +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]
> > +#else
> > +static unsigned long __initdata buddy_alloc_size;
>
> ... this were folded. Which I think would be possible if the Kconfig
> addition went like this:
>
> config BUDDY_ALLOCATOR_SIZE
> int "Buddy allocator reserved memory size (MiB)" if LLC_COLORING
> default "0" if !LLC_COLORING
> default "64"

But I know that...

> > @@ -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.

buddy-alloc-size must not be parsed when !LLC_COLORING. I'll change the code
accordingly to what you said.

> > +#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).

For the time being, I'll add a note that can be updated when the overall
position is found.

> > +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.

Yes.

> > +        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.

Ok.

> Jan

Thanks.
- Carlo