On 02.01.2024 10:51, Carlo Nonato wrote: > PGC_static and PGC_extra are flags that 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. > > The new macro is used also in free_heap_pages() allowing future commits to > extended it with other flags that must stop merging, as it now works for > PGC_static. PGC_extra is no harm here since it's only ever being set on > allocated pages.
Is it? I can't see where free_domheap_pages() would clear it before calling free_heap_pages(). Or wait, that may happen in mark_page_free(), but then PGC_static would be cleared there, too. I must be missing something. > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -158,6 +158,8 @@ > #define PGC_static 0 > #endif > > +#define preserved_flags (PGC_extra | PGC_static) I think this wants to (a) have a PGC_ prefix and (b) as a #define be all capitals. > @@ -1504,7 +1506,7 @@ static void free_heap_pages( > /* Merge with predecessor block? */ > if ( !mfn_valid(page_to_mfn(predecessor)) || > !page_state_is(predecessor, free) || > - (predecessor->count_info & PGC_static) || > + (predecessor->count_info & preserved_flags) || > (PFN_ORDER(predecessor) != order) || > (page_to_nid(predecessor) != node) ) > break; > @@ -1528,7 +1530,7 @@ static void free_heap_pages( > /* Merge with successor block? */ > if ( !mfn_valid(page_to_mfn(successor)) || > !page_state_is(successor, free) || > - (successor->count_info & PGC_static) || > + (successor->count_info & preserved_flags) || > (PFN_ORDER(successor) != order) || > (page_to_nid(successor) != node) ) > break; Irrespective of the comment at the top, this looks like an abuse of the new constant: There's nothing inherently making preserved flags also suppress merging (assuming it was properly checked that both sided have the same flags set/clear). Jan
