| Message ID | 20220418122251.3583049-6-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | populate/unpopulate memory when domain on static | expand |
On 18.04.2022 14:22, Penny Zheng wrote: > --- a/xen/arch/arm/include/asm/mm.h > +++ b/xen/arch/arm/include/asm/mm.h > @@ -358,6 +358,23 @@ 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) { \ > + if ( (pg)->count_info & PGC_reserved ) \ > + { \ > + page_list_del(pg, page_to_list(d, pg)); \ > + page_list_add_tail(pg, &(d)->resv_page_list); \ > + (d)->resv_pages++; \ There's no consumer of this counter, so I'd like to ask that it be introduced once a consumer appears. > + } \ > + else \ > + page_list_del(pg, page_to_list(d, pg)); \ Is there a particular reason to have this page_list_del() twice, instead of just once ahead of the if()? > +} Also this entire construct want to be an expression, not a (compound) statement. And it probably would better evaluate its parameters just once. Jan
Hi jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, April 19, 2022 5:11 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Julien Grall <julien@xen.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>; > Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is > static > > On 18.04.2022 14:22, Penny Zheng wrote: > > --- a/xen/arch/arm/include/asm/mm.h > > +++ b/xen/arch/arm/include/asm/mm.h > > @@ -358,6 +358,23 @@ 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) { \ > > + if ( (pg)->count_info & PGC_reserved ) \ > > + { \ > > + page_list_del(pg, page_to_list(d, pg)); \ > > + page_list_add_tail(pg, &(d)->resv_page_list); \ > > + (d)->resv_pages++; \ > > There's no consumer of this counter, so I'd like to ask that it be introduced > once a consumer appears. > > > + } \ > > + else \ > > + page_list_del(pg, page_to_list(d, pg)); \ > > Is there a particular reason to have this page_list_del() twice, instead of just > once ahead of the if()? > > > +} > > Also this entire construct want to be an expression, not a > (compound) statement. And it probably would better evaluate its parameters > just once. > #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); \ } I'm trying to refine the arch_free_heap_page() here, but I'm a bit confused about to let it be an expression, not a compound statement. Do you mean that you prefer to let the if-clause out of the arch_free_heap_page()? > Jan
On 25.04.2022 08:34, Penny Zheng wrote: >> -----Original Message----- >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, April 19, 2022 5:11 PM >> >> On 18.04.2022 14:22, Penny Zheng wrote: >>> --- a/xen/arch/arm/include/asm/mm.h >>> +++ b/xen/arch/arm/include/asm/mm.h >>> @@ -358,6 +358,23 @@ 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) { \ >>> + if ( (pg)->count_info & PGC_reserved ) \ >>> + { \ >>> + page_list_del(pg, page_to_list(d, pg)); \ >>> + page_list_add_tail(pg, &(d)->resv_page_list); \ >>> + (d)->resv_pages++; \ >> >> There's no consumer of this counter, so I'd like to ask that it be introduced >> once a consumer appears. >> >>> + } \ >>> + else \ >>> + page_list_del(pg, page_to_list(d, pg)); \ >> >> Is there a particular reason to have this page_list_del() twice, instead of just >> once ahead of the if()? >> >>> +} >> >> Also this entire construct want to be an expression, not a >> (compound) statement. And it probably would better evaluate its parameters >> just once. >> > > #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); \ > } > > I'm trying to refine the arch_free_heap_page() here, but I'm a bit confused > about to let it be an expression, not a compound statement. Do you mean that > you prefer to let the if-clause out of the arch_free_heap_page()? No. You want to put parentheses around the braces, using a gcc extension we make extensive use of throughout the code base. Jan
Hi, jan > -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Monday, April 25, 2022 4:01 PM > To: Penny Zheng <Penny.Zheng@arm.com> > Cc: Wei Chen <Wei.Chen@arm.com>; Stefano Stabellini > <sstabellini@kernel.org>; Julien Grall <julien@xen.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>; > Wei Liu <wl@xen.org>; xen-devel@lists.xenproject.org > Subject: Re: [PATCH v2 5/6] xen/arm: unpopulate memory when domain is > static > > On 25.04.2022 08:34, Penny Zheng wrote: > >> -----Original Message----- > >> From: Jan Beulich <jbeulich@suse.com> > >> Sent: Tuesday, April 19, 2022 5:11 PM > >> > >> On 18.04.2022 14:22, Penny Zheng wrote: > >>> --- a/xen/arch/arm/include/asm/mm.h > >>> +++ b/xen/arch/arm/include/asm/mm.h > >>> @@ -358,6 +358,23 @@ 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) { \ > >>> + if ( (pg)->count_info & PGC_reserved ) \ > >>> + { \ > >>> + page_list_del(pg, page_to_list(d, pg)); \ > >>> + page_list_add_tail(pg, &(d)->resv_page_list); \ > >>> + (d)->resv_pages++; \ > >> > >> There's no consumer of this counter, so I'd like to ask that it be > >> introduced once a consumer appears. > >> > >>> + } \ > >>> + else \ > >>> + page_list_del(pg, page_to_list(d, pg)); \ > >> > >> Is there a particular reason to have this page_list_del() twice, > >> instead of just once ahead of the if()? > >> > >>> +} > >> > >> Also this entire construct want to be an expression, not a > >> (compound) statement. And it probably would better evaluate its > >> parameters just once. > >> > > > > #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); \ > > } > > > > I'm trying to refine the arch_free_heap_page() here, but I'm a bit > > confused about to let it be an expression, not a compound statement. > > Do you mean that you prefer to let the if-clause out of the > arch_free_heap_page()? > > No. You want to put parentheses around the braces, using a gcc extension we > make extensive use of throughout the code base. > Oh, oh, thanks! put parentheses around the braces, then that's what you said about make it be an expression > Jan
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 424aaf2823..a2dde01cfa 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -358,6 +358,23 @@ 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) { \ + if ( (pg)->count_info & PGC_reserved ) \ + { \ + page_list_del(pg, page_to_list(d, pg)); \ + page_list_add_tail(pg, &(d)->resv_page_list); \ + (d)->resv_pages++; \ + } \ + else \ + page_list_del(pg, page_to_list(d, pg)); \ +} +#endif + #endif /* __ARCH_ARM_MM__ */ /* * Local variables: diff --git a/xen/common/domain.c b/xen/common/domain.c index 859cc13d3b..b499cf8d2c 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -605,6 +605,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 68eb08058e..85ef378752 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() @@ -395,6 +398,9 @@ struct domain #ifdef CONFIG_MEM_PAGING atomic_t paged_pages; /* paged-out pages */ #endif +#ifdef CONFIG_STATIC_MEMORY + unsigned int resv_pages; /* reserved pages from static region. */ +#endif /* Scheduling. */ void *sched_priv; /* scheduler-specific data */
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> --- v2 changes: - put reserved pages on resv_page_list after having taken them off the "normal" list --- xen/arch/arm/include/asm/mm.h | 17 +++++++++++++++++ xen/common/domain.c | 4 ++++ xen/include/xen/sched.h | 6 ++++++ 3 files changed, 27 insertions(+)