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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, June 29, 2022 1:56 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> 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; Julien Grall <julien@xxxxxxx>
> Subject: Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is
> static
> 
> 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.

If you don't prefer let free_domheap_pages to call free_domstatic_page, then, 
maybe
the if-array should happen at put_page
"
@@ -1622,6 +1622,8 @@ void put_page(struct page_info *page)

     if ( unlikely((nx & PGC_count_mask) == 0) )
     {
+        if ( unlikely(page->count_info & PGC_static) )
+            free_domstatic_page(page);
         free_domheap_page(page);
     }
 }
"
Wdyt now?
 
> 
> Jan