This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


Re: [Xen-devel] [PATCH] xen: correctly rebuild mfn list list after migra

To: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] xen: correctly rebuild mfn list list after migration.
From: Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>
Date: Thu, 14 Oct 2010 07:55:34 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>
Delivery-date: Wed, 13 Oct 2010 23:56:14 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4CB6603F.6070703@xxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
Organization: Citrix Systems, Inc.
References: <1286878488-9027-1-git-send-email-ian.campbell@xxxxxxxxxx> <4CB6603F.6070703@xxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
On Thu, 2010-10-14 at 02:43 +0100, Jeremy Fitzhardinge wrote: 
> On 10/12/2010 03:14 AM, Ian Campbell wrote:
> >  static RESERVE_BRK_ARRAY(unsigned long **, p2m_top, P2M_TOP_PER_PAGE);
> >  static RESERVE_BRK_ARRAY(unsigned long, p2m_top_mfn, P2M_TOP_PER_PAGE);
> > +static RESERVE_BRK_ARRAY(unsigned long *, p2m_mid_mfn_p, P2M_TOP_PER_PAGE);
> This should be called "p2m_top_mfn_p" to have consistent naming, since
> its at the top of the hierarchy and is indexed by topidx.  The "mid" in
> the name makes it look like it should be indexed by mididx, but then it
> wouldn't make sense to have just one of these (since there needs to be a
> mid for each entry in top).  The fact that it points to mids is
> irrelevant (or at least implied by the fact that its a top).


> > @@ -322,14 +339,19 @@ void xen_build_mfn_list_list(void)
> >             mid = p2m_top[topidx];
> >  
> >             /* Don't bother allocating any mfn mid levels if
> > -              they're just missing */
> > -           if (mid[mididx] == p2m_missing)
> > +            * they're just missing, just update the stored mfn,
> > +            * since all could have changed over a migrate.
> > +            */
> > +           if (mid == p2m_mid_missing) {
> > +                   p2m_top_mfn[topidx] = virt_to_mfn(p2m_mid_missing);
> > +                   pfn += P2M_MID_PER_PAGE - 1;
> Why -1?  How does this interact with the "pfn += P2M_PER_PAGE" in the
> for loop?  Should it be "(P2M_MID_PER_PAGE - 1) * P2M_PER_PAGE", with
> the pfn += P2M_PER_PAGE moved to the bottom of the loop rather than in
> the header?

The -1 was supposed to offset the ++ in the for header, except as you
point out its actually a +=.

I'll figure out what the correct increment should be.

> Is this test even necessary?  Is it to save redundant re-testing of the
> mid level?

It avoids descending P2M_MID_PER_PAGE times per p2m_mis_missing top
level entry into leaf entries which we know are going to be p2m_missing
because they are referred to by p2m_mid_missing. Perhaps its a premature
optimisation, if the test and increment end up too complex once I've
fixed them I may just drop it.


Xen-devel mailing list