Re: [RFC PATCH] PVH: cleanup of p2m upon p2m destroy

[Mukesh Rathor <mukesh.rathor@oracle.com>]

On Thu, 19 Dec 2013 11:50:58 +0100
Tim Deegan <tim@xen.org> wrote:

> At 18:01 -0800 on 18 Dec (1387386115), Mukesh Rathor wrote:
> > On Wed, 18 Dec 2013 17:51:52 +0100
> > Tim Deegan <tim@xen.org> wrote:
> > 
> > > At 11:09 +0100 on 18 Dec (1387361398), Tim Deegan wrote:
> > > > > An alternative might be to just create a link list then and
> > > > > walk it. In general, foreign mappings should be very small,
> > > > > so the overhead of 16 bytes per page for the link list might
> > > > > not be too bad. I will code it if there is no disagreement
> > > > > from any maintainer... everyone has different ideas :)...
> > > > 
> > > > I think it would be best to walk the p2m trie (i.e. bounded by
> > > > amount of RAM, rather than max GFN) and do it preemptably.  I'll
> > > > look into something like that for the mem_sharing loop today,
> > > > and foreign mapping code can reuse it.
> > > 
> > > What I've ended up with is making p2m_change_entry_type_global()
> > > preemptible (which is a bigger task but will be needed as domains
> > > get bigger).  Do you think that using that function to switch all
> > > mappings from p2m_foreign to p2m_invalid, appropriately late in
> > > the teardown, will be good enough for what you need?
> > 
> > No, not quite, because I need to know which mfns are foreign and
> > do put_page on them. 
> 
> Oh right, but you need to do that anyway -- once you've got refcounts
> held by ept entries, you need to do _all_ entry writes through the
> function that preserves the refcount invariant.  So
> p2m_change_entry_type_global() has to use that function too, in case
> either of the types it's called with is p2m_foreign.  And once that's
> fixed, a call to s/foreign/invalid/ DTRT.

Right, right... i keep not realizing, it's there now and it's use
will scope beyond the current PVH usage. So rather than deferring to
when someone actually finds a use case for p2m_change_entry_type_global
using foreign, i should be handled now. 

I'll have next, un-rushed this time, patch prob first week of jan 
given the upcoming holidays.

> Also, this:
> 
> > +static int ept_put_foreign_mfn(ept_entry_t *entry, unsigned long
> > data) +{
> > +    put_page(mfn_to_page(entry->mfn));
> > +    return 0;
> > +}
> 
> gives me the heebie-jeebies, because it explcitly _breaks_ the
> invariant, leaving the foreign mapping in place without its refcount.

I was thinking the case of multiple mappings, but re-thinking about it,
multiple mappings should just not be allowed IMO, so as soon as
put_page is done, the entry be invalidated. I'll redo it.

thanks a lot,
mukesh