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

[Julien Grall <julien@xxxxxxx>]

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