Re: [PATCH V4 04/10] xen: introduce mark_page_free

[Julien Grall <julien@xen.org>]

Hi Penny,

On 28/07/2021 11:27, Penny Zheng wrote:
> This commit defines a new helper mark_page_free to extract common code,
> like following the same cache/TLB coherency policy, between free_heap_pages
> and the new function free_staticmem_pages, which will be introduced later.
> 
> Signed-off-by: Penny Zheng <penny.zheng@arm.com>
> Acked-by: Jan Beulich <jbeulich@suse.com>
> ---
>   xen/common/page_alloc.c | 89 ++++++++++++++++++++++-------------------
>   1 file changed, 48 insertions(+), 41 deletions(-)
> 
> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
> index 958ba0cd92..a3ee5eca9e 100644
> --- a/xen/common/page_alloc.c
> +++ b/xen/common/page_alloc.c
> @@ -1376,6 +1376,53 @@ bool scrub_free_pages(void)
>       return node_to_scrub(false) != NUMA_NO_NODE;
>   }
>   
> +static void mark_page_free(struct page_info *pg, mfn_t mfn)
> +{
> +    ASSERT(mfn_x(mfn) == mfn_x(page_to_mfn(pg)));

NIT: If I got it correctly, the assumption is page_to_mfn() is 
expensive, so we want to avoid the conversation. I am not sure I agree 
with that but I would at least suggest to write it down in the commit 
message.

So it is easier for the next person to figure out the rationale. 
Something like:

"The PDX compression makes makes conversion between the MFN and the page 
can be potentially non-trivial. As the function is internal, pass the 
MFN and the page. They are both expected to match."

For the rest of the patch:

Reviewed-by: Julien Grall <jgrall@amazon.com>

Cheers,

-- 
Julien Grall