From mboxrd@z Thu Jan 1 00:00:00 1970 From: Julien Grall Subject: Re: [PATCH v2 07/10] xen/arm: Introduce relinquish_p2m_mapping to remove refcount every mapped page Date: Tue, 10 Dec 2013 01:31:52 +0000 Message-ID: <52A66F08.1040408@linaro.org> References: <1386560047-17500-1-git-send-email-julien.grall@linaro.org> <1386560047-17500-8-git-send-email-julien.grall@linaro.org> <1386606523.7812.54.camel@kazak.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" 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 1VqCB3-0001qh-6c for xen-devel@lists.xenproject.org; Tue, 10 Dec 2013 01:31:57 +0000 Received: by mail-wi0-f176.google.com with SMTP id hq4so4709320wib.3 for ; Mon, 09 Dec 2013 17:31:55 -0800 (PST) In-Reply-To: <1386606523.7812.54.camel@kazak.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Ian Campbell Cc: xen-devel@lists.xenproject.org, tim@xen.org, stefano.stabellini@citrix.com, patches@linaro.org List-Id: xen-devel@lists.xenproject.org On 12/09/2013 04:28 PM, Ian Campbell wrote: > On Mon, 2013-12-09 at 03:34 +0000, Julien Grall wrote: >> This function will be called when the domain relinquishes its memory. >> It removes refcount on every mapped page to a valid MFN. >> >> Currently, Xen doesn't take refcount on every new mapping but only for foreign >> mapping. Restrict the function only on foreign mapping. > > Skimming the remainder of the patch's titles and recalling a previous > conversation the intention is not to extend this for 4.4, correct? Right, it's too big for Xen 4.4. >> >> Signed-off-by: Julien Grall >> >> --- >> Changes in v2: >> - Introduce the patch >> --- >> xen/arch/arm/domain.c | 5 +++++ >> xen/arch/arm/p2m.c | 47 ++++++++++++++++++++++++++++++++++++++++++ >> xen/include/asm-arm/domain.h | 1 + >> xen/include/asm-arm/p2m.h | 15 ++++++++++++++ >> 4 files changed, 68 insertions(+) >> >> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c >> index 1590708..e7c2f67 100644 >> --- a/xen/arch/arm/domain.c >> +++ b/xen/arch/arm/domain.c >> @@ -717,6 +717,11 @@ int domain_relinquish_resources(struct domain *d) >> if ( ret ) >> return ret; >> >> + case RELMEM_mapping: > > Something somewhere should be setting d->arch.relmem = RELMEM_mapping at > the appropriate time. (immediately above I think?) > > You also want a "Fallthrough" comment just above. Oops, I will update the patch for the next version. >> + ret = relinquish_p2m_mapping(d); >> + if ( ret ) >> + return ret; >> + >> d->arch.relmem = RELMEM_done; >> /* Fallthrough */ >> >> diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c >> index f0bbaca..dbd6a06 100644 >> --- a/xen/arch/arm/p2m.c >> +++ b/xen/arch/arm/p2m.c >> @@ -6,6 +6,7 @@ >> #include >> #include >> #include >> +#include >> >> /* First level P2M is 2 consecutive pages */ >> #define P2M_FIRST_ORDER 1 >> @@ -320,6 +321,16 @@ static int create_p2m_entries(struct domain *d, >> flush_tlb_all_local(); >> } >> >> + if ( (t == p2m_ram_rw) || (t == p2m_ram_ro) || (t == p2m_map_foreign)) >> + { >> + unsigned long sgfn = paddr_to_pfn(start_gpaddr); >> + unsigned long egfn = paddr_to_pfn(end_gpaddr); >> + >> + p2m->max_mapped_gfn = MAX(p2m->max_mapped_gfn, egfn); >> + /* Use next_gfn_to_relinquish to store the lowest gfn mapped */ >> + p2m->next_gfn_to_relinquish = MIN(p2m->next_gfn_to_relinquish, sgfn); >> + } >> + >> rc = 0; >> >> out: >> @@ -503,12 +514,48 @@ int p2m_init(struct domain *d) >> >> p2m->first_level = NULL; >> >> + p2m->max_mapped_gfn = 0; >> + p2m->next_gfn_to_relinquish = ULONG_MAX; >> + >> err: >> spin_unlock(&p2m->lock); >> >> return rc; >> } >> >> +int relinquish_p2m_mapping(struct domain *d) >> +{ >> + struct p2m_domain *p2m = &d->arch.p2m; >> + unsigned long gfn, count = 0; >> + int rc = 0; >> + >> + for ( gfn = p2m->next_gfn_to_relinquish; >> + gfn < p2m->max_mapped_gfn; gfn++ ) > > I know that Tim has been keen to get rid of these sorts of loops on x86, > and with good reason I think. > >> + { >> + p2m_type_t t; >> + paddr_t p = p2m_lookup(d, gfn, &t); > > This does the full walk for each address, even though 2/3 of the levels > are more than likely identical to the previous gfn. > > It would be better to do a full walk, which sadly will look a lot like > p2m_lookup, no avoiding that I think. > > You can still resume the walk based on next_gfn_to_relinquish and bound > it on max_mapped_gfn, although I don't think it is strictly necessary. >> + unsigned long mfn = p >> PAGE_SHIFT; >> + >> + if ( mfn_valid(mfn) && p2m_is_foreign(t) ) > > I think it would be worth reiterating in a comment that we only take a > ref for foreign mappings right now. Will do. >> + { >> + put_page(mfn_to_page(mfn)); >> + guest_physmap_remove_page(d, gfn, mfn, 0); > > You should unmap it and then put it I think. > > Is this going to do yet another lookup/walk? > > The REMOVE case of create_p2m_entries is so trivial you could open code > it here, or if you wanted to you could refactor it into a helper. > > I am wondering if the conditional put page ought to be at the point of > removal (i.e. in the helper) rather than here. (I think Tim made a > similar comment on the x86 version of the remove_from_physmap pvh > patches, you probably need to match the generic change which that > implies) > > BTW, if you do the clear (but not the put_page) for every entry then the > present bit (or pte.bits == 0) might be a useful proxy for > next_lN_entry? I suppose even walking, say, 4GB of mostly empty P2M is > not going to be super cheap. Following the discution we had IRL, I will try to extend create_p2m_entries by adding RELINQUISH option. >> + } >> + >> + count++; >> + >> + /* Preempt every 2MiB. Arbitrary */ >> + if ( (count == 512) && hypercall_preempt_check() ) >> + { >> + p2m->next_gfn_to_relinquish = gfn + 1; >> + rc = -EAGAIN; > > I think I'm just failing to find it, but where is the call to > hypercall_create_continuation? I suppose it is somewhere way up the > stack? Actually rc = -EAGAIN will be catched by xc_domain_destroy in libxc. The function will call again the hypercall if needed. I'm not sure why... do you have any idea why x86 also uses this trick? > I'm not sure the count == 512 is needed -- hypercall_preempt_check > should be sufficient? On ARM hypercall_preempt_check is a bit complex. Checking every loop seems a bit overkill. -- Julien Grall