[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v7 7/9] xen/arm: unpopulate memory when domain is static
On 29.06.2022 08:08, Penny Zheng wrote: >> From: Jan Beulich <jbeulich@xxxxxxxx> >> Sent: Wednesday, June 29, 2022 1:56 PM >> >> On 29.06.2022 05:12, Penny Zheng wrote: >>>> From: Julien Grall <julien@xxxxxxx> >>>> Sent: Monday, June 27, 2022 6:19 PM >>>> >>>> On 27/06/2022 11:03, Penny Zheng wrote: >>>>>> -----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? Personally I'd prefer this variant, but we'll have to see what Julien or the other Arm maintainers think. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |