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

From: Jan Beulich <jbeulich@suse.com>
To: Penny Zheng <penny.zheng@arm.com>
Cc: Bertrand.Marquis@arm.com, Wei.Chen@arm.com, nd@arm.com,
	xen-devel@lists.xenproject.org, sstabellini@kernel.org,
	julien@xen.org
Subject: Re: [PATCH V3 04/10] xen: introduce mark_page_free
Date: Mon, 19 Jul 2021 10:13:35 +0200	[thread overview]
Message-ID: <d6962e4d-4121-206a-a60c-00c9074d898f@suse.com> (raw)
In-Reply-To: <20210715051819.3073628-5-penny.zheng@arm.com>

On 15.07.2021 07:18, 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>
with ...

> --- 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)));
> +
> +    /*
> +     * Cannot assume that count_info == 0, as there are some corner cases
> +     * where it isn't the case and yet it isn't a bug:
> +     *  1. page_get_owner() is NULL
> +     *  2. page_get_owner() is a domain that was never accessible by
> +     *     its domid (e.g., failed to fully construct the domain).
> +     *  3. page was never addressable by the guest (e.g., it's an
> +     *     auto-translate-physmap guest and the page was never included
> +     *     in its pseudophysical address space).
> +     * In all the above cases there can be no guest mappings of this page.
> +     */
> +    switch ( pg->count_info & PGC_state )
> +    {
> +    case PGC_state_inuse:
> +        BUG_ON(pg->count_info & PGC_broken);
> +        pg->count_info = PGC_state_free;
> +        break;
> +
> +    case PGC_state_offlining:
> +        pg->count_info = (pg->count_info & PGC_broken) |
> +                           PGC_state_offlined;

... indentation here not screwed up (it was correct originally).

> +        tainted = 1;
> +        break;
> +
> +    default:
> +        printk(XENLOG_ERR
> +               "pg MFN %"PRI_mfn" c=%#lx o=%u v=%#lx t=%#x\n",
> +               mfn_x(mfn),
> +               pg->count_info, pg->v.free.order,
> +               pg->u.free.val, pg->tlbflush_timestamp);

Just as a remark: Let's hope that the lost piece of information here
(the caller's induction variable) won't cause us trouble later on.

Jan