Hot/cold allocation -- swsusp can not handle hot pages
Subsystems (2)
Hi!
Swsusp counts free pages, and relies on fact that when it allocates page there's one free page less. That is no longer true with hot pages.
I attempted to work it around but it seems I am getting hot pages even when I ask for cold one. This seems to fix it. Does it looks like "possibly mergable" patch?
Pavel
kernel/suspend.c +2 -2
@@ -549,7 +550,7 @@ pagedir_order =get_bitmask_order (SUSPEND_PD_PAGES (nr_copy_pages)); - p = pagedir = (suspend_pagedir_t *)__get_free_pages (GFP_ATOMIC , pagedir_order); + p = pagedir = (suspend_pagedir_t *)__get_free_pages (GFP_ATOMIC | __GFP_COLD, pagedir_order);if (!pagedir)return NULL ;
@@ -558,11 +559,11 @@SetPageNosave (page++);while (nr_copy_pages--) { - p->address =get_zeroed_page (GFP_ATOMIC ); + p->address =get_zeroed_page (GFP_ATOMIC | __GFP_COLD);if (!p->address) {free_suspend_pagedir ((unsigned long ) pagedir);return NULL ; }SetPageNosave (virt_to_page (p->address)); p->orig_address =0 ; p++;
mm/page_alloc.c +1 -1
@@ -361,7 +361,7 @@unsigned long flags;struct page *page =NULL ; -if (order ==0 ) { +if ((order ==0 ) && !cold) {struct per_cpu_pages *pcp; pcp = &zone->pageset[get_cpu ()].pcp[cold];
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net. What goes next? Inserting advertisment *into* email?
Quoted 10 lines · Pavel Machek
Swsusp counts free pages, and relies on fact that when it allocates page there's one free page less. That is no longer true with hot pages.
I attempted to work it around but it seems I am getting hot pages even when I ask for cold one. This seems to fix it. Does it looks like "possibly mergable" patch?
mm/page_alloc.c line 367
Quoted 8 lines
mm/page_alloc.c +1
-1@@ -361,7 +361,7 @@unsigned long flags;struct page *page =NULL ; -if (order ==0 ) { +if ((order ==0 ) && !cold) {struct per_cpu_pages *pcp; pcp = &zone->pageset[get_cpu ()].pcp[cold];
This doesn't seem to be doing what you want, even if it seems to work. If you want there to be one free page less, then allocating it will work regardless. What are you looking for besides that? If it's not already working you want some additional semantics. Could this involve is_head_of_free_region()? That should be solvable with a per-cpu list shootdown algorithm to fully merge all the buddy bitmap things.
Bill
Hi!
Quoted 16 lines
Quoted 16 lines
Swsusp counts free pages, and relies on fact that when it allocates page there's one free page less. That is no longer true with hot pages.
I attempted to work it around but it seems I am getting hot pages even when I ask for cold one. This seems to fix it. Does it looks like "possibly mergable" patch?mm/page_alloc.c +1
-1@@ -361,7 +361,7 @@unsigned long flags;struct page *page =NULL ; -if (order ==0 ) { +if ((order ==0 ) && !cold) {struct per_cpu_pages *pcp;
Quoted 9 lines
mm/page_alloc.c line 367
Quoted 2 lines
mm/page_alloc.c +0
-0pcp = &zone->pageset[get_cpu ()].pcp[cold];This doesn't seem to be doing what you want, even if it seems to work. If you want there to be one free page less, then allocating it will work regardless. What are you looking for besides that? If it's not already working you want some additional semantics. Could this involve
is_head_of_free_region()? That should be solvable with a per-cpu list shootdown algorithm to fully merge all the buddy bitmap things.
I need pages I allocate to disappear from "is_head_of_free_region()", so my counts match.
Pavel
Signature · 2 lines
Casualities in World Trade Center: ~3k dead inside the building, cryptography in U.S.A. and free speech in Czech Republic.
Quoted 29 lines · Pavel Machek
Hi!
Quoted 16 lines
Quoted 16 lines
Swsusp counts free pages, and relies on fact that when it allocates page there's one free page less. That is no longer true with hot pages.
I attempted to work it around but it seems I am getting hot pages even when I ask for cold one. This seems to fix it. Does it looks like "possibly mergable" patch?mm/page_alloc.c 2002-11-01 22:53:47.000000000 +0100 renamed from mm/page_alloc.c 2002-11-01 00:37:44.000000000 +0100 +1
-1@@ -361,7 +361,7 @@ unsigned long flags; struct page *page = NULL; - if (order == 0) { + if ((order == 0) && !cold) { struct per_cpu_pages *pcp;Quoted 9 lines
mm/page_alloc.c 2002-11-01 22:53:47.000000000 +0100 line 367
Quoted 2 lines
mm/page_alloc.c 2002-11-01 22:53:47.000000000 +0100 renamed from mm/page_alloc.c 2002-11-01 00:37:44.000000000 +0100 +0
-0pcp = &zone->pageset[get_cpu()].pcp[cold];This doesn't seem to be doing what you want, even if it seems to work. If you want there to be one free page less, then allocating it will work regardless. What are you looking for besides that? If it's not already working you want some additional semantics. Could this involve
is_head_of_free_region()? That should be solvable with a per-cpu list shootdown algorithm to fully merge all the buddy bitmap things.
Quoted 3 lines
I need pages I allocate to disappear from "
is_head_of_free_region()", so my counts match.
hm. swsusp does funny things. Would it be posible to get a big-picture "how this whole thing works" story? What exactly is the nature of its relationship with the page allocator?
I'm not really sure what to suggest here. Emptying the per-cpu page pools would be tricky. Maybe a swsusp-special page allocator which goes direct to the buddy lists or something.
Hi!
Quoted 26 lines
Quoted 26 lines
Quoted 25 lines
Quoted 10 lines
Swsusp counts free pages, and relies on fact that when it allocates page there's one free page less. That is no longer true with hot pages.
I attempted to work it around but it seems I am getting hot pages even when I ask for cold one. This seems to fix it. Does it looks like "possibly mergable" patch?mm/page_alloc.c 2002-11-01 22:53:47.000000000 +0100 line 367
Quoted 8 lines
mm/page_alloc.c 2002-11-01 22:53:47.000000000 +0100 renamed from mm/page_alloc.c 2002-11-01 00:37:44.000000000 +0100 +1
-1@@ -361,7 +361,7 @@ unsigned long flags; struct page *page = NULL; - if (order == 0) { + if ((order == 0) && !cold) { struct per_cpu_pages *pcp; pcp = &zone->pageset[get_cpu()].pcp[cold];This doesn't seem to be doing what you want, even if it seems to work. If you want there to be one free page less, then allocating it will work regardless. What are you looking for besides that? If it's not already working you want some additional semantics. Could this involve
is_head_of_free_region()? That should be solvable with a per-cpu list shootdown algorithm to fully merge all the buddy bitmap things.
Quoted 7 lines
Quoted 3 lines
I need pages I allocate to disappear from "
is_head_of_free_region()", so my counts match.hm. swsusp does funny things. Would it be posible to get a big-picture "how this whole thing works" story? What exactly is the nature of its relationship with the page allocator?
"big-picture" should be in Documentation/swsusp.txt...
*Should* be :-(. I need to copy all used memory, to make sure my snapshot is atomic.
Copying works by looking at what is allocated, counting needed pages, allocating 'directory' for them, allocating memory for copies, and actually copying.
When I suddenly find I have less data to copy than I thought, it screws up the code.
Quoted 3 lines
I'm not really sure what to suggest here. Emptying the per-cpu page pools would be tricky. Maybe a swsusp-special page allocator which goes direct to the buddy lists or something.
Well, see the patch above. That seems to do the trick for me. It seems that even "cold" allocation can give page from per-cpu pool. I thought that was a bug?
Pavel
Signature · 2 lines
Worst form of spam? Adding advertisment signatures ala sourceforge.net. What goes next? Inserting advertisment *into* email?
Quoted 3 lines · Andrew Morton
Quoted 3 lines
I'm not really sure what to suggest here. Emptying the per-cpu page pools would be tricky. Maybe a swsusp-special page allocator which goes direct to the buddy lists or something.
Quoted 3 lines · Pavel Machek
Well, see the patch above. That seems to do the trick for me. It seems that even "cold" allocation can give page from per-cpu pool. I thought that was a bug?
Not a bug. The semantics only imply a preference for merging, not a requirement to do so. A direct dequeue-from-buddy method either by adjusting watermarks or a specialized interface in combination with per-cpu list shootdown is required to enforce your invariants.
Bill
Quoted 10 lines · Pavel Machek
...
"big-picture" should be inDocumentation/swsusp.txt...*Should* be :-(. I need to copy all used memory, to make sure my snapshot is atomic.
Copying works by looking at what is allocated, counting needed pages, allocating 'directory' for them, allocating memory for copies, and actually copying.
Ah. I see.
Quoted 2 lines
When I suddenly find I have less data to copy than I thought, it screws up the code.
Having a less-than-expected amount to copy sounds like it can be safely handled?
Quoted 7 lines
Quoted 3 lines
I'm not really sure what to suggest here. Emptying the per-cpu page pools would be tricky. Maybe a swsusp-special page allocator which goes direct to the buddy lists or something.
Well, see the patch above. That seems to do the trick for me. It seems that even "cold" allocation can give page from per-cpu pool. I thought that was a bug?
There are two queues per cpu. cache-warm pages and cache-cold pages. The cold queue is mainly for lock amortisation, to avoid taking the zone lock once per page. But we can also allocate from the cold queue for situations where we'll be invalidating the cache anyway (file readahead). We don't want to waste cache-hot pages. Your change breaks that.
Hi!
Quoted 10 lines
Quoted 9 lines
...
"big-picture" should be inDocumentation/swsusp.txt...*Should* be :-(. I need to copy all used memory, to make sure my snapshot is atomic.
Copying works by looking at what is allocated, counting needed pages, allocating 'directory' for them, allocating memory for copies, and actually copying.
Quoted 7 lines
Ah. I see.
Quoted 2 lines
When I suddenly find I have less data to copy than I thought, it screws up the code.
Having a less-than-expected amount to copy sounds like it can be safely handled?
It means I have to kill some sanity checks and complicate things a little bit. Yes it can be done.
Quoted 8 lines
Quoted 7 lines
Quoted 3 lines
I'm not really sure what to suggest here. Emptying the per-cpu page pools would be tricky. Maybe a swsusp-special page allocator which goes direct to the buddy lists or something.
Well, see the patch above. That seems to do the trick for me. It seems that even "cold" allocation can give page from per-cpu pool. I thought that was a bug?
Quoted 6 lines
There are two queues per cpu. cache-warm pages and cache-cold pages. The cold queue is mainly for lock amortisation, to avoid taking the zone lock once per page. But we can also allocate from the cold queue for situations where we'll be invalidating the cache anyway (file readahead). We don't want to waste cache-hot pages. Your change breaks that.
I thought I was making it go to "cold" pages when user requested it, not to hot ones, but I did not read the code too much.
Pavel
Signature · 2 lines
Casualities in World Trade Center: ~3k dead inside the building, cryptography in U.S.A. and free speech in Czech Republic.
Quoted 8 lines · Pavel Machek
"big-picture" should be in
Documentation/swsusp.txt... *Should* be :-(. I need to copy all used memory, to make sure my snapshot is atomic.
Copying works by looking at what is allocated, counting needed pages, allocating 'directory' for them, allocating memory for copies, and actually copying.
When I suddenly find I have less data to copy than I thought, it screws up the code.
Why don't we just mark PG_nosave in page->flags and stuff order in page->index (or elsewhere) for you and then you can safely walk the zone->zone_mem_map? A single pass of marking up-front should be a bit quicker than searching the lists for a list head at every page, and also something more maintainable/comprehensible: we can basically know what we're dealing with and do a kind of "shutdown" of the stuff for you:
static void empty_pageset(struct zone *zone, struct per_cpu_pages *pcp)
{
pcp->batch = pcp->low = pcp->high = 1;
pcp->count -= free_pages_bulk(zone, pcp->batch, &pcp->list, 0);
}static void empty_cpu_pages(void *unused)
{
struct zone *zone;
unsigned long flags;
int cpu;
cpu = get_cpu();
local_irq_save(flags);
for_each_zone(zone) {
empty_pcp(zone, &zone->pageset[cpu].pcp[0]);
empty_pcp(zone, &zone->pageset[cpu].pcp[1]);
}
local_irq_restore(flags);
put_cpu();
}static void shootdown_per_cpu_pages(void)
{
smp_call_function(empty_cpu_pages, NULL, 0, 1);
empty_cpu_pages(NULL);
}void suspend_mark_free_pages(void)
{
struct zone *zone;
struct free_area *area;
struct page *page;
unsigned long flags;
int order;
shootdown_per_cpu_pages();
for_each_zone(zone) {
spin_lock_irqsave(&zone->lock, flags);
for (order = 0; order < MAX_ORDER; ++order) {
area = &zone->free_area[order];
list_for_each_entry(page, &area->free_list, list) {
SetPageNosave(page);
/* if ->index clashes, use flag bits */
page->index = order;
}
}
spin_unlock_irqrestore(&zone->lock, flags);
}
}Then you can just do
if (PageNosave(page)) pfn += 1 << page->index;
in kernel/suspend.c and everybody's happy, no?
Bill
Quoted 5 lines · William Lee Irwin III
static void
empty_pageset(struct zone *zone, struct per_cpu_pages *pcp){ pcp->batch = pcp->low = pcp->high = 1; pcp->count -= free_pages_bulk(zone, pcp->batch, &pcp->list, 0); }
Should be
pcp->count -= free_pages_bulk(zone, pcp->count, &pcp->list, 0);
Bill
Hi!
Quoted 9 lines
Quoted 8 lines
"big-picture" should be in
Documentation/swsusp.txt... *Should* be :-(. I need to copy all used memory, to make sure my snapshot is atomic.
Copying works by looking at what is allocated, counting needed pages, allocating 'directory' for them, allocating memory for copies, and actually copying.
When I suddenly find I have less data to copy than I thought, it screws up the code.
Quoted 7 lines
Why don't we just mark
PG_nosaveinpage->flagsand stuff order inpage->index(or elsewhere) for you and then you can safely walk thezone->zone_mem_map? A single pass of marking up-front should be a bit quicker than searching the lists for a list head at every page, and also something more maintainable/comprehensible: we can basically know what we're dealing with and do a kind of "shutdown" of the stuff for you:
Storing order in page->index is kind of hack, right?
Quoted 7 lines
static void
empty_pageset(struct zone *zone, struct per_cpu_pages *pcp){ pcp->batch = pcp->low = pcp->high = 1; pcp->count -= free_pages_bulk(zone, pcp->batch, &pcp->list, 0); }static void
empty_cpu_pages(void *unused)
Quoted 16 lines
{
struct zone *zone;
unsigned long flags;
int cpu;
cpu = get_cpu();
local_irq_save(flags);
for_each_zone(zone) {
empty_pcp(zone, &zone->pageset[cpu].pcp[0]);
empty_pcp(zone, &zone->pageset[cpu].pcp[1]);
}
local_irq_restore(flags);
put_cpu();
}You don't need to make it SMP-safe.. Plan is to hotremove cpus before we enter this stage.
Quoted 30 lines
static void
shootdown_per_cpu_pages(void){ smp_call_function(empty_cpu_pages, NULL, 0, 1); empty_cpu_pages(NULL); }void
suspend_mark_free_pages(void){ struct zone *zone; struct free_area *area; struct page *page; unsigned long flags; int order; shootdown_per_cpu_pages(); for_each_zone(zone) { spin_lock_irqsave(&zone->lock, flags); for (order = 0; order < MAX_ORDER; ++order) { area = &zone->free_area[order]; list_for_each_entry(page, &area->free_list, list) { SetPageNosave(page); /* if ->index clashes, use flag bits */ page->index = order; } } spin_unlock_irqrestore(&zone->lock, flags); } }
Quoted 6 lines
Then you can just do
if (PageNosave(page)) pfn += 1 << page->index;in
kernel/suspend.cand everybody's happy, no?
Well, empty_cpu_pages() seems like function I need to call before I do my current stuff, and it will start working. Thanx. I do not really want to store ->index anywhere... Its not performance critical.
I'll try that, probably tuesday.
Pavel
Signature · 2 lines
Casualities in World Trade Center: ~3k dead inside the building, cryptography in U.S.A. and free speech in Czech Republic.