lkml.org 
[lkml]   [2014]   [Nov]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH V3 2/8] xen: Delay remapping memory of pv-domain
    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



    \
     
     \ /
      Last update: 2014-11-13 08:01    [W:4.854 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site