Re: [PATCH RFC] hotplug-memory: refactor online_pages to separate zone growth from page onlining

[Dave Hansen <dave@linux.vnet.ibm.com>]

On Fri, 2008-03-28 at 17:00 -0700, Jeremy Fitzhardinge wrote:
> The Xen balloon driver needs to separate the process of hot-installing
> memory into two phases: one to allocate the page structures and
> configure the zones, and another to actually online the pages of newly
> installed memory.
> 
> This patch splits up the innards of online_pages() into two pieces which
> correspond to these two phases.  The behaviour of online_pages() itself
> is unchanged.
...
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -180,31 +180,35 @@
>         return 0;
>  }
> 
> -
> -int online_pages(unsigned long pfn, unsigned long nr_pages)
> +/* Tell anyone who's interested that we're onlining some memory */
> +static int notify_going_online(unsigned long pfn, unsigned long nr_pages)
>  {
> -       unsigned long flags;
> -       unsigned long onlined_pages = 0;
> -       struct zone *zone;
> -       int need_zonelists_rebuild = 0;
> +       struct memory_notify arg;
>         int nid;
>         int ret;
> -       struct memory_notify arg;
> 
>         arg.start_pfn = pfn;
>         arg.nr_pages = nr_pages;
>         arg.status_change_nid = -1;
> -
> +
>         nid = page_to_nid(pfn_to_page(pfn));
>         if (node_present_pages(nid) == 0)
>                 arg.status_change_nid = nid;

That's kind a weird line in the patch.  How'd that get there?  Why are
you moving 'arg'?

This look OK, but it does add ~45 lines of code, and I'm not immediately
sure how you're going to use it.  Could you address that a bit?

I do kinda wish you'd take a real hard look at what the new functions
are doing now.  I'm not sure their current names are very good.

> +/* Grow the zone to fit the expected amount of memory being added */
> +static struct zone *online_pages_zone(unsigned long pfn, unsigned long nr_pages)

The comment is good, but the function name is not. :)  How about
grow_zone_span() or something?

> +/* Mark a set of pages as online */
> +unsigned long mark_pages_onlined(unsigned long pfn, unsigned long nr_pages)

Isn't the comment on this one a bit redundant? :)

This looks to me to have become the real online_pages() now.  This
function is what goes and individually onlines pages.  If someone was
trying to figure out whether to call online_pages() or
mark_pages_onlined(), which one would they know to call?

> -       if (onlined_pages)
> +       if (onlined_pages) {
> +               struct memory_notify arg;
> +
> +               arg.start_pfn = pfn;  /* ? */
> +               arg.nr_pages = onlined_pages;
> +               arg.status_change_nid = -1;  /* ? */
> +
>                 memory_notify(MEM_ONLINE, &arg);
> +       }

We should really wrap up memory notify:

static void memory_notify(int state, unsigned long start_pfn,
			  unsigned long nr_pages, int status_change_nid)
{
	struct memory_notify arg;
	arg.start_pfn = start_pfn;
	arg.nr_pages = nr_pages;
	arg.status_change_nid = status_change_nid;
	return the_current_memory_notify(state, &arg);
}

We can use that in a couple of spots, right?

-- Dave