| Message ID | 20220427092743.925563-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | populate/unpopulate memory when domain on static | expand |
Hi Penny, On 27/04/2022 10:27, 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. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > 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/arch/arm/include/asm/mm.h | 12 ++++++++++++ > xen/common/domain.c | 4 ++++ > xen/include/xen/sched.h | 3 +++ > 3 files changed, 19 insertions(+) > > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h > index 424aaf2823..c6426c1705 100644 > --- a/xen/arch/arm/include/asm/mm.h > +++ b/xen/arch/arm/include/asm/mm.h > @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info *page); > > unsigned int arch_get_dma_bitsize(void); > > +/* > + * Put free pages on the resv page list after having taken them > + * off the "normal" page list, when pages from static memory > + */ > +#ifdef CONFIG_STATIC_MEMORY > +#define arch_free_heap_page(d, pg) ({ \ > + page_list_del(pg, page_to_list(d, pg)); \ > + if ( (pg)->count_info & PGC_reserved ) \ > + page_list_add_tail(pg, &(d)->resv_page_list); \ > +}) > +#endif I am a bit puzzled how this is meant to work. Looking at the code, arch_free_heap_page() will be called from free_domheap_pages(). If I am not mistaken, reserved pages are not considered as xen heap pages, so we would go in the else which will end up to call free_heap_pages(). free_heap_pages() will end up to add the page in the heap allocator and corrupt the d->resv_page_list because there are only one link list. What did I miss? Cheers,
Hi julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Wednesday, April 27, 2022 6:11 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; > Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; > Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is > static > > Hi Penny, > > On 27/04/2022 10:27, 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. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > 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/arch/arm/include/asm/mm.h | 12 ++++++++++++ > > xen/common/domain.c | 4 ++++ > > xen/include/xen/sched.h | 3 +++ > > 3 files changed, 19 insertions(+) > > > > diff --git a/xen/arch/arm/include/asm/mm.h > > b/xen/arch/arm/include/asm/mm.h index 424aaf2823..c6426c1705 100644 > > --- a/xen/arch/arm/include/asm/mm.h > > +++ b/xen/arch/arm/include/asm/mm.h > > @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info > > *page); > > > > unsigned int arch_get_dma_bitsize(void); > > > > +/* > > + * Put free pages on the resv page list after having taken them > > + * off the "normal" page list, when pages from static memory */ > > +#ifdef CONFIG_STATIC_MEMORY > > +#define arch_free_heap_page(d, pg) ({ \ > > + page_list_del(pg, page_to_list(d, pg)); \ > > + if ( (pg)->count_info & PGC_reserved ) \ > > + page_list_add_tail(pg, &(d)->resv_page_list); \ > > +}) > > +#endif > > I am a bit puzzled how this is meant to work. > > Looking at the code, arch_free_heap_page() will be called from > free_domheap_pages(). If I am not mistaken, reserved pages are not > considered as xen heap pages, so we would go in the else which will end up to > call free_heap_pages(). > > free_heap_pages() will end up to add the page in the heap allocator and > corrupt the d->resv_page_list because there are only one link list. > > What did I miss? > In my first commit "do not free reserved memory into heap", I've changed the behavior for reserved pages in free_heap_pages() + if ( pg->count_info & PGC_reserved ) + /* Reserved page shall not go back to the heap. */ + return free_staticmem_pages(pg, 1UL << order, need_scrub); + > Cheers, > > -- > Julien Grall Cheers, -- Penny Zheng
Hi Penny, On 27/04/2022 11:19, Penny Zheng wrote: >>> +/* >>> + * Put free pages on the resv page list after having taken them >>> + * off the "normal" page list, when pages from static memory */ >>> +#ifdef CONFIG_STATIC_MEMORY >>> +#define arch_free_heap_page(d, pg) ({ \ >>> + page_list_del(pg, page_to_list(d, pg)); \ >>> + if ( (pg)->count_info & PGC_reserved ) \ >>> + page_list_add_tail(pg, &(d)->resv_page_list); \ >>> +}) >>> +#endif >> >> I am a bit puzzled how this is meant to work. >> >> Looking at the code, arch_free_heap_page() will be called from >> free_domheap_pages(). If I am not mistaken, reserved pages are not >> considered as xen heap pages, so we would go in the else which will end up to >> call free_heap_pages(). >> >> free_heap_pages() will end up to add the page in the heap allocator and >> corrupt the d->resv_page_list because there are only one link list. >> >> What did I miss? >> > > In my first commit "do not free reserved memory into heap", I've changed the behavior > for reserved pages in free_heap_pages() > + if ( pg->count_info & PGC_reserved ) > + /* Reserved page shall not go back to the heap. */ > + return free_staticmem_pages(pg, 1UL << order, need_scrub); > + Hmmm... somehow this e-mail is neither in my inbox nor in the archives on lore.kernel.org. Cheers,
Hi julien > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: Wednesday, April 27, 2022 6:23 PM > To: Penny Zheng <Penny.Zheng@arm.com>; xen-devel@lists.xenproject.org > Cc: Wei Chen <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; > Stefano Stabellini <sstabellini@kernel.org>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; > Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org> > Subject: Re: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is > static > > Hi Penny, > > On 27/04/2022 11:19, Penny Zheng wrote: > >>> +/* > >>> + * Put free pages on the resv page list after having taken them > >>> + * off the "normal" page list, when pages from static memory */ > >>> +#ifdef CONFIG_STATIC_MEMORY > >>> +#define arch_free_heap_page(d, pg) ({ \ > >>> + page_list_del(pg, page_to_list(d, pg)); \ > >>> + if ( (pg)->count_info & PGC_reserved ) \ > >>> + page_list_add_tail(pg, &(d)->resv_page_list); \ > >>> +}) > >>> +#endif > >> > >> I am a bit puzzled how this is meant to work. > >> > >> Looking at the code, arch_free_heap_page() will be called from > >> free_domheap_pages(). If I am not mistaken, reserved pages are not > >> considered as xen heap pages, so we would go in the else which will > >> end up to call free_heap_pages(). > >> > >> free_heap_pages() will end up to add the page in the heap allocator > >> and corrupt the d->resv_page_list because there are only one link list. > >> > >> What did I miss? > >> > > > > In my first commit "do not free reserved memory into heap", I've > > changed the behavior for reserved pages in free_heap_pages() > > + if ( pg->count_info & PGC_reserved )that > > + /* Reserved page shall not go back to the heap. */ > > + return free_staticmem_pages(pg, 1UL << order, need_scrub); > > + > > Hmmm... somehow this e-mail is neither in my inbox nor in the archives on > lore.kernel.org. > Oh.... I just got email from tessian that they held my first commit, and needed my confirmation to send. So sorry about that!!! I'll re-send my first commit ASAP. > Cheers, > -- > Julien Grall
On Wed, 27 Apr 2022, Penny Zheng wrote: > > Hi Penny, > > > > On 27/04/2022 11:19, Penny Zheng wrote: > > >>> +/* > > >>> + * Put free pages on the resv page list after having taken them > > >>> + * off the "normal" page list, when pages from static memory */ > > >>> +#ifdef CONFIG_STATIC_MEMORY > > >>> +#define arch_free_heap_page(d, pg) ({ \ > > >>> + page_list_del(pg, page_to_list(d, pg)); \ > > >>> + if ( (pg)->count_info & PGC_reserved ) \ > > >>> + page_list_add_tail(pg, &(d)->resv_page_list); \ > > >>> +}) > > >>> +#endif > > >> > > >> I am a bit puzzled how this is meant to work. > > >> > > >> Looking at the code, arch_free_heap_page() will be called from > > >> free_domheap_pages(). If I am not mistaken, reserved pages are not > > >> considered as xen heap pages, so we would go in the else which will > > >> end up to call free_heap_pages(). > > >> > > >> free_heap_pages() will end up to add the page in the heap allocator > > >> and corrupt the d->resv_page_list because there are only one link list. > > >> > > >> What did I miss? > > >> > > > > > > In my first commit "do not free reserved memory into heap", I've > > > changed the behavior for reserved pages in free_heap_pages() > > > + if ( pg->count_info & PGC_reserved )that > > > + /* Reserved page shall not go back to the heap. */ > > > + return free_staticmem_pages(pg, 1UL << order, need_scrub); > > > + > > > > Hmmm... somehow this e-mail is neither in my inbox nor in the archives on > > lore.kernel.org. > > > > Oh.... I just got email from tessian that they held my first commit, and needed my > confirmation to send. So sorry about that!!! > > I'll re-send my first commit ASAP. Just FYI I still cannot see the first patch anywhere in my inbox
Hi > -----Original Message----- > From: Stefano Stabellini <sstabellini@kernel.org> > Sent: Thursday, April 28, 2022 6:32 AM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Julien Grall <julien@xen.org>; xen-devel@lists.xenproject.org; Wei Chen > <Wei.Chen@arm.com>; Henry Wang <Henry.Wang@arm.com>; Stefano > Stabellini <sstabellini@kernel.org>; Bertrand Marquis > <Bertrand.Marquis@arm.com>; Volodymyr Babchuk > <Volodymyr_Babchuk@epam.com>; Andrew Cooper > <andrew.cooper3@citrix.com>; George Dunlap <george.dunlap@citrix.com>; > Jan Beulich <jbeulich@suse.com>; Wei Liu <wl@xen.org> > Subject: RE: [PATCH v3 5/6] xen/arm: unpopulate memory when domain is > static > > On Wed, 27 Apr 2022, Penny Zheng wrote: > > > Hi Penny, > > > > > > On 27/04/2022 11:19, Penny Zheng wrote: > > > >>> +/* > > > >>> + * Put free pages on the resv page list after having taken them > > > >>> + * off the "normal" page list, when pages from static memory > > > >>> +*/ #ifdef CONFIG_STATIC_MEMORY > > > >>> +#define arch_free_heap_page(d, pg) ({ \ > > > >>> + page_list_del(pg, page_to_list(d, pg)); \ > > > >>> + if ( (pg)->count_info & PGC_reserved ) \ > > > >>> + page_list_add_tail(pg, &(d)->resv_page_list); \ > > > >>> +}) > > > >>> +#endif > > > >> > > > >> I am a bit puzzled how this is meant to work. > > > >> > > > >> Looking at the code, arch_free_heap_page() will be called from > > > >> free_domheap_pages(). If I am not mistaken, reserved pages are > > > >> not considered as xen heap pages, so we would go in the else > > > >> which will end up to call free_heap_pages(). > > > >> > > > >> free_heap_pages() will end up to add the page in the heap > > > >> allocator and corrupt the d->resv_page_list because there are only one > link list. > > > >> > > > >> What did I miss? > > > >> > > > > > > > > In my first commit "do not free reserved memory into heap", I've > > > > changed the behavior for reserved pages in free_heap_pages() > > > > + if ( pg->count_info & PGC_reserved )that > > > > + /* Reserved page shall not go back to the heap. */ > > > > + return free_staticmem_pages(pg, 1UL << order, > > > > + need_scrub); > > > > + > > > > > > Hmmm... somehow this e-mail is neither in my inbox nor in the > > > archives on lore.kernel.org. > > > > > > > Oh.... I just got email from tessian that they held my first commit, > > and needed my confirmation to send. So sorry about that!!! > > > > I'll re-send my first commit ASAP. > > Just FYI I still cannot see the first patch anywhere in my inbox So sorry about the mess again... I've resend it just now, PLZ check https://patchwork.kernel.org/project/xen-devel/patch/20220428030127.998670-1-Penny.Zheng@arm.com/
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 424aaf2823..c6426c1705 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -358,6 +358,18 @@ void clear_and_clean_page(struct page_info *page); unsigned int arch_get_dma_bitsize(void); +/* + * Put free pages on the resv page list after having taken them + * off the "normal" page list, when pages from static memory + */ +#ifdef CONFIG_STATIC_MEMORY +#define arch_free_heap_page(d, pg) ({ \ + page_list_del(pg, page_to_list(d, pg)); \ + if ( (pg)->count_info & PGC_reserved ) \ + page_list_add_tail(pg, &(d)->resv_page_list); \ +}) +#endif + #endif /* __ARCH_ARM_MM__ */ /* * Local variables: diff --git a/xen/common/domain.c b/xen/common/domain.c index 6373407047..13fe7cecff 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/include/xen/sched.h b/xen/include/xen/sched.h index 49415a113a..368e5c1c53 100644 --- a/xen/include/xen/sched.h +++ b/xen/include/xen/sched.h @@ -376,6 +376,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 (size resv_pages) */ +#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> --- 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/arch/arm/include/asm/mm.h | 12 ++++++++++++ xen/common/domain.c | 4 ++++ xen/include/xen/sched.h | 3 +++ 3 files changed, 19 insertions(+)