On Mon, 2011-03-28 at 11:25 +0200, Daniel Kiper wrote:
> This patch contains online_page_chain and apropriate functions
> for registering/unregistering online page notifiers. It allows
> to do some machine specific tasks during online page stage which
> is required to implement memory hotplug in virtual machines.
> Additionally, __online_page_increment_counters() and
> __online_page_free() function was add to ease generic
> hotplug operation.
I really like that you added some symbolic constants there. It makes it
potentially a lot more readable.
My worry is that the next person who comes along is going to _really_
scratch their head asking why they would use:
OP_DO_NOT_INCREMENT_TOTAL_COUNTERS or: OP_INCREMENT_TOTAL_COUNTERS.
There aren't any code comments about it, and the patch description
doesn't really help.
In the end, we're only talking about a couple of lines of code for each
case (reordering the function a bit too):
void online_page(struct page *page)
{
// 1. pfn-based bits upping the max physical address markers:
unsigned long pfn = page_to_pfn(page);
if (pfn >= num_physpages)
num_physpages = pfn + 1;
#ifdef CONFIG_FLATMEM
max_mapnr = max(page_to_pfn(page), max_mapnr);
#endif
// 2. number of pages counters:
totalram_pages++;
#ifdef CONFIG_HIGHMEM
if (PageHighMem(page))
totalhigh_pages++;
#endif
// 3. preparing 'struct page' and freeing:
ClearPageReserved(page);
init_page_count(page);
__free_page(page);
}
Your stuff already extracted the free stuff very nicely. I think now we
just need to separate out the totalram_pages/totalhigh_pages bits from
the num_physpages/max_mapnr ones.
If done right, this should also help the totalram_pages/totalhigh_pages
go away balloon_retrieve(), and make Xen less likely to break in the
future. It also makes it immediately obvious why Xen skips incrementing
those counters: it does it later.
I also note that Xen has a copy of a part of online_page() in its
increase_reservation():
/* Relinquish the page back to the allocator. */
ClearPageReserved(page);
init_page_count(page);
__free_page(page);
That means that Xen is basically carrying an open-coded copy of
online_page() all by itself today.
-- Dave
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>