lkml.org 
[lkml]   [2011]   [Feb]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH R3 7/7] xen/balloon: Memory hotplug support for Xen balloon driver
    From
    Date
    On Thu, 2011-02-03 at 17:30 +0100, Daniel Kiper wrote:
    > +static struct resource *allocate_memory_resource(unsigned long nr_pages)
    > +{
    > + resource_size_t r_min, r_size;
    > + struct resource *r;
    > +
    > + /*
    > + * Look for first unused memory region starting at page
    > + * boundary. Skip last memory section created at boot time
    > + * because it may contains unused memory pages with PG_reserved
    > + * bit not set (online_pages() require PG_reserved bit set).
    > + */

    Could you elaborate on this comment a bit? I think it's covering both
    the "PAGE_SIZE" argument to allocate_resource() and something else, but
    I'm not quite sure.

    > + r = kzalloc(sizeof(struct resource), GFP_KERNEL);
    > +
    > + if (!r)
    > + return NULL;
    > +
    > + r->name = "System RAM";
    > + r->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
    > + r_min = PFN_PHYS(section_nr_to_pfn(pfn_to_section_nr(balloon_stats.boot_max_pfn) + 1));

    Did you do this for alignment reasons? It might be a better idea to
    just make a nice sparsemem function to do alignment.

    > + r_size = nr_pages << PAGE_SHIFT;
    > +
    > + if (allocate_resource(&iomem_resource, r, r_size, r_min,
    > + ULONG_MAX, PAGE_SIZE, NULL, NULL) < 0) {
    > + kfree(r);
    > + return NULL;
    > + }
    > +
    > + return r;
    > +}

    This function should probably be made generic. I bet some more
    hypervisors come around and want to use this. They generally won't care
    where the memory goes, and the kernel can allocate a spot for them.

    ...
    > +static void hotplug_allocated_memory(struct resource *r)
    > {
    > - unsigned long target = balloon_stats.target_pages;
    > + int nid, rc;
    > + resource_size_t r_size;
    > + struct memory_block *mem;
    > + unsigned long pfn;
    > +
    > + r_size = r->end + 1 - r->start;
    > + nid = memory_add_physaddr_to_nid(r->start);
    > +
    > + rc = add_registered_memory(nid, r->start, r_size);
    > +
    > + if (rc) {
    > + pr_err("%s: add_registered_memory: Memory hotplug failed: %i\n",
    > + __func__, rc);
    > + balloon_stats.target_pages = balloon_stats.current_pages;
    > + return;
    > + }
    > +
    > + if (xen_pv_domain())
    > + for (pfn = PFN_DOWN(r->start); pfn < PFN_UP(r->end); ++pfn)
    > + if (!PageHighMem(pfn_to_page(pfn))) {
    > + rc = HYPERVISOR_update_va_mapping(
    > + (unsigned long)__va(pfn << PAGE_SHIFT),
    > + mfn_pte(pfn_to_mfn(pfn), PAGE_KERNEL), 0);
    > + BUG_ON(rc);
    > + }
    >
    > - target = min(target,
    > - balloon_stats.current_pages +
    > - balloon_stats.balloon_low +
    > - balloon_stats.balloon_high);
    > + rc = online_pages(PFN_DOWN(r->start), r_size >> PAGE_SHIFT);

    The memory hotplug code is a bit weird at first glance. It has two
    distinct stages: first, add_memory() is called from
    architecture-specific code. Later, online_pages() is called, but that
    part is driven by userspace.

    For all the other hotplug users online_pages() is done later, and
    triggered by userspace. The idea is that you could have memory sitting
    in "added, but offline" state which would be trivial to remove if
    someone else needed it, but also trivial to add without the need to
    allocate additional memory.

    In other words, I think you can take everything from and including
    online_pages() down in the function and take it out. Use a udev hotplug
    rule to online it immediately if that's what you want.

    > - return target;
    > + if (rc) {
    > + pr_err("%s: online_pages: Failed: %i\n", __func__, rc);
    > + balloon_stats.target_pages = balloon_stats.current_pages;
    > + return;
    > + }
    > +
    > + for (pfn = PFN_DOWN(r->start); pfn < PFN_UP(r->end); pfn += PAGES_PER_SECTION) {
    > + mem = find_memory_block(__pfn_to_section(pfn));
    > + BUG_ON(!mem);
    > + BUG_ON(!present_section_nr(mem->phys_index));
    > + mutex_lock(&mem->state_mutex);
    > + mem->state = MEM_ONLINE;
    > + mutex_unlock(&mem->state_mutex);
    > + }
    > +
    > + balloon_stats.current_pages += r_size >> PAGE_SHIFT;
    > }
    >
    > +static enum bp_state request_additional_memory(long credit)
    > +{
    > + int rc;
    > + static struct resource *r;
    > + static unsigned long pages_left;
    > +
    > + if ((credit <= 0 || balloon_stats.balloon_low ||
    > + balloon_stats.balloon_high) && !r)
    > + return BP_DONE;
    > +
    > + if (!r) {
    > + r = allocate_memory_resource(credit);
    > +
    > + if (!r)
    > + return BP_ERROR;
    > +
    > + pages_left = credit;
    > + }

    'r' is effectively a global variable here. Could you give it a more
    proper name? Maybe "last add location" or something. It might even
    make sense to move it up in to the global scope to make it _much_ more
    clear that it's not just locally used.

    > + rc = allocate_additional_memory(r, pages_left);
    > +
    > + if (rc < 0) {
    > + if (balloon_stats.mh_policy == MH_POLICY_TRY_UNTIL_SUCCESS)
    > + return BP_ERROR;
    > +
    > + r = adjust_memory_resource(r, pages_left);
    > +
    > + if (!r)
    > + return BP_ERROR;
    > + } else {
    > + pages_left -= rc;
    > +
    > + if (pages_left)
    > + return BP_HUNGRY;
    > + }
    > +
    > + hotplug_allocated_memory(r);
    > +
    > + r = NULL;
    > +
    > + return BP_DONE;
    > +}

    Could you explain a bit about why you chose to do this stuff with memory
    resources? Is that the only visibility that you have in to what memory
    the guest actually has?

    What troubles did you run in to when you did

    add_memory(0, balloon_stats.boot_max_pfn, credit);

    ?

    It's just that all the other memory hotplug users are _told_ by the
    hardware where to put things. Is that not the case here?

    -- Dave



    \
     
     \ /
      Last update: 2011-02-03 19:15    [W:0.034 / U:94.856 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site