another attempt at making hotplug memory and xen play together

Apart from x86-32, all architectures use an identical online_page. Define it weakly in mm/memory_hotplug.c so that architectures can override it if necessary.

This is OK, but I do think the __weak stuff can make it a bit counter-intuitive to track down which version is actually getting used. Are there any people the currently override that __weak?

If not, we should probably just kill it.

Quoted 5 lines · Dave Hansen

This is OK, but I do think the __weak stuff can make it a bit counter-intuitive to track down which version is actually getting used. Are there any people the currently override that __weak?

If not, we should probably just kill it.

Yes, x86-32 has a different implementation.

I guess we could just make the copies conditional on CONFIG_HIGHMEM and define them both next to each other. It's really a highmem thing more than a per-arch thing.

Quoted 3 lines · Dave Hansen

I guess we could just make the copies conditional on CONFIG_HIGHMEM and define them both next to each other. It's really a highmem thing more than a per-arch thing.

Sounds good to me. On that subject, how different are the various arch_add_memory() functions really? There seems to be a lot of common stuff in there.

+static void grow_zone_span(unsigned long start_pfn, unsigned long end_pfn)

mm/memory_hotplug.c +13 -12

+{
        unsigned long old_zone_end_pfn;
+       struct zone *zone;
+       unsigned long flags;
+
+       /*
+        * This doesn't need a lock to do pfn_to_page().
+        * The section can't be removed here because of the
+        * memory_block->state_sem.
+        */
+       zone = page_zone(pfn_to_page(start_pfn));
+       pgdat_resize_lock(zone->zone_pgdat, &flags);
 
        zone_span_writelock(zone);

@@ -149,19 +171,9 @@
                                zone->zone_start_pfn;
 
        zone_span_writeunlock(zone);
-}
 
