| Message ID | 20240502165533.319988-9-carlo.nonato@minervasys.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Arm cache coloring | expand |
On 02.05.2024 18:55, Carlo Nonato wrote: > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -159,6 +159,7 @@ > #endif > > #define PGC_no_buddy_merge PGC_static > +#define PGC_preserved (PGC_extra | PGC_static) Seeing this again and its use ... > @@ -1426,11 +1427,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) | > + pg->count_info = (pg->count_info & (PGC_broken | PGC_preserved)) | > PGC_state_offlined; > pg_offlined = true; > break; ... here: Shouldn't PGC_broken also be included in PGC_preserved? > @@ -2484,6 +2485,11 @@ struct page_info *alloc_domheap_pages( > } > if ( assign_page(pg, order, d, memflags) ) > { > + unsigned long i; > + > + for ( i = 0; i < (1UL << order); i++ ) > + pg[i].count_info &= ~PGC_extra; For larger order this loop is non-trivial and may have a fair effect on caches. Looking at the code just outside of upper patch context, is this loop needed at all when MEMF_no_refcount is clear in memflags? Jan
Hi Jan, On Mon, May 6, 2024 at 2:22 PM Jan Beulich <jbeulich@suse.com> wrote: > > On 02.05.2024 18:55, Carlo Nonato wrote: > > --- a/xen/common/page_alloc.c > > +++ b/xen/common/page_alloc.c > > @@ -159,6 +159,7 @@ > > #endif > > > > #define PGC_no_buddy_merge PGC_static > > +#define PGC_preserved (PGC_extra | PGC_static) > > Seeing this again and its use ... > > > @@ -1426,11 +1427,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) | > > + pg->count_info = (pg->count_info & (PGC_broken | PGC_preserved)) | > > PGC_state_offlined; > > pg_offlined = true; > > break; > > ... here: Shouldn't PGC_broken also be included in PGC_preserved? I hope there are no other problems like the one with PGC_extra. > > @@ -2484,6 +2485,11 @@ struct page_info *alloc_domheap_pages( > > } > > if ( assign_page(pg, order, d, memflags) ) > > { > > + unsigned long i; > > + > > + for ( i = 0; i < (1UL << order); i++ ) > > + pg[i].count_info &= ~PGC_extra; > > For larger order this loop is non-trivial and may have a fair effect on > caches. Looking at the code just outside of upper patch context, is this > loop needed at all when MEMF_no_refcount is clear in memflags? Nope. I will wrap it in a if. Thanks. > Jan
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index d2032a79b0..e71b571a3b 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -159,6 +159,7 @@ #endif #define PGC_no_buddy_merge PGC_static +#define PGC_preserved (PGC_extra | PGC_static) #ifndef PGT_TYPE_INFO_INITIALIZER #define PGT_TYPE_INFO_INITIALIZER 0 @@ -1426,11 +1427,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) | + pg->count_info = (pg->count_info & (PGC_broken | PGC_preserved)) | PGC_state_offlined; pg_offlined = true; break; @@ -2365,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++; } @@ -2425,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])); } @@ -2484,6 +2485,11 @@ struct page_info *alloc_domheap_pages( } if ( assign_page(pg, order, d, memflags) ) { + 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; } @@ -2538,6 +2544,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> --- 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 | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)