Re: [PATCH v7 3/7] xen: introduce mark_page_free

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2021 04:52, 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.
> 
> The PDX compression makes that 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.
> 
> Signed-off-by: Penny Zheng <penny.zheng@xxxxxxx>
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>
> Reviewed-by: Julien Grall <jgrall@xxxxxxxxxx>
> ---
>  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)));
> +
> +    /*
> +     * 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;
> +        tainted = 1;

You've broken two things at the same time here: You write to the global
variable of this name now, while ...

> @@ -1392,47 +1439,7 @@ static void free_heap_pages(
>  
>      for ( i = 0; i < (1 << order); i++ )
>      {
> -        /*
> -         * 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[i].count_info & PGC_state )
> -        {
> -        case PGC_state_inuse:
> -            BUG_ON(pg[i].count_info & PGC_broken);
> -            pg[i].count_info = PGC_state_free;
> -            break;
> -
> -        case PGC_state_offlining:
> -            pg[i].count_info = (pg[i].count_info & PGC_broken) |
> -                               PGC_state_offlined;
> -            tainted = 1;

... the local variable here doesn't get written anymore. Which Coverity
was kind enough to point out - please reference Coverity ID 1491872 in
the fix that I hope you will be able to provide quickly. (The easiest
change would seem to be to make mark_page_free() return bool, and set
"tainted" to 1 here accordingly.)

I understand that the two variables having the same name isn't very
helpful. I certainly wouldn't mind if you renamed the local one
suitably.

Jan