[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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 <jbeulich@xxxxxxxx> 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.nonato@xxxxxxxxxxxxxxx/20230123154735.74832-8-carlo.nonato@xxxxxxxxxxxxxxx/#c843b031-52f7-056d-e8c0-75fe9c426343@xxxxxxxx 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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |