Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.06.2022 05:12, Penny Zheng wrote:
> Hi Julien and Jan
> 
>> -----Original Message-----
>> From: Julien Grall <julien@xxxxxxx>
>> Sent: Monday, June 27, 2022 6:19 PM
>> To: Penny Zheng <Penny.Zheng@xxxxxxx>; Jan Beulich <jbeulich@xxxxxxxx>
>> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
>> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
>> Stefano Stabellini <sstabellini@xxxxxxxxxx>; Wei Liu <wl@xxxxxxx>; xen-
>> devel@xxxxxxxxxxxxxxxxxxxx
>> Subject: Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is
>> static
>>
>>
>>
>> On 27/06/2022 11:03, Penny Zheng wrote:
>>> Hi jan
>>>
>>>> -----Original Message-----
>>> put_static_pages, that is, adding pages to the reserved list, is only
>>> for freeing static pages on runtime. In static page initialization
>>> stage, I also use free_statimem_pages, and in which stage, I think the
>>> domain has not been constructed at all. So I prefer the freeing of
>>> staticmem pages is split into two parts: free_staticmem_pages and
>>> put_static_pages
>>
>> AFAIU, all the pages would have to be allocated via
>> acquire_domstatic_pages(). This call requires the domain to be allocated and
>> setup for handling memory.
>>
>> Therefore, I think the split is unnecessary. This would also have the
>> advantage to remove one loop. Admittly, this is not important when the
>> order 0, but it would become a problem for larger order (you may have to
>> pull the struct page_info multiple time in the cache).
>>
> 
> How about this:
> I create a new func free_domstatic_page, and it will be like:
> "
> static void free_domstatic_page(struct domain *d, struct page_info *page)
> {
>     unsigned int i;
>     bool need_scrub;
> 
>     /* NB. May recursively lock from relinquish_memory(). */
>     spin_lock_recursive(&d->page_alloc_lock);
> 
>     arch_free_heap_page(d, page);
> 
>     /*
>      * Normally we expect a domain to clear pages before freeing them,
>      * if it cares about the secrecy of their contents. However, after
>      * a domain has died we assume responsibility for erasure. We do
>      * scrub regardless if option scrub_domheap is set.
>      */
>     need_scrub = d->is_dying || scrub_debug || opt_scrub_domheap;
> 
>     free_staticmem_pages(page, 1, need_scrub);
> 
>     /* Add page on the resv_page_list *after* it has been freed. */
>     put_static_page(d, page);
> 
>     drop_dom_ref = !domain_adjust_tot_pages(d, -1);
> 
>     spin_unlock_recursive(&d->page_alloc_lock);
> 
>     if ( drop_dom_ref )
>         put_domain(d);
> }
> "
> 
> In free_domheap_pages, we just call free_domstatic_page:
> 
> "
> @@ -2430,6 +2430,9 @@ void free_domheap_pages(struct page_info *pg, unsigned 
> int order)
> 
>      ASSERT_ALLOC_CONTEXT();
> 
> +    if ( unlikely(pg->count_info & PGC_static) )
> +        return free_domstatic_page(d, pg);
> +
>      if ( unlikely(is_xen_heap_page(pg)) )
>      {
>          /* NB. May recursively lock from relinquish_memory(). */
> @@ -2673,6 +2676,38 @@ void free_staticmem_pages(struct page_info *pg, 
> unsigned long nr_mfns,
> "
> 
> Then the split could be avoided and we could save the loop as much as 
> possible.
> Any suggestion? 

Looks reasonable at the first glance (will need to see it in proper
context for a final opinion), provided e.g. Xen heap pages can never
be static.

Jan