Re: [PATCH 21/36] xen/common: add colored allocator initialization

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.03.2022 18:46, Marco Solieri wrote:
> From: Luca Miccio <lucmiccio@xxxxxxxxx>
> 
> Initialize colored heap and allocator data structures. It is assumed
> that pages are given to the init function is in ascending order.

I don't think this is a good assumption to make.

> To
> ensure that, pages are retrieved from bootmem_regions starting from the
> first one. Moreover, this allows quickly insertion of freed pages into
> the colored allocator's internal data structures -- sorted lists.

I wouldn't call insertion by linear scan "quick", to be honest.

> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -2154,11 +2154,26 @@ void __init end_boot_allocator(void)
>              break;
>          }
>      }
> -    for ( i = nr_bootmem_regions; i-- > 0; )
> +
> +    for ( i = 0; i < nr_bootmem_regions; i++ )
>      {
>          struct bootmem_region *r = &bootmem_region_list[i];
> -        if ( r->s < r->e )
> -            init_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);
> +
> +        /*
> +         * Find the first region that can fill the buddy allocator memory
> +         * specified by buddy_required_size.
> +         */

Why would all of this memory need to come from a single region? And
why would any region - regardless of address - be okay?

> +        if ( buddy_required_size && (r->e - r->s) >
> +            PFN_DOWN(buddy_required_size) )

I think >= will do here?

Also - nit: Indentation.

> +        {
> +            init_heap_pages(mfn_to_page(_mfn(r->s)),
> +                PFN_DOWN(buddy_required_size));

And again - indentation.

> +            r->s += PFN_DOWN(buddy_required_size);
> +            buddy_required_size = 0;
> +        }
> +
> +        init_col_heap_pages(mfn_to_page(_mfn(r->s)), r->e - r->s);

Judging from this, buddy_required_size can actually be __initdata in
the previous patch. Being able to spot such is another reason to not
split patches like this.

> @@ -2619,9 +2634,12 @@ int assign_pages(
>          page_set_owner(&pg[i], d);
>          smp_wmb(); /* Domain pointer must be visible before updating refcnt. 
> */
>          pg[i].count_info =
> -            (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated 
> | 1;
> +             (pg[i].count_info & (PGC_extra | PGC_reserved)) | PGC_allocated 
> | 1;

Why the change?

> @@ -2642,6 +2660,15 @@ struct page_info *alloc_domheap_pages(
>      unsigned int bits = memflags >> _MEMF_bits, zone_hi = NR_ZONES - 1;
>      unsigned int dma_zone;
>  
> +    /* Only Dom0 and DomUs are supported for coloring */
> +    if ( d && d->max_colors > 0 )
> +    {
> +        /* Colored allocation must be done on 0 order */
> +        if (order)

Nit: Missing blanks.

> @@ -2761,8 +2788,10 @@ void free_domheap_pages(struct page_info *pg, unsigned 
> int order)
>              scrub = 1;
>          }
>  
> -        free_heap_pages(pg, order, scrub);
> -    }
> +        if ( is_page_colored(pg) )
> +            free_col_heap_page(pg);
> +        else
> +            free_heap_pages(pg, order, scrub);}

Very interesting brace placement.

Jan