Re: [PATCH v9 3/8] xen: do not merge reserved pages in free_heap_pages()

[Julien Grall <julien@xxxxxxx>]

Hi Penny,

Sorry I only spotted a couple of typos in the commit message now.

On 20/07/2022 06:46, Penny Zheng wrote:
> The code in free_heap_pages() will try to merge pages with the
> successor/predecessor if pages are suitably aligned. So if the pages
> reserved are right next to the pages given to the heap allocator,
> free_heap_pages() will merge them, and give the reserved pages to heap
> allocator accidently as a result.
s/accidently/accidentally/ I think.

> So in order to avoid the above scenario, this commit updates free_heap_pages()
> to check whether the predecessor and/or successor has PGC_reserved set,
s/PGC_reserved/PGC_staticmem/

I think both #1 and #3 can go in now. So I would be happy to handle the 
typoes on commit.
> when trying to merge the about-to-be-freed chunk with the predecessor
> and/or successor.
>
> Suggested-by: Julien Grall <jgrall@xxxxxxxxxx>
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
> v9 changes:
> - no change
> ---
> v8 changes:
> - no change
> ---
> v7 changes:
> - no change
> ---
> v6 changes:
> - adapt to PGC_static
> ---
> v5 changes:
> - change PGC_reserved to adapt to PGC_staticmem
> ---
> v4 changes:
> - no changes
> ---
> v3 changes:
> - no changes
> ---
> v2 changes:
> - new commit
> ---
>   xen/common/page_alloc.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index a12622e921..45bd88a685 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1475,6 +1475,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) ||
>                    (PFN_ORDER(predecessor) != order) ||
>                    (phys_to_nid(page_to_maddr(predecessor)) != node) )
>                   break;
> @@ -1498,6 +1499,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) ||
>                    (PFN_ORDER(successor) != order) ||
>                    (phys_to_nid(page_to_maddr(successor)) != node) )
>                   break;
Cheers,

--
Julien Grall