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 02:36:48 +0000 Message-ID: <52A67E40.4020403@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> <52A66F08.1040408@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VqDBt-0006Lw-Bl for xen-devel@lists.xenproject.org; Tue, 10 Dec 2013 02:36:53 +0000 Received: by mail-wi0-f172.google.com with SMTP id en1so4756330wid.11 for ; Mon, 09 Dec 2013 18:36:51 -0800 (PST) In-Reply-To: <52A66F08.1040408@linaro.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: 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/10/2013 01:31 AM, Julien Grall wrote: > > > 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. > So I gave a try to remove the (count == 512) and it's very very slow (more than 10 times slower). So I will keep it. -- Julien Grall