On 31.03.2022 12:30, Penny Zheng wrote:
> Hi Jan
>
>> -----Original Message-----
>> From: Jan Beulich <jbeul...@suse.com>
>> Sent: Thursday, March 31, 2022 3:06 PM
>> To: Penny Zheng <penny.zh...@arm.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>; Julien Grall <jul...@xen.org>; Stefano Stabellini
>> <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; xen-
>> de...@lists.xenproject.org
>> 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 <jbeul...@suse.com>
>>>> Sent: Wednesday, March 30, 2022 5:53 PM
>>>> To: Penny Zheng <penny.zh...@arm.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>; Julien Grall <jul...@xen.org>; Stefano
>>>> Stabellini <sstabell...@kernel.org>; Wei Liu <w...@xen.org>; xen-
>>>> de...@lists.xenproject.org
>>>> 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
