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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan 

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Thursday, March 31, 2022 3:06 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang <Henry.Wang@xxxxxxx>;
> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; 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
> 
> On 31.03.2022 08:13, Penny Zheng wrote:
> > Hi Jan
> >
> >> -----Original Message-----
> >> From: Jan Beulich <jbeulich@xxxxxxxx>
> >> Sent: Wednesday, March 30, 2022 5:53 PM
> >> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> >> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Henry Wang
> <Henry.Wang@xxxxxxx>;
> >> Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; George Dunlap
> >> <george.dunlap@xxxxxxxxxx>; Julien Grall <julien@xxxxxxx>; 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
> >>
> >> On 30.03.2022 11:36, Penny Zheng wrote:
> >>> --- a/xen/common/memory.c
> >>> +++ b/xen/common/memory.c
> >>> @@ -35,6 +35,10 @@
> >>>  #include <asm/guest.h>
> >>>  #endif
> >>>
> >>> +#ifndef is_domain_on_static_allocation #define
> >>> +is_domain_on_static_allocation(d) 0
> >>
> >> Nit: "false", not "0".
> >>
> >>> @@ -405,13 +409,29 @@ int guest_remove_page(struct domain *d,
> >> unsigned long gmfn)
> >>>       * device must retrieve the same pfn when the hypercall
> >> populate_physmap
> >>>       * is called.
> >>>       *
> >>> +     * When domain on static allocation, they should always get
> >>> + pages from
> >> the
> >>> +     * reserved static region when the hypercall populate_physmap is
> called.
> >>> +     *
> >>>       * For this purpose (and to match populate_physmap() behavior), the
> page
> >>>       * is kept allocated.
> >>>       */
> >>> -    if ( !rc && !is_domain_direct_mapped(d) )
> >>> +    if ( !rc && !(is_domain_direct_mapped(d) ||
> >>> +                  is_domain_on_static_allocation(d)) )
> >>>          put_page_alloc_ref(page);
> >>>
> >>>      put_page(page);
> >>> +#ifdef CONFIG_STATIC_MEMORY
> >>> +    /*
> >>> +     * When domain on static allocation, we shall store pages to
> >> resv_page_list,
> >>> +     * so the hypercall populate_physmap could retrieve pages from it,
> >>> +     * rather than allocating from heap.
> >>> +     */
> >>> +    if ( is_domain_on_static_allocation(d) )
> >>> +    {
> >>> +        page_list_add_tail(page, &d->resv_page_list);
> >>> +        d->resv_pages++;
> >>> +    }
> >>> +#endif
> >>
> >> I think this is wrong, as a result of not integrating with put_page().
> >> The page should only go on that list once its last ref was dropped. I
> >> don't recall for sure, but iirc staticmem pages are put on the
> >> domain's page list just like other pages would be. But then you also
> >> corrupt the list when this isn't the last ref which is put.
> >
> > Yes, staticmem pages are put on the domain's page list.
> > Here, I tried to only destroy the P2M mapping, and keep the page still
> > allocated to this domain.
> 
> Well, much depends on what you call "allocated". For populate_physmap you
> then take pages from resv_page_list. So I'd like to distinguish "allocated" 
> from
> "reserved": The pages are allocated to the domain when they're on the normal
> page list; they're reserved when they're on the new list you introduce. And
> what you want to initiate here is an "allocated" -> "reserved" transition.
> 
> > resv_page_list is just providing an easy way to track down the unpopulated
> memory.
> > '''
> > But then you also corrupt the list when this isn't the last ref which
> > is put.
> > '''
> > I'm sorry, would you like to be more specific on this comment?
> > I want these pages to linked in the domain's page list, then it could
> > be freed properly when domain get destroyed through relinquish_memory.
> 
> Clearly they can't be on both lists. Hence you can put them on the new list
> only _after_ having taken them off the "normal" list. That "taking off the
> normal list" should happen when the last ref is dropped, not here - see
> free_domheap_pages()'s uses of arch_free_heap_page(), recalling that
> free_domheap_page() is what
> put_page() calls upon dropping the last ref.
> 

Right, right, I've missed the critical point "they can't be on both lists".
Here is a thing, put_page is a very common helper that it is also beening
used when freeing memory on domain deconstruction. At that time, I
don't want to put these pages in resv_page_list, I'd like them to be
freed to the heap. This putting pages in resv_page_list thing is only for
unpopulating memory on the runtime. So I'd suggest introducing a
new helper put_pages_resv(...) to do the work.
 
About before you mentioned that "The pages are allocated to the
domain when they're on the normal page list; they're reserved when
they're on the new list you introduce. " Is there a possibility that I
still keep the pages allocated but just moving them from normal page list
to the new resv_page_list? Of course, a few extra things needed to be
covered, like domain_adjust_tot_pages, scrubing the pages. 

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. Later when populating them, we shall do the
exact in backwards, setting the owner back to the domain, changing the
state from free back to in_use, which seems a bit useless. And actually
for domain on static allocation, these staticmem pages are reserved
from the very beginning, and when it is allocated to the domain, it
forever belongs to the domain, and it could never be used in any other way.
 
Later when populating the memory, we could just move the pages from
resv_page_list back to the normal list, and also domain_adjust_tot_pages.

Another thing I'd consider to be affected is that when domain is dying, and
doing relinquish_memory, I need extra relinquishing for pages in resv_page_list.

> Jan