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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

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? 

> Cheers,
> 
> --
> Julien Grall