RE: [PATCH v4 1/6] xen: do not free reserved memory into heap

[Penny Zheng <Penny.Zheng@xxxxxxx>]

Hi Julien

> -----Original Message-----
> From: Julien Grall <julien@xxxxxxx>
> Sent: Tuesday, May 17, 2022 5:29 PM
> To: Penny Zheng <Penny.Zheng@xxxxxxx>; xen-devel@xxxxxxxxxxxxxxxxxxxx
> Cc: Wei Chen <Wei.Chen@xxxxxxx>; Andrew Cooper
> <andrew.cooper3@xxxxxxxxxx>; George Dunlap <george.dunlap@xxxxxxxxxx>;
> Jan Beulich <jbeulich@xxxxxxxx>; Stefano Stabellini <sstabellini@xxxxxxxxxx>;
> Wei Liu <wl@xxxxxxx>
> Subject: Re: [PATCH v4 1/6] xen: do not free reserved memory into heap
> 
> Hi,
> 
> On 17/05/2022 09:21, Penny Zheng wrote:
> > Yes,  I remembered that asynchronous is still on the to-do list for static
> memory.
> >
> > If it doesn't bother too much to you, I would like to ask some help on this
> issue, ;).
> > I only knew basic knowledge on the scrubbing,
> My kwnoledge on the scrubbing code is not much better than yours :).
> 
> > I knew that dirty pages is placed at the
> > end of list heap(node, zone, order) for scrubbing and "first_dirty" is used 
> > to
> track down
> > the dirty pages. IMO, Both two parts are restricted to the heap thingy,  not
> reusable for
> > static memory,
> 
> That's correct.
> 
> > so maybe I need to re-write scrub_free_page for static memory, and also
> > link the need-to-scrub reserved pages to a new global list e.g.  
> > dirty_resv_list
> for aync
> > scrubbing?
> 
> So I can foresee two problems with scrubbing static memory:
>    1) Once the page is scrubbed, we need to know which domain it belongs
> so we can link the page again
>    2) A page may still wait for scrubbing when the domain allocate
> memory (IOW the reserved list may be empty). So we need to find a page
> belonging to the domain and then scrubbed.
>

Understood, thanks for the to-the-point instruction! ;)
For scrubbing on runtime, un-populating memory will free reserved pages
to reserved list, then async scrubbing will move them to a per-domain list. 
Later
when scrubbing is finished, we need to again move it back to the reserved
list.
And if we failed on acquiring a page from reserved list, then trying to get a 
page
from the per-domain list and scrub it. 

And with initial scrubbing, since the concept of domain is not constructed, a
global list is better.
Right now, we always allocate static memory from specified starting address,
so just make sure that page is scrubbed before allocation.

> The two problems above would indicate that a per-domain scrub list would
> be the best here. We would need to deal with initial scrubbing
> differently (maybe a global list as you suggested).
> 
> I expect it will take some times to implement it properly. While writing
> this, I was wondering if there is actually any point to scrub pages when
> the domain is releasing them. Even if they are free they are still
> belonging to the domain, so scrubbing them is technically not necessary.
> 

True, true, if static memory used as guest memory, even if they are free, they 
are still
belonging to the domain. Even as static shared memory, it is pre-configured in 
boot-time
and could not be used for any other purpose.
Hmmm, may I ask that if we reboot the domain and didn't scrub the pages before, 
the
old stale contents will not affect the rebooting machine?
Or it should be the guest's responsibility to do the cleaning up before using 
it?

If it is the guest's responsibility, yeah, maybe scrubbing them is technically 
not
necessary, flushing TLB and cleaning cache is enough~ So should I remove the 
scrubbing
totally for static memory?

> Any thoughts?
> 
> >>>    {
> >>>        mfn_t mfn = page_to_mfn(pg);
> >>>        unsigned long i;
> >>> @@ -2653,7 +2657,8 @@ void __init free_staticmem_pages(struct
> page_info
> >> *pg, unsigned long nr_mfns,
> >>>            }
> >>>
> >>>            /* In case initializing page of static memory, mark it 
> >>> PGC_reserved. */
> >>> -        pg[i].count_info |= PGC_reserved;
> >>> +        if ( !(pg[i].count_info & PGC_reserved) )
> >>
> >> NIT: I understand the flag may have already been set, but I am not
> convinced if
> >> it is worth checking it and then set.
> >>
> >
> > Jan suggested that since we remove the __init from free_staticmem_pages,
> it's now in preemptable
> > state at runtime, so better be adding this check here.
> 
> Well, count_info is already modified within that loop (see
> mark_page_free()). So I think the impact of setting PGC_reserved is
> going to be meaningless.
> 
> However... mark_page_free() is going to set count_info to PGC_state_free
> and by consequence clear PGC_reserved. Theferore, in the current
> implementation we always need to re-set PGC_reserved.
> 
> So effectively, the "if" is pointless here.
> 
> Cheers,
> 
> --
> Julien Grall