| Message ID | 20220531031241.90374-3-Penny.Zheng@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | populate/unpopulate memory when domain on static allocation | expand |
On 31.05.2022 05:12, Penny Zheng wrote: > Pages used as guest RAM for static domain, shall be reserved to this > domain only. > So in case reserved pages being used for other purpose, users > shall not free them back to heap, even when last ref gets dropped. > > free_staticmem_pages will be called by free_heap_pages in runtime > for static domain freeing memory resource, so let's drop the __init > flag. > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > --- > v5 changes: > - In order to avoid stub functions, we #define PGC_staticmem to non-zero only > when CONFIG_STATIC_MEMORY > - use "unlikely()" around pg->count_info & PGC_staticmem > - remove pointless "if", since mark_page_free() is going to set count_info > to PGC_state_free and by consequence clear PGC_staticmem > - move #define PGC_staticmem 0 to mm.h > --- > v4 changes: > - no changes > --- > v3 changes: > - fix possible racy issue in free_staticmem_pages() > - introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case > - move the change to free_heap_pages() to cover other potential call sites > - fix the indentation > --- > v2 changes: > - new commit > --- > xen/arch/arm/include/asm/mm.h | 2 ++ > xen/common/page_alloc.c | 16 +++++++++------- > xen/include/xen/mm.h | 6 +++++- > 3 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h > index 1226700085..56d0939318 100644 > --- a/xen/arch/arm/include/asm/mm.h > +++ b/xen/arch/arm/include/asm/mm.h > @@ -108,9 +108,11 @@ struct page_info > /* Page is Xen heap? */ > #define _PGC_xen_heap PG_shift(2) > #define PGC_xen_heap PG_mask(1, 2) > +#ifdef CONFIG_STATIC_MEMORY > /* Page is static memory */ > #define _PGC_staticmem PG_shift(3) > #define PGC_staticmem PG_mask(1, 3) > +#endif > /* ... */ > /* Page is broken? */ > #define _PGC_broken PG_shift(7) > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 44600dd9cd..6425761116 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -151,10 +151,6 @@ > #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL) > #endif > > -#ifndef PGC_staticmem > -#define PGC_staticmem 0 > -#endif > - Is the moving of this into the header really a necessary part of this change? Afaics the symbol is still only ever used in this one C file. > --- a/xen/include/xen/mm.h > +++ b/xen/include/xen/mm.h > @@ -85,10 +85,10 @@ bool scrub_free_pages(void); > } while ( false ) > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > -#ifdef CONFIG_STATIC_MEMORY > /* These functions are for static memory */ > void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, > bool need_scrub); > +#ifdef CONFIG_STATIC_MEMORY > int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns, > unsigned int memflags); > #endif Is the #ifdef really worth retaining at this point? Code is generally better readable without. Jan
> -----Original Message----- > From: Jan Beulich <jbeulich@suse.com> > Sent: Tuesday, May 31, 2022 4:37 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 v5 2/9] xen: do not free reserved memory into heap > > On 31.05.2022 05:12, Penny Zheng wrote: > > Pages used as guest RAM for static domain, shall be reserved to this > > domain only. > > So in case reserved pages being used for other purpose, users shall > > not free them back to heap, even when last ref gets dropped. > > > > free_staticmem_pages will be called by free_heap_pages in runtime for > > static domain freeing memory resource, so let's drop the __init flag. > > > > Signed-off-by: Penny Zheng <penny.zheng@arm.com> > > --- > > v5 changes: > > - In order to avoid stub functions, we #define PGC_staticmem to > > non-zero only when CONFIG_STATIC_MEMORY > > - use "unlikely()" around pg->count_info & PGC_staticmem > > - remove pointless "if", since mark_page_free() is going to set > > count_info to PGC_state_free and by consequence clear PGC_staticmem > > - move #define PGC_staticmem 0 to mm.h > > --- > > v4 changes: > > - no changes > > --- > > v3 changes: > > - fix possible racy issue in free_staticmem_pages() > > - introduce a stub free_staticmem_pages() for the > > !CONFIG_STATIC_MEMORY case > > - move the change to free_heap_pages() to cover other potential call > > sites > > - fix the indentation > > --- > > v2 changes: > > - new commit > > --- > > xen/arch/arm/include/asm/mm.h | 2 ++ > > xen/common/page_alloc.c | 16 +++++++++------- > > xen/include/xen/mm.h | 6 +++++- > > 3 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/xen/arch/arm/include/asm/mm.h > > b/xen/arch/arm/include/asm/mm.h index 1226700085..56d0939318 100644 > > --- a/xen/arch/arm/include/asm/mm.h > > +++ b/xen/arch/arm/include/asm/mm.h > > @@ -108,9 +108,11 @@ struct page_info > > /* Page is Xen heap? */ > > #define _PGC_xen_heap PG_shift(2) > > #define PGC_xen_heap PG_mask(1, 2) > > +#ifdef CONFIG_STATIC_MEMORY > > /* Page is static memory */ > > #define _PGC_staticmem PG_shift(3) > > #define PGC_staticmem PG_mask(1, 3) > > +#endif > > /* ... */ > > /* Page is broken? */ > > #define _PGC_broken PG_shift(7) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index > > 44600dd9cd..6425761116 100644 > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -151,10 +151,6 @@ > > #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL) > > #endif > > > > -#ifndef PGC_staticmem > > -#define PGC_staticmem 0 > > -#endif > > - > > Is the moving of this into the header really a necessary part of this change? > Afaics the symbol is still only ever used in this one C file. Later, in commit "xen/arm: unpopulate memory when domain is static", we will use this flag in xen/arch/arm/include/asm/mm.h > > --- a/xen/include/xen/mm.h > > +++ b/xen/include/xen/mm.h > > @@ -85,10 +85,10 @@ bool scrub_free_pages(void); } while ( false ) > > #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) > > > > -#ifdef CONFIG_STATIC_MEMORY > > /* These functions are for static memory */ void > > free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, > > bool need_scrub); > > +#ifdef CONFIG_STATIC_MEMORY > > int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int > nr_mfns, > > unsigned int memflags); #endif > > Is the #ifdef really worth retaining at this point? Code is generally better > readable without. > Sure, will remove > Jan
On 02.06.2022 04:18, Penny Zheng wrote: >> From: Jan Beulich <jbeulich@suse.com> >> Sent: Tuesday, May 31, 2022 4:37 PM >> >> On 31.05.2022 05:12, Penny Zheng wrote: >>> --- a/xen/common/page_alloc.c >>> +++ b/xen/common/page_alloc.c >>> @@ -151,10 +151,6 @@ >>> #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL) >>> #endif >>> >>> -#ifndef PGC_staticmem >>> -#define PGC_staticmem 0 >>> -#endif >>> - >> >> Is the moving of this into the header really a necessary part of this change? >> Afaics the symbol is still only ever used in this one C file. > > Later, in commit "xen/arm: unpopulate memory when domain is static", > we will use this flag in xen/arch/arm/include/asm/mm.h IOW you want to move this change there. Jan
diff --git a/xen/arch/arm/include/asm/mm.h b/xen/arch/arm/include/asm/mm.h index 1226700085..56d0939318 100644 --- a/xen/arch/arm/include/asm/mm.h +++ b/xen/arch/arm/include/asm/mm.h @@ -108,9 +108,11 @@ struct page_info /* Page is Xen heap? */ #define _PGC_xen_heap PG_shift(2) #define PGC_xen_heap PG_mask(1, 2) +#ifdef CONFIG_STATIC_MEMORY /* Page is static memory */ #define _PGC_staticmem PG_shift(3) #define PGC_staticmem PG_mask(1, 3) +#endif /* ... */ /* Page is broken? */ #define _PGC_broken PG_shift(7) diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 44600dd9cd..6425761116 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -151,10 +151,6 @@ #define p2m_pod_offline_or_broken_replace(pg) BUG_ON(pg != NULL) #endif -#ifndef PGC_staticmem -#define PGC_staticmem 0 -#endif - /* * Comma-separated list of hexadecimal page numbers containing bad bytes. * e.g. 'badpage=0x3f45,0x8a321'. @@ -1443,6 +1439,13 @@ static void free_heap_pages( ASSERT(order <= MAX_ORDER); + if ( unlikely(pg->count_info & PGC_staticmem) ) + { + /* Pages of static memory shall not go back to the heap. */ + free_staticmem_pages(pg, 1UL << order, need_scrub); + return; + } + spin_lock(&heap_lock); for ( i = 0; i < (1 << order); i++ ) @@ -2636,8 +2639,8 @@ struct domain *get_pg_owner(domid_t domid) #ifdef CONFIG_STATIC_MEMORY /* Equivalent of free_heap_pages to free nr_mfns pages of static memory. */ -void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, - bool need_scrub) +void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, + bool need_scrub) { mfn_t mfn = page_to_mfn(pg); unsigned long i; @@ -2652,7 +2655,6 @@ void __init free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, scrub_one_page(pg); } - /* In case initializing page of static memory, mark it PGC_staticmem. */ pg[i].count_info |= PGC_staticmem; } } diff --git a/xen/include/xen/mm.h b/xen/include/xen/mm.h index 3be754da92..ca2c6f033e 100644 --- a/xen/include/xen/mm.h +++ b/xen/include/xen/mm.h @@ -85,10 +85,10 @@ bool scrub_free_pages(void); } while ( false ) #define FREE_XENHEAP_PAGE(p) FREE_XENHEAP_PAGES(p, 0) -#ifdef CONFIG_STATIC_MEMORY /* These functions are for static memory */ void free_staticmem_pages(struct page_info *pg, unsigned long nr_mfns, bool need_scrub); +#ifdef CONFIG_STATIC_MEMORY int acquire_domstatic_pages(struct domain *d, mfn_t smfn, unsigned int nr_mfns, unsigned int memflags); #endif @@ -212,6 +212,10 @@ extern struct domain *dom_cow; #include <asm/mm.h> +#ifndef PGC_staticmem +#define PGC_staticmem 0 +#endif + static inline bool is_special_page(const struct page_info *page) { return is_xen_heap_page(page) || (page->count_info & PGC_extra);
Pages used as guest RAM for static domain, shall be reserved to this domain only. So in case reserved pages being used for other purpose, users shall not free them back to heap, even when last ref gets dropped. free_staticmem_pages will be called by free_heap_pages in runtime for static domain freeing memory resource, so let's drop the __init flag. Signed-off-by: Penny Zheng <penny.zheng@arm.com> --- v5 changes: - In order to avoid stub functions, we #define PGC_staticmem to non-zero only when CONFIG_STATIC_MEMORY - use "unlikely()" around pg->count_info & PGC_staticmem - remove pointless "if", since mark_page_free() is going to set count_info to PGC_state_free and by consequence clear PGC_staticmem - move #define PGC_staticmem 0 to mm.h --- v4 changes: - no changes --- v3 changes: - fix possible racy issue in free_staticmem_pages() - introduce a stub free_staticmem_pages() for the !CONFIG_STATIC_MEMORY case - move the change to free_heap_pages() to cover other potential call sites - fix the indentation --- v2 changes: - new commit --- xen/arch/arm/include/asm/mm.h | 2 ++ xen/common/page_alloc.c | 16 +++++++++------- xen/include/xen/mm.h | 6 +++++- 3 files changed, 16 insertions(+), 8 deletions(-)