| Message ID | 20241025095014.42376-9-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Arm cache coloring | expand |
On 25.10.2024 11:50, Carlo Nonato wrote: > PGC_static and PGC_extra needs to be preserved when assigning a page. > Define a new macro that groups those flags and use it instead of or'ing > every time. > > To make preserved flags even more meaningful, they are kept also when > switching state in mark_page_free(). > Enforce the removal of PGC_extra before freeing new pages as this is > considered an error and can cause ASSERT violations. What does "new" here mean? > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > --- > v9: > - add PGC_broken to PGC_preserved Which then also wants reflecting in the description. > @@ -2485,6 +2485,14 @@ struct page_info *alloc_domheap_pages( > } > if ( assign_page(pg, order, d, memflags) ) > { > + if ( memflags & MEMF_no_refcount ) > + { > + unsigned long i; > + > + for ( i = 0; i < (1UL << order); i++ ) > + pg[i].count_info &= ~PGC_extra; > + } The description doesn't cover this, only ... > @@ -2539,6 +2547,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) > { > ASSERT(d->extra_pages); > d->extra_pages--; > + pg[i].count_info &= ~PGC_extra; > } > } ... this is mentioned there. And it's not clear to me why it would need doing in both places. Jan
Hi Jan, On Tue, Nov 5, 2024 at 4:59 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 25.10.2024 11:50, Carlo Nonato wrote: > > PGC_static and PGC_extra needs to be preserved when assigning a page. > > Define a new macro that groups those flags and use it instead of or'ing > > every time. > > > > To make preserved flags even more meaningful, they are kept also when > > switching state in mark_page_free(). > > Enforce the removal of PGC_extra before freeing new pages as this is > > considered an error and can cause ASSERT violations. > > What does "new" here mean? I believe it means "assigned to domains". So yes "new" it's definitely wrong. > > Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> > > --- > > v9: > > - add PGC_broken to PGC_preserved > > Which then also wants reflecting in the description. Yep. > > @@ -2485,6 +2485,14 @@ struct page_info *alloc_domheap_pages( > > } > > if ( assign_page(pg, order, d, memflags) ) > > { > > + if ( memflags & MEMF_no_refcount ) > > + { > > + unsigned long i; > > + > > + for ( i = 0; i < (1UL << order); i++ ) > > + pg[i].count_info &= ~PGC_extra; > > + } > > The description doesn't cover this, only ... PGC_extra must be cleared before freeing pages. Since here we're going to free pages, PGC_extra must be removed. > > @@ -2539,6 +2547,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) > > { > > ASSERT(d->extra_pages); > > d->extra_pages--; > > + pg[i].count_info &= ~PGC_extra; > > } > > } > > ... this is mentioned there. And it's not clear to me why it would need doing > in both places. > > Jan - Carlo
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 7b911b5ed9..34cd473150 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -160,6 +160,7 @@ #endif #define PGC_no_buddy_merge PGC_static +#define PGC_preserved (PGC_extra | PGC_static | PGC_broken) #ifndef PGT_TYPE_INFO_INITIALIZER #define PGT_TYPE_INFO_INITIALIZER 0 @@ -1427,12 +1428,11 @@ static bool mark_page_free(struct page_info *pg, mfn_t mfn) { case PGC_state_inuse: BUG_ON(pg->count_info & PGC_broken); - pg->count_info = PGC_state_free; + pg->count_info = PGC_state_free | (pg->count_info & PGC_preserved); break; case PGC_state_offlining: - pg->count_info = (pg->count_info & PGC_broken) | - PGC_state_offlined; + pg->count_info = (pg->count_info & PGC_preserved) | PGC_state_offlined; pg_offlined = true; break; @@ -2366,7 +2366,7 @@ int assign_pages( for ( i = 0; i < nr; i++ ) { - ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static))); + ASSERT(!(pg[i].count_info & ~PGC_preserved)); if ( pg[i].count_info & PGC_extra ) extra_pages++; } @@ -2426,7 +2426,7 @@ int assign_pages( page_set_owner(&pg[i], d); smp_wmb(); /* Domain pointer must be visible before updating refcnt. */ pg[i].count_info = - (pg[i].count_info & (PGC_extra | PGC_static)) | PGC_allocated | 1; + (pg[i].count_info & PGC_preserved) | PGC_allocated | 1; page_list_add_tail(&pg[i], page_to_list(d, &pg[i])); } @@ -2485,6 +2485,14 @@ struct page_info *alloc_domheap_pages( } if ( assign_page(pg, order, d, memflags) ) { + if ( memflags & MEMF_no_refcount ) + { + unsigned long i; + + for ( i = 0; i < (1UL << order); i++ ) + pg[i].count_info &= ~PGC_extra; + } + free_heap_pages(pg, order, memflags & MEMF_no_scrub); return NULL; } @@ -2539,6 +2547,7 @@ void free_domheap_pages(struct page_info *pg, unsigned int order) { ASSERT(d->extra_pages); d->extra_pages--; + pg[i].count_info &= ~PGC_extra; } }
PGC_static and PGC_extra needs to be preserved when assigning a page. Define a new macro that groups those flags and use it instead of or'ing every time. To make preserved flags even more meaningful, they are kept also when switching state in mark_page_free(). Enforce the removal of PGC_extra before freeing new pages as this is considered an error and can cause ASSERT violations. Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech> --- v9: - add PGC_broken to PGC_preserved - clear PGC_extra in alloc_domheap_pages() only if MEMF_no_refcount is set v8: - fixed PGC_extra ASSERT fail in alloc_domheap_pages() by removing PGC_extra before freeing v7: - PGC_preserved used also in mark_page_free() v6: - preserved_flags renamed to PGC_preserved - PGC_preserved is used only in assign_pages() v5: - new patch --- xen/common/page_alloc.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-)