Re: [PATCH v5 08/13] xen/page_alloc: introduce preserved page flags macro
On 12.01.2024 11:01, Carlo Nonato wrote: > On Mon, Jan 8, 2024 at 6:08 PM Jan Beulich wrote: >> 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). > > Sorry, I may have misinterpreted your comments on the previous version of the > series (I know it was a really long time ago) > > https://patchew.org/Xen/20230123154735.74832-1-carlo.non...@minervasys.tech/20230123154735.74832-8-carlo.non...@minervasys.tech/#c843b031-52f7-056d-e8c0-75fe9c426...@suse.com > > Anyway, would the solution here be to have two distinct #define? One for > suppress merging and the other for preserved flags. This would probably also > remove any confusion with the usage of PGC_extra. That's one way to deal with this. Another would be to refine the above checks, such that both buddies' preserved flags are actually compared, and merging be suppressed if they're different. Then going with a single #define would imo be quite okay. Jan
Re: [PATCH v5 08/13] xen/page_alloc: introduce preserved page flags macro
Hi Jan, On Mon, Jan 8, 2024 at 6:08 PM Jan Beulich wrote: > > 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). Sorry, I may have misinterpreted your comments on the previous version of the series (I know it was a really long time ago) https://patchew.org/Xen/20230123154735.74832-1-carlo.non...@minervasys.tech/20230123154735.74832-8-carlo.non...@minervasys.tech/#c843b031-52f7-056d-e8c0-75fe9c426...@suse.com Anyway, would the solution here be to have two distinct #define? One for suppress merging and the other for preserved flags. This would probably also remove any confusion with the usage of PGC_extra. Thanks. > Jan
Re: [PATCH v5 08/13] xen/page_alloc: introduce preserved page flags macro
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
