[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1206751622.27091.20.camel@nimitz.home.sr71.net>
Date: Fri, 28 Mar 2008 17:47:02 -0700
From: Dave Hansen <dave@...ux.vnet.ibm.com>
To: Jeremy Fitzhardinge <jeremy@...p.org>
Cc: KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
Yasunori Goto <y-goto@...fujitsu.com>,
Christoph Lameter <clameter@....com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC] hotplug-memory: refactor online_pages to separate
zone growth from page onlining
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists