From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH v2] x86/PoD: shorten certain operations on higher order ranges Date: Wed, 30 Sep 2015 15:23:28 +0100 Message-ID: <560BF060.4020009@citrix.com> References: <56096B3902000078000A63BE@prv-mh.provo.novell.com> <560BEDD402000078000A719F@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZhIIB-0006FP-GP for xen-devel@lists.xenproject.org; Wed, 30 Sep 2015 14:23:35 +0000 In-Reply-To: <560BEDD402000078000A719F@prv-mh.provo.novell.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: Jan Beulich , George Dunlap Cc: Andrew Cooper , Tim Deegan , Keir Fraser , xen-devel List-Id: xen-devel@lists.xenproject.org On 30/09/15 13:12, Jan Beulich wrote: >>>> On 29.09.15 at 18:45, wrote: >> On Mon, Sep 28, 2015 at 3:30 PM, Jan Beulich wrote: >>> Now that p2m->get_entry() always returns a valid order, utilize this >>> to accelerate some of the operations in PoD code. (There are two uses >>> of p2m->get_entry() left which don't easily lend themselves to this >>> optimization.) >>> >>> Also adjust a few types as needed and remove stale comments from >>> p2m_pod_cache_add() (to avoid duplicating them yet another time). >>> >>> Signed-off-by: Jan Beulich >> >> So I was just looking at my own suggestion, and I want to withdraw my >> "reviewed-by" temporarily to ask a question... > > Okay, meaning we'd need to revert. > >>> @@ -649,7 +656,8 @@ p2m_pod_zero_check_superpage(struct p2m_ >>> p2m_type_t type, type0 = 0; >>> unsigned long * map = NULL; >>> int ret=0, reset = 0; >>> - int i, j; >>> + unsigned long i, n; >>> + unsigned int j; >>> int max_ref = 1; >>> struct domain *d = p2m->domain; >>> >>> @@ -668,10 +676,13 @@ p2m_pod_zero_check_superpage(struct p2m_ >>> >>> /* Look up the mfns, checking to make sure they're the same mfn >>> * and aligned, and mapping them. */ >>> - for ( i=0; i>> + for ( i = 0; i < SUPERPAGE_PAGES; i += n ) >>> { >>> p2m_access_t a; >>> - mfn = p2m->get_entry(p2m, gfn + i, &type, &a, 0, NULL, NULL); >>> + unsigned int cur_order; >>> + >>> + mfn = p2m->get_entry(p2m, gfn + i, &type, &a, 0, &cur_order, NULL); >>> + n = 1UL << min(cur_order, SUPERPAGE_ORDER + 0U); >>> >>> if ( i == 0 ) >>> { >> >> The check at the bottom of this loop not only checks to see that all >> the mfns are contiguous; they also check that each of the indivual >> (4k) mfns has certain properties; namely: >> >> + None of the mfns are used as pagetables, or allocated via xenheap >> + None of the mfns are likely to be mapped elsewhere (refcount 2 or >> less for shadow, 1 for hap) >> >> This change makes it so that if the p2m entry is 2M or larger, only >> the first mfn actually gets checked. But I don't think we can assume >> that just because the first page of a superpage is not used as a >> pagetable, or mapped elsewhere, that none of the pages are. (Please >> correct me if I'm wrong here.) >> >> If that's the case, then we would need to run this loop all the way >> through, even if cur_order is SUPERPAGE_ORDER. >> >> I suppose we could have two loops, one which checks for superpage >> integrity, and another which checks for the necessary properties for >> each individual mfn in the superpage. >> >> Thoughts? > > You're right - those checks not depending on individual page > attributes in that condition have misled me to assume all are of > that kind. We indeed do need an inner loop dealing with per-page > properties. I suppose you won't mind cleaning this up a little in the > course of the anyway necessary transformation. Yes, I've got a prototype patch I'll probably send out tomorrow. -George