Hi Julien and Jan > -----Original Message----- > From: Julien Grall <jul...@xen.org> > Sent: Saturday, April 2, 2022 2:54 AM > To: Penny Zheng <penny.zh...@arm.com>; Jan Beulich <jbeul...@suse.com> > Cc: Wei Chen <wei.c...@arm.com>; Henry Wang <henry.w...@arm.com>; > Andrew Cooper <andrew.coop...@citrix.com>; George Dunlap > <george.dun...@citrix.com>; Stefano Stabellini <sstabell...@kernel.org>; Wei > Liu <w...@xen.org>; xen-devel@lists.xenproject.org > 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
