RE: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on static allocation

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien and Jan

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Saturday, April 2, 2022 2:54 AM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>;
> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei
> Liu <wl@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH v1 3/5] xen/arm: unpopulate memory when domain on
> static allocation
> 
> Hi Penny,
> 
> On 31/03/2022 11:30, Penny Zheng wrote:
> > Another reason I want to keep page allocated is that if putting pages
> > in resv_page_list upon dropping the last ref, we need to do a lot
> > things on pages to totally let it free, like set its owner to NULL,
> > changing page state from in_use to free, etc.
> This is not only about optimization here. Bad things can happen if you let a
> page in state free that is not meant to be used by the buddy allocator (e.g. 
> it
> was reserved for static memory).
> 
> I discovered it earlier this year when trying to optimize
> init_heap_pages() for Live-Update. It was quite hard to debug because the
> corruption very rarely happened. So let me explain it before you face the same
> issue :).
> 
> free_heap_pages() will try to merge the about-to-be-freed chunk with the
> predecessor and/or successor. To know if the page can be merged, the
> algorithm is looking at whether the chunk is suitably aligned (e.g. same
> order) and if the page is in state free.
> 
> AFAICT, the pages belonging to the buddy allocator could be right next to
> region reserved memory. So there is a very slim chance that
> free_heap_pages() may decide to merge a chunk from the static region with
> the about-to-be-free chunk. Nothing very good will ensue.
> 

Oh,,, that's a thousand true.
If the free static region is the buddy of the about-to-be-free chunk, current
code will merge the static region to the heap, which is unacceptable.

I'll fix it in this patch serie too and I'm also preferring option 1~

And for unpopulating memory on runtime, I'll do what Jan suggests,
adding a new logic of moving the page from d->page_list to
d->resv_page_list in arch_free_heap_page() for reserved pages. 

> Technically, this is already a bug in the already merged implementation of the
> static memory allocator.
> 
> I can see two ways to fix it:
>    1) Update free_heap_pages() to check whether the page has PGC_reserved
> set.
>    2) Use a different state for pages used by the static allocator.
> 
> So far my preference is leaning towards 1. But I would like to hear other
> opinions.
> 
> Cheers,
> 
> --
> Julien Grall