WARNING - OLD ARCHIVES

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/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH R4 7/7] xen/balloon: Memory hotplug support for X

On Tue, 2011-03-08 at 22:50 +0100, Daniel Kiper wrote:
> +static enum bp_state reserve_additional_memory(long credit)
> +{
> +     int rc;
> +     unsigned long balloon_hotplug = credit;
> +
> +     balloon_hotplug <<= PAGE_SHIFT;
> +
> +     rc = add_virtual_memory((u64 *)&balloon_hotplug);

This would work if all 'unsigned long's were 64-bits.  It'll break on
32-bit kernels in a very bad way by overwriting 4 bytes of stack.

> +     if (rc) {
> +             pr_info("xen_balloon: %s: add_virtual_memory() failed: %i\n", 
> __func__, rc);
> +             return BP_EAGAIN;
> +     }
> +
> +     balloon_hotplug >>= PAGE_SHIFT;
> +
> +     balloon_hotplug -= credit;
> +
> +     balloon_stats.hotplug_pages += credit;
> +     balloon_stats.balloon_hotplug = balloon_hotplug;
> +
> +     return BP_DONE;
> +}
> +
> +static int xen_online_page_notifier(struct notifier_block *nb, unsigned long 
> val, void *v)
> +{
> +     struct page *page = v;
> +     unsigned long pfn = page_to_pfn(page);
> +
> +     if (pfn >= num_physpages)
> +             num_physpages = pfn + 1;
> +
> +     inc_totalhigh_pages();
> +
> +#ifdef CONFIG_FLATMEM
> +     max_mapnr = max(pfn, max_mapnr);
> +#endif

I really don't like that this is a direct copy of online_page() up to
this point.  They're already subtly different.  I'm also curious if this
breaks on 32-bit kernels because of the unconditional
inc_totalhigh_pages().

If it's done this way, I'd almost guarantee that the first time someone
fixes a bug or adds a generic feature in online_page() that Xen gets
missed.  

> +     mutex_lock(&balloon_mutex);
> +
> +     __balloon_append(page);
> +
> +     if (balloon_stats.hotplug_pages)
> +             --balloon_stats.hotplug_pages;
> +     else
> +             --balloon_stats.balloon_hotplug;
> +
> +     mutex_unlock(&balloon_mutex);
> +
> +     return NOTIFY_STOP;
> +}

I'm not a _huge_ fan of these notifier chains, but I guess it works.
However, if you're going to use these notifier chains, then we probably
should use them to full effect.  Have a notifier list like this:

        1. generic online_page()
        2. xen_online_page_notifier() (returns NOTIFY_STOP)
        3. free_online_page()

Where finish_online_page() does something like this:

finish_online_page(...)
{
        ClearPageReserved(page);
        init_page_count(page);
        __free_page(page);
}

These patches are definitely getting there.  Just another round or two,
and they should be ready to go.

-- Dave


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel