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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2022 12:30, Penny Zheng wrote:
> 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.

I'd like to suggest to avoid introducing special case helpers as much
as possible. put_page() would better remain the single, common
function to use when dropping a ref. Any special treatment for certain
kinds of pages should happen without the callers needing to care.

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

It's all a matter of definition. What I said (and what you quoted) was
merely based on my understanding of your intentions.

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

I don't mind the particular model you want to implement. What I do care
about is that the end result is consistent within itself, with as little
special casing (potentially) all over the code base as possible.

Jan