[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Penny Zheng <Penny.Zheng@xxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Mon, 4 Apr 2022 13:33:00 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=2vninudnNY5iyLT5yME4S0Ywt/TnpeHxiHfTBgQ6Hl0=; b=O/eN1nUmYhHFU8rW0SoLU3sGIf7hWsRur6SQhEYv0OE4IRpkB6qUd22k+G4d7naufm5F6P29K13xZNI4vshLfHgMNtmhYW52yUmHcPT8GQ1sl1uOyLl6Gi0+3zb1poFdbsLt6LrFC+QmYFkU3SnYXzNbjwWvq0e+2RYUktuSzCdAlUoazFWIF2BKCgsv9a3XO2HSyiMsh/pEj5DJi1KNgOPE727K27Ee2xoTqelfYLcGKfy9PWNuv6SHkiIu9ro/cLGbqLlSVvnXFVfKvHMTW/dRs8P3QVoMrpub/QvJ+D3qwO/tKWuR3xj402F0QzM3BRdWF5xxQIgpEYBwE+Wqmw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TLkUryBJGZ+mCLMNh8hCvKmDHeORW+19SkfTkid8Smkwstw9D/THdqMdh4YUSgrfwreyuLI4nwdwwMH+4rRc7s9r2/EXVuuwv5XRNsEe1D7SKXCsLT+iQiK2RAFQhO/JbcRzWoaPjBeBJ5eY30qJCaMe7fHgwUVET9R1Ofk6UliMIZlhf8wN6I8AbmF3eQ91Tor9AoXprBTMgdkTN3ngzVXpWCNzZYc27/J7jW5GByk//49HZ/YqJ7BfyJEqn3wKZ7k1VJpWIkBiiCAj84Hk268XRYtIcVFqpnQJmjV/0uJiEWSx2zLxFgSSdmB2Z1O68P3pCNZnZq9PJlFpXfSb3A==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • 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" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Mon, 04 Apr 2022 11:33:19 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.