Re: [PATCH] xen: correctly rebuild mfn list list after migration.

[Ian Campbell <Ian.Campbell@eu.citrix.com>]

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).

OK.

> > @@ -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.

Ian.