diff mbox series

[v5,08/13] xen/page_alloc: introduce preserved page flags macro

Message ID 20240102095138.17933-9-carlo.nonato@minervasys.tech (mailing list archive)
State Superseded
Headers show
Series Arm cache coloring | expand

Commit Message

Carlo Nonato Jan. 2, 2024, 9:51 a.m. UTC 
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.

Signed-off-by: Carlo Nonato <carlo.nonato@minervasys.tech>
Signed-off-by: Marco Solieri <marco.solieri@minervasys.tech>
---
v5:
- new patch
---
 xen/common/page_alloc.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Jan Beulich Jan. 8, 2024, 5:08 p.m. UTC | #1 
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
Carlo Nonato Jan. 12, 2024, 10:01 a.m. UTC | #2 
Hi Jan,

On Mon, Jan 8, 2024 at 6:08 PM Jan Beulich <jbeulich@suse.com> 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@minervasys.tech/20230123154735.74832-8-carlo.nonato@minervasys.tech/#c843b031-52f7-056d-e8c0-75fe9c426343@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
Jan Beulich Jan. 12, 2024, 10:22 a.m. UTC | #3 
On 12.01.2024 11:01, Carlo Nonato wrote:
> On Mon, Jan 8, 2024 at 6:08 PM Jan Beulich <jbeulich@suse.com> 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@minervasys.tech/20230123154735.74832-8-carlo.nonato@minervasys.tech/#c843b031-52f7-056d-e8c0-75fe9c426343@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
diff mbox series

Patch

diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 9ee3981bb5..3bf3120287 100644
--- 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)
+
 #ifndef PGT_TYPE_INFO_INITIALIZER
 #define PGT_TYPE_INFO_INITIALIZER 0
 #endif
@@ -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;
@@ -2365,7 +2367,7 @@  int assign_pages(
 
         for ( i = 0; i < nr; i++ )
         {
-            ASSERT(!(pg[i].count_info & ~(PGC_extra | PGC_static)));
+            ASSERT(!(pg[i].count_info & ~preserved_flags));
             if ( pg[i].count_info & PGC_extra )
                 extra_pages++;
         }
@@ -2425,7 +2427,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 & preserved_flags) | PGC_allocated | 1;
 
         page_list_add_tail(&pg[i], page_to_list(d, &pg[i]));
     }