| Message ID | 20220720054611.2695787-7-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | populate/unpopulate memory when domain on static allocation | expand |
On 20.07.2022 07:46, Penny Zheng wrote: > Today when a domain unpopulates the memory on runtime, they will always > hand the memory back to the heap allocator. And it will be a problem if domain > is static. > > Pages as guest RAM for static domain shall be reserved to only this domain > and not be used for any other purposes, so they shall never go back to heap > allocator. > > This commit puts reserved pages on the new list resv_page_list only after > having taken them off the "normal" list, when the last ref dropped. I guess this wording somehow relates to ... > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -2674,10 +2674,14 @@ void free_domstatic_page(struct page_info *page) > > drop_dom_ref = !domain_adjust_tot_pages(d, -1); > > - spin_unlock_recursive(&d->page_alloc_lock); > - > free_staticmem_pages(page, 1, scrub_debug); > > + /* Add page on the resv_page_list *after* it has been freed. */ > + if ( !drop_dom_ref ) > + page_list_add_tail(page, &d->resv_page_list); ... the conditional used here. I cannot, however, figure why there is this conditional (and said part of the description also doesn't help me figure it out). As an aside: A title prefix of xen/arm: suggests you're mostly touching Arm code. But you're touching exclusively common code here. I for one would almost have skipped this patch (more than once) when deciding which ones (may) want me looking at them. Jan
Hi Jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, July 25, 2022 11:36 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; > Julien Grall <julien@xen.org>; Stefano Stabellini <sstabellini@kernel.org>; > Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v9 6/8] xen/arm: unpopulate memory when domain is > static > > On 20.07.2022 07:46, Penny Zheng wrote: > > Today when a domain unpopulates the memory on runtime, they will > > always hand the memory back to the heap allocator. And it will be a > > problem if domain is static. > > > > Pages as guest RAM for static domain shall be reserved to only this > > domain and not be used for any other purposes, so they shall never go > > back to heap allocator. > > > > This commit puts reserved pages on the new list resv_page_list only > > after having taken them off the "normal" list, when the last ref dropped. > > I guess this wording somehow relates to ... > > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -2674,10 +2674,14 @@ void free_domstatic_page(struct page_info > > *page) > > > > drop_dom_ref = !domain_adjust_tot_pages(d, -1); > > > > - spin_unlock_recursive(&d->page_alloc_lock); > > - > > free_staticmem_pages(page, 1, scrub_debug); > > > > + /* Add page on the resv_page_list *after* it has been freed. */ > > + if ( !drop_dom_ref ) > > + page_list_add_tail(page, &d->resv_page_list); > > ... the conditional used here. I cannot, however, figure why there is this > conditional (and said part of the description also doesn't help me figure it > out). > I was thinking that if drop_dom_ref true, then later the whole domain struct will be released, then there is no need to add the page to d->resv_page_list > As an aside: A title prefix of xen/arm: suggests you're mostly touching Arm > code. But you're touching exclusively common code here. > I for one would almost have skipped this patch (more than once) when > deciding which ones (may) want me looking at them. > Sorry for that, I’ll fix it > Jan
On 26.07.2022 04:57, Penny Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Monday, July 25, 2022 11:36 PM >> >> On 20.07.2022 07:46, Penny Zheng wrote: >>> Today when a domain unpopulates the memory on runtime, they will >>> always hand the memory back to the heap allocator. And it will be a >>> problem if domain is static. >>> >>> Pages as guest RAM for static domain shall be reserved to only this >>> domain and not be used for any other purposes, so they shall never go >>> back to heap allocator. >>> >>> This commit puts reserved pages on the new list resv_page_list only >>> after having taken them off the "normal" list, when the last ref dropped. >> >> I guess this wording somehow relates to ... >> >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -2674,10 +2674,14 @@ void free_domstatic_page(struct page_info >>> *page) >>> >>> drop_dom_ref = !domain_adjust_tot_pages(d, -1); >>> >>> - spin_unlock_recursive(&d->page_alloc_lock); >>> - >>> free_staticmem_pages(page, 1, scrub_debug); >>> >>> + /* Add page on the resv_page_list *after* it has been freed. */ >>> + if ( !drop_dom_ref ) >>> + page_list_add_tail(page, &d->resv_page_list); >> >> ... the conditional used here. I cannot, however, figure why there is this >> conditional (and said part of the description also doesn't help me figure it >> out). >> > > I was thinking that if drop_dom_ref true, then later the whole domain struct > will be released, then there is no need to add the page to d->resv_page_list If that was the intention, then it should have been spelled out imo. But I think it's not a good idea to skip the list addition in that one special case, when keeping things uniform is actually cheaper _and_ more consistent. In the end I expect sooner or later there'll be a desire to restart DomU-s which have crashed, which would include re-using their designated static memory. Then you want to avoid leaking pages like it would happen with your approach. Jan
diff --git a/xen/common/domain.c b/xen/common/domain.c index 7062393e37..c23f449451 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -604,6 +604,10 @@ struct domain *domain_create(domid_t domid, INIT_PAGE_LIST_HEAD(&d->page_list); INIT_PAGE_LIST_HEAD(&d->extra_page_list); INIT_PAGE_LIST_HEAD(&d->xenpage_list); +#ifdef CONFIG_STATIC_MEMORY + INIT_PAGE_LIST_HEAD(&d->resv_page_list); +#endif + spin_lock_init(&d->node_affinity_lock); d->node_affinity = NODE_MASK_ALL; diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 45bd88a685..a568be55e3 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -2674,10 +2674,14 @@ void free_domstatic_page(struct page_info *page) drop_dom_ref = !domain_adjust_tot_pages(d, -1); - spin_unlock_recursive(&d->page_alloc_lock); - free_staticmem_pages(page, 1, scrub_debug); + /* Add page on the resv_page_list *after* it has been freed. */ + if ( !drop_dom_ref ) + page_list_add_tail(page, &d->resv_page_list); + + spin_unlock_recursive(&d->page_alloc_lock); + if ( drop_dom_ref ) put_domain(d); } diff --git a/xen/include/xen/sched.h b/xen/include/xen/sched.h index 98e8001c89..d4fbd3dea7 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -381,6 +381,9 @@ struct domain struct page_list_head page_list; /* linked list */ struct page_list_head extra_page_list; /* linked list (size extra_pages) */ struct page_list_head xenpage_list; /* linked list (size xenheap_pages) */ +#ifdef CONFIG_STATIC_MEMORY + struct page_list_head resv_page_list; /* linked list */ +#endif /* * This field should only be directly accessed by domain_adjust_tot_pages()
Today when a domain unpopulates the memory on runtime, they will always hand the memory back to the heap allocator. And it will be a problem if domain is static. Pages as guest RAM for static domain shall be reserved to only this domain and not be used for any other purposes, so they shall never go back to heap allocator. This commit puts reserved pages on the new list resv_page_list only after having taken them off the "normal" list, when the last ref dropped. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v9 change: - remove macro helper put_static_page, and just expand its code inside free_domstatic_page --- v8 changes: - adapt this patch for newly introduced free_domstatic_page - order as a parameter is not needed here, as all staticmem operations are limited to order-0 regions - move d->page_alloc_lock after operation on d->resv_page_list --- v7 changes: - Add page on the rsv_page_list *after* it has been freed --- v6 changes: - refine in-code comment - move PGC_static !CONFIG_STATIC_MEMORY definition to common header --- v5 changes: - adapt this patch for PGC_staticmem --- v4 changes: - no changes --- v3 changes: - have page_list_del() just once out of the if() - remove resv_pages counter - make arch_free_heap_page be an expression, not a compound statement. --- v2 changes: - put reserved pages on resv_page_list after having taken them off the "normal" list --- xen/common/domain.c | 4 ++++ xen/common/page_alloc.c | 8 ++++++-- xen/include/xen/sched.h | 3 +++ 3 files changed, 13 insertions(+), 2 deletions(-)