Re: [Xen-devel] [PATCH V3 2/8] xen: Delay remapping memory of pv-domain

[Juergen Gross <jgross@xxxxxxxx>]

On 11/12/2014 10:45 PM, Konrad Rzeszutek Wilk wrote:
> On Tue, Nov 11, 2014 at 06:43:40AM +0100, Juergen Gross wrote:
> > diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> > index a8a1a3d..d3e492b 100644
> > --- a/arch/x86/xen/mmu.c
> > +++ b/arch/x86/xen/mmu.c
> > @@ -1223,6 +1223,10 @@ static void __init xen_pagetable_init(void)
> >         /* Allocate and initialize top and mid mfn levels for p2m structure */
> >         xen_build_mfn_list_list();
> >
> > +       /* Remap memory freed because of conflicts with E820 map */
> s/becasue of/due to
Okay.

> >         /* Boundary cross-over for the edges: */
> > -       p2m = extend_brk(PAGE_SIZE, PAGE_SIZE);
> > +       p2m = alloc_p2m_page();
> >
> >         p2m_init(p2m);
> >
> > @@ -640,7 +651,7 @@ static bool __init early_alloc_p2m_middle(unsigned long pfn)
> >
> >         mid = p2m_top[topidx];
> >         if (mid == p2m_mid_missing) {
> > -               mid = extend_brk(PAGE_SIZE, PAGE_SIZE);
> > +               mid = alloc_p2m_page();
> >
> >                 p2m_mid_init(mid, p2m_missing);
> >
> > @@ -649,100 +660,6 @@ static bool __init early_alloc_p2m_middle(unsigned long 
> > pfn)
> >         return true;
> >   }
> I would split this patch in two - one for the extend_brk/alloc_page conversation
> to alloc_p2m_page and free_page to free_p2m_page.
Okay.

> > -/* Buffer used to remap identity mapped pages */
> > -unsigned long xen_remap_buf[P2M_PER_PAGE] __initdata;
> > +/*
> > + * Buffer used to remap identity mapped pages. We only need the virtual space.
> Could you expand on the 'need the virtual space'?
I'll update the comment to:

/*
 * Buffer used to remap identity mapped pages. We only need the virtual
 * space. The physical page behind this address is remapped as needed to
 * different buffer pages.
 */

> .. snip..
> >   /*
> >    * This function updates the p2m and m2p tables with an identity map from
> > - * start_pfn to start_pfn+size and remaps the underlying RAM of the original
> > - * allocation at remap_pfn. It must do so carefully in P2M_PER_PAGE sized 
> > blocks
> > - * to not exhaust the reserved brk space. Doing it in properly aligned blocks
> > - * ensures we only allocate the minimum required leaf pages in the p2m table. 
> > It
> > - * copies the existing mfns from the p2m table under the 1:1 map, overwrites
> > - * them with the identity map and then updates the p2m and m2p tables with the
> > - * remapped memory.
> > + * start_pfn to start_pfn+size and prepares remapping the underlying RAM of the
> > + * original allocation at remap_pfn. The information needed for remapping is
> > + * saved in the memory itself to avoid the need for allocating buffers. The
> > + * complete remap information is contained in a list of MFNs each containing
> > + * up to REMAP_SIZE MFNs and the start target PFN for doing the remap.
> > + * This enables to preserve the original mfn sequence while doing the remapping
> us to
Yep.

> > + * at a time when the memory management is capable of allocating virtual and
> > + * physical memory in arbitrary amounts.
> You might want to add, see 'xen_remap_memory' and its callers.
Okay.

> > -               /* These two checks move from the start to end boundaries */
> > -               if (ident_boundary_pfn == ident_start_pfn_align)
> > -                       ident_boundary_pfn = ident_pfn_iter;
> > -               if (remap_boundary_pfn == remap_start_pfn_align)
> > -                       remap_boundary_pfn = remap_pfn_iter;
> > +               /* Map first pfn to xen_remap_buf */
> > +               mfn = pfn_to_mfn(ident_pfn_iter);
> > +               set_pte_mfn(buf, mfn, PAGE_KERNEL);
> So you set the buf to be point to 'mfn'.
Correct.

> > -               /* Check we aren't past the end */
> > -               BUG_ON(ident_boundary_pfn >= start_pfn + size);
> > -               BUG_ON(remap_boundary_pfn >= remap_pfn + size);
> > +               /* Save mapping information in page */
> > +               xen_remap_buf.next_area_mfn = xen_remap_mfn;
> > +               xen_remap_buf.target_pfn = remap_pfn_iter;
> > +               xen_remap_buf.size = chunk;
> > +               for (i = 0; i < chunk; i++)
> > +                       xen_remap_buf.mfns[i] = pfn_to_mfn(ident_pfn_iter + i);
> >
> > -               mfn = pfn_to_mfn(ident_boundary_pfn);
> > +               /* New element first in list */
> I don't get that comment. Don't you mean the MFN of the last chunk you
> had stashed the 'xen_remap_buf' structure in?
>
> The 'xen_remap_mfn' ends up being the the tail value of this
> "list".
I'll redo the comment:

/* Put remap buf into list. */

> > +/*
> > + * Remap the memory prepared in xen_do_set_identity_and_remap_chunk().
> > + */
> > +void __init xen_remap_memory(void)
> > +{
> > +       unsigned long buf = (unsigned long)&xen_remap_buf;
> > +       unsigned long mfn_save, mfn, pfn;
> > +       unsigned long remapped = 0, released = 0;
> > +       unsigned int i, free;
> > +       unsigned long pfn_s = ~0UL;
> > +       unsigned long len = 0;
> > +
> > +       mfn_save = virt_to_mfn(buf);
> > +
> > +       while (xen_remap_mfn != INVALID_P2M_ENTRY) {
> So the 'list' is constructed by going forward - that is from low-numbered
> PFNs to higher numbered ones. But the 'xen_remap_mfn' is going the
> other way - from the highest PFN to the lowest PFN.
>
> Won't that mean we will restore the chunks of memory in the wrong
> order? That is we will still restore them in chunks size, but the
> chunks will be in descending order instead of ascending?
No, the information where to put each chunk is contained in the chunk
data. I can add a comment explaining this.

> > +               /* Map the remap information */
> > +               set_pte_mfn(buf, xen_remap_mfn, PAGE_KERNEL);
> > +
> > +               BUG_ON(xen_remap_mfn != xen_remap_buf.mfns[0]);
> > +
> > +               free = 0;
> > +               pfn = xen_remap_buf.target_pfn;
> > +               for (i = 0; i < xen_remap_buf.size; i++) {
> > +                       mfn = xen_remap_buf.mfns[i];
> > +                       if (!released && xen_update_mem_tables(pfn, mfn)) {
> > +                               remapped++;
> If we fail 'xen_update_mem_tables' we will on the next chunk (so i+1) keep on
> freeing pages instead of trying to remap. Is that intentional? Could we
> try to remap?
Hmm, I'm not sure this is worth the effort. What could lead to failure
here? I suspect we could even just BUG() on failure. What do you think?


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel