Re: [PATCH v10 08/12] xen/page_alloc: introduce preserved page flags macro

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.11.2024 10:32, Carlo Nonato wrote:
> Hi Jan,
> 
> On Thu, Nov 28, 2024 at 12:05 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>>
>> On 19.11.2024 15:13, Carlo Nonato wrote:
>>> PGC_static, PGC_extra and PGC_broken need 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 domain pages as this is
>>> considered an error and can cause ASSERT violations.
>>>
>>> Signed-off-by: Carlo Nonato <carlo.nonato@xxxxxxxxxxxxxxx>
>>> ---
>>> v10:
>>> - fixed commit message
>>> v9:
>>> - add PGC_broken to PGC_preserved
>>> - clear PGC_extra in alloc_domheap_pages() only if MEMF_no_refcount is set
>>> 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 | 19 ++++++++++++++-----
>>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
>>> index 7b911b5ed9..34cd473150 100644
>>> --- a/xen/common/page_alloc.c
>>> +++ b/xen/common/page_alloc.c
>>> @@ -160,6 +160,7 @@
>>>  #endif
>>>
>>>  #define PGC_no_buddy_merge PGC_static
>>> +#define PGC_preserved (PGC_extra | PGC_static | PGC_broken)
>>>
>>>  #ifndef PGT_TYPE_INFO_INITIALIZER
>>>  #define PGT_TYPE_INFO_INITIALIZER 0
>>> @@ -1427,12 +1428,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;
>>
>> PGC_extra doesn't want preserving here. Since it's mark_page_free(), and
>> since PGC_extra is removed before freeing, the state change is apparently
>> fine. But an assertion may want adding, for documentation purposes if
>> nothing else.
>>
>> Alternatively it may make sense to indeed exclude PGC_extra here. In fact
>> PGC_static doesn't need using here either, as unprepare_staticmem_pages()
>> will explicitly set it again anyway.
>>
>> Hence I wonder whether the change here really is necessary (one will then
>> be needed in the next patch aiui, when PGC_colored is introduced). Which
>> would then eliminate the need for the final two hunks of the patch, I
>> think.
>>
>>>      case PGC_state_offlining:
>>> -        pg->count_info = (pg->count_info & PGC_broken) |
>>> -                         PGC_state_offlined;
>>> +        pg->count_info = (pg->count_info & PGC_preserved) | 
>>> PGC_state_offlined;
>>>          pg_offlined = true;
>>>          break;
>>
>> I'm similarly unconvinced that anything other than PGC_broken (and
>> subsequently perhaps PGC_colored) would need preserving here.
> 
> Yes, we (re)checked the code and also believe that the introduction of
> PGC_preserved is generating more confusion (and code) then the actual logical
> help it provides.
> 
> We'll remove this patch and integrate PGC_colored explicitly in the flags to
> be preserved. This avoid the clumsy logic of preserving something (extra)
> when it's not needed and then handling the special case to remove it
> explicitly.
> Basically my goal is to go back to v4 where this patch didn't exist.

Hmm, no, I don't think I said anything in the direction of removing 
PGC_preserved
again. I merely think you went too far in where it actually wants using.

Jan