-static void grow_pgdat_span(struct pglist_data *pgdat,
-               unsigned long start_pfn, unsigned long end_pfn)
-{
-       unsigned long old_pgdat_end_pfn =
-               pgdat->node_start_pfn + pgdat->node_spanned_pages;
-
-       if (start_pfn < pgdat->node_start_pfn)
-               pgdat->node_start_pfn = start_pfn;
-
-       pgdat->node_spanned_pages = max(old_pgdat_end_pfn, end_pfn) -
-                                       pgdat->node_start_pfn;
+       grow_pgdat_span(zone->zone_pgdat, start_pfn, end_pfn);
+       pgdat_resize_unlock(zone->zone_pgdat, &flags);
 }
 
 static int online_pages_range(unsigned long start_pfn, unsigned long

nr_pages,

Quoted 38 lines · Jeremy Fitzhardinge

+static void grow_zone_span(unsigned long start_pfn, unsigned long end_pfn)

mm/memory_hotplug.c +13 -12

+{
        unsigned long old_zone_end_pfn;
+       struct zone *zone;
+       unsigned long flags;
+
+       /*
+        * This doesn't need a lock to do pfn_to_page().
+        * The section can't be removed here because of the
+        * memory_block->state_sem.
+        */
+       zone = page_zone(pfn_to_page(start_pfn));
+       pgdat_resize_lock(zone->zone_pgdat, &flags);
 
        zone_span_writelock(zone);

@@ -149,19 +171,9 @@
                                zone->zone_start_pfn;
 
        zone_span_writeunlock(zone);
-}
 
-static void grow_pgdat_span(struct pglist_data *pgdat,
-               unsigned long start_pfn, unsigned long end_pfn)
-{
-       unsigned long old_pgdat_end_pfn =
-               pgdat->node_start_pfn + pgdat->node_spanned_pages;
-
-       if (start_pfn < pgdat->node_start_pfn)
-               pgdat->node_start_pfn = start_pfn;
-
-       pgdat->node_spanned_pages = max(old_pgdat_end_pfn, end_pfn) -
-                                       pgdat->node_start_pfn;
+       grow_pgdat_span(zone->zone_pgdat, start_pfn, end_pfn);
+       pgdat_resize_unlock(zone->zone_pgdat, &flags);
 }
 

mm/memory_hotplug.c line 194

Quoted 6 lines

(quoted diff) +0 -0

 static int online_pages_range(unsigned long start_pfn, unsigned long

nr_pages,

mm/memory_hotplug.c +0 -0

@@ -180,16 +192,12 @@
        return 0;
 }
 

I don't particularly like this change to have grow_pgdat_span() called *from* grow_zone_span(). Seems backwards to me. But, this diff look funny (not your fault) so I may just be seeing things. :)

Last time I posted this patch you complained about my name "online_pages_zone", suggesting "grow_zone_span". Since that already existed, I took that as a hint to fold the two functions together. Is that not what you meant?

mm/memory_hotplug.c line 270

Quoted 10 lines · Jeremy Fitzhardinge

mm/memory_hotplug.c +8 -0

+int prepare_online_pages(unsigned long pfn, unsigned long nr_pages)
+{
+       int ret = notify_going_online(pfn, nr_pages);
+       if (ret)
+               return ret;
+
+       grow_zone_span(pfn, pfn+nr_pages);
        return 0;
+}

OK, after seeing this used in the Xen driver, I'm even less a fan of the name. Mostly because it doesn't actually prepare *pages* to be onlined. It prepares the zones/pgdats and notifies that pages might soon be online. Can you think of any better names?

grow_and_notify...??

It's also a bit funky because you're calling the online notifiers, but you're not actually onlining the pages, yet. Does that have any repercussions?

Quoted 17 lines · Dave Hansen

mm/memory_hotplug.c line 270

Quoted 10 lines · Jeremy Fitzhardinge

mm/memory_hotplug.c +8 -0

+int prepare_online_pages(unsigned long pfn, unsigned long nr_pages)
+{
+       int ret = notify_going_online(pfn, nr_pages);
+       if (ret)
+               return ret;
+
+       grow_zone_span(pfn, pfn+nr_pages);
        return 0;
+}

OK, after seeing this used in the Xen driver, I'm even less a fan of the name. Mostly because it doesn't actually prepare *pages* to be onlined. It prepares the zones/pgdats and notifies that pages might soon be online. Can you think of any better names?

Quoted 1 line

grow_and_notify...??

Does it even need to be a separately visible function? Could it just be part of add_memory()? Is there any reason delay doing it until online_pages()?

Quoted 3 lines

It's also a bit funky because you're calling the online notifiers, but you're not actually onlining the pages, yet. Does that have any repercussions?

No. It will always call the GOING_ONLINE notifier, but it will only call the ONLINE notifier if it actually bulk-onlines all the pages. In my page-by-page case, it will never end up calling the ONLINE notifier. I could call it repeatedly for each page, but I'm not sure how useful that is (the lack of any users of the ONLINE notifier makes it hard to judge).

mm/memory_hotplug.c +6 -0

+/* Mark a set of pages as online */
+unsigned long mark_pages_onlined(unsigned long pfn, unsigned long nr_pages)
+{
+	struct zone *zone = page_zone(pfn_to_page(pfn));
+	unsigned long onlined_pages = 0;
+	int need_zonelists_rebuild = 0;
 
 	/*
 	 * If this zone is not populated, then it is not in zonelist.

Quoted 13 lines

mm/memory_hotplug.c +6 -0

+/* Mark a set of pages as online */
+unsigned long mark_pages_onlined(unsigned long pfn, unsigned long nr_pages)
+{
+	struct zone *zone = page_zone(pfn_to_page(pfn));
+	unsigned long onlined_pages = 0;
+	int need_zonelists_rebuild = 0;
 
 	/*
 	 * If this zone is not populated, then it is not in zonelist.

@@ -240,10 +246,38 @@
 	vm_total_pages = nr_free_pagecache_pages();
 	writeback_set_ratelimit();
 

mm/memory_hotplug.c line 258

Quoted 8 lines

mm/memory_hotplug.c +6 -1

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

status_change_nid is to prepare data structures which are allocated on each NUMA node.

When memory less node gets new memory, and it changes status to normal memory, then status_change_nid must be set.

SLUB uses it now.

Add a per-section "section_ops" structure, allowing each section to have specific functions for onlining and offlining pages within the section. This is used by the Xen balloon driver to make sure that pages are not onlined without some physical memory backing them.

Quoted 5 lines · Jeremy Fitzhardinge

Add a per-section "section_ops" structure, allowing each section to have specific functions for onlining and offlining pages within the section. This is used by the Xen balloon driver to make sure that pages are not onlined without some physical memory backing them.

This is kinda a lot of code and mucking around for what we actually get out of it, especially since you just condensed down all the actual online_page() instances.

I think it might just be nicer to have a global list of these handlers somewhere. The Xen driver can just say "put me on the list of callbacks" and we'll call them at online_page(). I really don't think we need to be passing an ops structure around.

Quoted 6 lines · Dave Hansen

On Thu, 2008-04-03 at 17:05 -0700, Jeremy Fitzhardinge wrote:
I think it might just be nicer to have a global list of these handlers somewhere. The Xen driver can just say "put me on the list of callbacks" and we'll call them at online_page(). I really don't think we need to be passing an ops structure around.

Yes, but it seems a bit awkward. If we assume that:

1. Xen will be the only user of the hook, and
2. Xen-balloon hotplug is exclusive of real memory hotplug

then I guess its reasonable (though if that's the case it would be simpler to just put a direct call under #ifdef CONFIG_XEN_BALLOON in there).

But if we think there can be multiple callbacks, and they all get called on the online of each page, and there can be multiple kinds of hotplug memory it gets pretty messy. Each has to determine "why was I called on this page?" and you'd to work out which one actually does the job of onlining. It just seems cleaner to say "this section needs to be onlined like this", and there's no ambiguity.

I'm already anticipating using the ops mechanism to support another class of Xen hotplug memory for managing large pages.

Quoted 14 lines · Jeremy Fitzhardinge

Quoted 7 lines · Dave Hansen

On Thu, 2008-04-03 at 17:05 -0700, Jeremy Fitzhardinge wrote:
I think it might just be nicer to have a global list of these handlers somewhere. The Xen driver can just say "put me on the list of callbacks" and we'll call them at online_page(). I really don't think we need to be passing an ops structure around.

Yes, but it seems a bit awkward. If we assume that:

1. Xen will be the only user of the hook, and
2. Xen-balloon hotplug is exclusive of real memory hotplug

Quoted 2 lines

then I guess its reasonable (though if that's the case it would be simpler to just put a direct call under #ifdef CONFIG_XEN_BALLOON in there).

Yeah, I'm OK with something along those lines, too. I'd prefer sticking some stubs in a header and putting the #ifdef there, if only for aesthetic reasons.

Quoted 2 lines

But if we think there can be multiple callbacks, and they all get called on the online of each page, and there can be multiple kinds of hotplug memory it gets pretty messy. Each has to determine "why was I called on this page?" and you'd to work out which one actually does the job of onlining. It just seems cleaner to say "this section needs to be onlined like this", and there's no ambiguity.

I really wish we'd stop calling it "page online". :)

Let me think out loud for a sec here. Here's how memory hotplug works in a nutshell:

First step (add_memory() or probe time):
1. get more memory made available
2. create kva mapping for that memory (for lowmem)
3. allocate 'struct pages'

Second step, 'echo 1 > .../memoryXXX/online' time:
4. modify zone/pgdat spans (make the VM account for the memory)
5. Initialize the 'struct page'
6. free the memory into the buddy allocator

You can't do (2) because Xen doesn't allow mappings to be created until real backing is there. You've already done this, right?

You don't want to do (6) either, because there is no mapping for the page and it isn't committed in hardware, yet, so you don't want someone grabbing it *out* of the buddy allocator and using it.

Your solution is to take those first 1-5 steps, and had the balloon driver call them directly. The online_page() modifications are because 5/6 are a bit intertwined right now. Are we on the same page so far?

Why don't we just free the page back into the balloon driver? Take the existing steps 1-5, use them as they stand today, and just chop of step 6 for Xen. It'd save a bunch of this code churn and also stop us from proliferating any kind of per-config section-online behavior like you're asking about above.

That might also be generalizable to anyone else that wants the "fresh meat" newly-hotplugged memory. Large page users might be other consumers here.

Quoted 2 lines

I'm already anticipating using the ops mechanism to support another class of Xen hotplug memory for managing large pages.

Do tell. :)

Quoted 12 lines · Dave Hansen

First step (add_memory() or probe time):
1. get more memory made available
2. create kva mapping for that memory (for lowmem)
3. allocate 'struct pages'

Second step, 'echo 1 > .../memoryXXX/online' time:
4. modify zone/pgdat spans (make the VM account for the memory)
5. Initialize the 'struct page'
6. free the memory into the buddy allocator

You can't do (2) because Xen doesn't allow mappings to be created until real backing is there. You've already done this, right?

Quoted 4 lines

You don't want to do (6) either, because there is no mapping for the page and it isn't committed in hardware, yet, so you don't want someone grabbing it *out* of the buddy allocator and using it.

Right. I do that page by page in the balloon driver; each time I get a machine page, I bind it to the corresponding page structure and free it into the allocator.

And I skip the whole "echo online > /sys..." part, because its
redundant: the use of hotplug memory is an internal implementation detail of the balloon driver, which users needn't know about when they deal with the balloon driver.

Quoted 3 lines

That might also be generalizable to anyone else that wants the "fresh meat" newly-hotplugged memory. Large page users might be other consumers here.

Sure. The main problem is that 1-3 also ends up implicitly registering the new section with sysfs, so the bulk online interface becomes to usermode. If we make that optional (ie, a separate explicit call the memory-adder can choose to take) then everything is rosy.

Quoted 6 lines

Quoted 3 lines

I'm already anticipating using the ops mechanism to support another class of Xen hotplug memory for managing large pages.

Do tell. :)

I think I mentioned it before. If we 1) modify Xen to manage domain memory in large pages, 2) have a reasonably small section size, then we can reasonably do all memory management directly via the hotplug interface. Bringing each (large) page online would still require some explicit action, but it would be a much closer fit to how the hotplug machinery currently works. Then a small user or kernel mode policy daemon could use it to replicate the existing balloon driver's functionality.

I disagree that it is redundant. There is actually a lot of power there, and it was done for some very specific reasons. They're actually especially important to your particular use, which I'll try to explain.

Say that you have a very fragmented lowmem-only machine and you want to add a section of new mem_map[]. That's 512MB/PAGE_SIZE*sizeof(struct page), which is between 4 and 8MB. So, you need 8MB of contiguous pages for that mem_map[]. The way we designed it, you could always have that extra section sitting there, unused. You have that overhead of 8MB sitting there, but the benefit is that you can *ALWAYS* add memory.

I worry that the current patches as posted don't allow this kind of behavior. If there was ever a need to preallocate the space, it would need to do it inside the balloon driver itself and a way found to pass that into the hotplug code.

It is especially important for Xen because all of the other architectures, when getting a new section, magically have a large new contiguous area out of which to get a new mem_map[] or other structures. Xen doesn't actually have any contiguous memory at all since it will be adding a page at a time. Could you add some new functionality to the Xen driver and make sure to allocate the new section's mem_map[] inside the section? That would get around the problem.

Quoted 6 lines

Quoted 4 lines

That might also be generalizable to anyone else that wants the "fresh meat" newly-hotplugged memory. Large page users might be other consumers here.

Sure. The main problem is that 1-3 also ends up implicitly registering the new section with sysfs, so the bulk online interface becomes to usermode. If we make that optional (ie, a separate explicit call the memory-adder can choose to take) then everything is rosy.

OK, but the registering isn't a bad thing, either. You just don't want all of the pages to be stuck back into the allocator. You want to suck them into the balloon driver. Right?

Let's put a hook in online_page() to divert the new pages into your driver instead of into the allocator. Problem solved, right?

We might do this with a free_hotplugged_page() function. It can, in the initial introduction patch, just call free_page(). You can modify it later to do your bidding.

This sounds like a tall order considering Xen's current tight coupling between memory mapping and allocation, but I trust you and your colleagues to get it done. :)

Quoted 4 lines

Say that you have a very fragmented lowmem-only machine and you want to add a section of new mem_map[]. That's 512MB/PAGE_SIZE*sizeof(struct page), which is between 4 and 8MB. So, you need 8MB of contiguous pages for that mem_map[]. The way we designed it, you could always have that

Note that you could use 4k page size chunks for the memmap if its virtualized (CONFIG_SPARSEMEM_VMEMMAP). Instead of a single PMD pointing to a 2MB block you would have the PMD point to a block of ptes that would in turn point to the discontiguous series of 4k pages.

The memory is virtually contiguous so all the logic stays the same. What would be needed is some enhancements to the way the memmap is populated.

Quoted 4 lines · Jeremy Fitzhardinge

Add a per-section "section_ops" structure, allowing each section to have specific functions for onlining and offlining pages within the section. This is used by the Xen balloon driver to make sure that pages are not onlined without some physical memory backing them.

This is kinda a lot of code and mucking around for what we actually get out of it, especially since you just condensed down all the actual online_page() instances.

I think it might just be nicer to have a global list of these handlers somewhere. The Xen driver can just say "put me on the list of callbacks" and we'll call them at online_page(). I really don't think we need to be passing an ops structure around.

KAME, did you have some other ideas about this?

Quoted 16 lines · On Thu, 03 Apr 2008 17:51:12 -0700 Dave Hansen <[email protected]> wrote:

Quoted 5 lines · Jeremy Fitzhardinge

Add a per-section "section_ops" structure, allowing each section to have specific functions for onlining and offlining pages within the section. This is used by the Xen balloon driver to make sure that pages are not onlined without some physical memory backing them.

This is kinda a lot of code and mucking around for what we actually get out of it, especially since you just condensed down all the actual online_page() instances.

I think it might just be nicer to have a global list of these handlers somewhere. The Xen driver can just say "put me on the list of callbacks" and we'll call them at online_page(). I really don't think we need to be passing an ops structure around.

Quoted 3 lines

KAME, did you have some other ideas about this?

At first. please don't call online_page() handler in add_memory() phase. online_page() handler should be called in online_pages().

Passing handler to online_pages() is much easier and it's ok to me.