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

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Jan

Sorry for the broken.

> -----Original Message-----
> From: Jan Beulich <jbeulich@xxxxxxxx>
> Sent: Wednesday, September 15, 2021 9:56 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>
> Cc: Bertrand Marquis <Bertrand.Marquis@xxxxxxx>; Wei Chen
> <Wei.Chen@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx;
> sstabellini@xxxxxxxxxx; julien@xxxxxxx
> Subject: Re: [PATCH v7 3/7] xen: introduce mark_page_free
> 
> 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.)
> 

Sure. I'll fix it today and let mark_page_free() return the status of "tainted".

> 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.
> 

I'll rename the local one to "pg_tainted" to tell the difference.

> Jan

Penny