From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mukesh Rathor Subject: Re: [RFC PATCH] PVH: cleanup of p2m upon p2m destroy Date: Thu, 19 Dec 2013 18:00:09 -0800 Message-ID: <20131219180009.01d61971@mantra.us.oracle.com> References: <20131216174728.2ba3ad9a@mantra.us.oracle.com> <52B01C88020000780010E042@nat28.tlf.novell.com> <20131217101957.GB32721@deinos.phlegethon.org> <20131217184412.2372eb45@mantra.us.oracle.com> <20131218100958.GB24792@deinos.phlegethon.org> <20131218165152.GO24792@deinos.phlegethon.org> <20131218180155.1dc09717@mantra.us.oracle.com> <20131219105058.GA20571@deinos.phlegethon.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VtpNy-0003Sn-Ir for xen-devel@lists.xenproject.org; Fri, 20 Dec 2013 02:00:18 +0000 In-Reply-To: <20131219105058.GA20571@deinos.phlegethon.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Tim Deegan Cc: xen-devel , Jan Beulich List-Id: xen-devel@lists.xenproject.org On Thu, 19 Dec 2013 11:50:58 +0100 Tim Deegan wrote: > At 18:01 -0800 on 18 Dec (1387386115), Mukesh Rathor wrote: > > On Wed, 18 Dec 2013 17:51:52 +0100 > > Tim Deegan 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