Hot/cold allocation -- swsusp can not handle hot pages

Hot/cold allocation -- swsusp can not handle hot pages

[Pavel Machek]

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

--- clean/kernel/suspend.c	2002-11-01 00:37:42.000000000 +0100
+++ linux-swsusp/kernel/suspend.c	2002-11-01 22:51:28.000000000 +0100
@@ -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++;
--- clean/mm/page_alloc.c	2002-11-01 00:37:44.000000000 +0100
+++ linux-swsusp/mm/page_alloc.c	2002-11-01 22:53:47.000000000 +0100
@@ -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];


-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?

Re: Hot/cold allocation -- swsusp can not handle hot pages

[William Lee Irwin III]

On Sat, Nov 02, 2002 at 07:19:00PM +0100, Pavel Machek wrote:
> 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?
> --- clean/mm/page_alloc.c	2002-11-01 00:37:44.000000000 +0100
> +++ linux-swsusp/mm/page_alloc.c	2002-11-01 22:53:47.000000000 +0100
> @@ -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

Re: Hot/cold allocation -- swsusp can not handle hot pages

[Pavel Machek]

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?
> > --- clean/mm/page_alloc.c	2002-11-01 00:37:44.000000000 +0100
> > +++ linux-swsusp/mm/page_alloc.c	2002-11-01 22:53:47.000000000 +0100
> > @@ -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.

I need pages I allocate to disappear from "is_head_of_free_region()",
so my counts match.

								Pavel
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Hot/cold allocation -- swsusp can not handle hot pages

[Andrew Morton]

Pavel Machek wrote:
> 
> 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?
> > > --- clean/mm/page_alloc.c   2002-11-01 00:37:44.000000000 +0100
> > > +++ linux-swsusp/mm/page_alloc.c    2002-11-01 22:53:47.000000000 +0100
> > > @@ -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.
> 
> 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.

Re: Hot/cold allocation -- swsusp can not handle hot pages

[Pavel Machek]

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?
> > > > --- clean/mm/page_alloc.c   2002-11-01 00:37:44.000000000 +0100
> > > > +++ linux-swsusp/mm/page_alloc.c    2002-11-01 22:53:47.000000000 +0100
> > > > @@ -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.
> > 
> > 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.

> 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
-- 
Worst form of spam? Adding advertisment signatures ala sourceforge.net.
What goes next? Inserting advertisment *into* email?

Re: Hot/cold allocation -- swsusp can not handle hot pages

[William Lee Irwin III]

At some point in the past, Andrew Morton wrote:
>> 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.

On Sun, Nov 03, 2002 at 09:08:09PM +0100, Pavel Machek wrote:
> 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

Re: Hot/cold allocation -- swsusp can not handle hot pages

[Andrew Morton]

Pavel Machek wrote:
> 
> ...
> "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.

Ah.  I see.
 
> 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?

> > 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.

Re: Hot/cold allocation -- swsusp can not handle hot pages

[Pavel Machek]

Hi!

> > ...
> > "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.
> 
> Ah.  I see.
>  
> > 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.


> > > 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.

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

-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.

Re: Hot/cold allocation -- swsusp can not handle hot pages

[William Lee Irwin III]

On Sun, Nov 03, 2002 at 09:08:09PM +0100, Pavel Machek wrote:
> "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

Re: Hot/cold allocation -- swsusp can not handle hot pages

[William Lee Irwin III]

On Sun, Nov 03, 2002 at 12:52:06PM -0800, William Lee Irwin III wrote:
> 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

Re: Hot/cold allocation -- swsusp can not handle hot pages

[Pavel Machek]

Hi!

> > "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:

Storing order in page->index is kind of hack, right?

> 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();
> }

You don't need to make it SMP-safe.. Plan is to hotremove cpus before
we enter this stage.

> 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?

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
-- 
Casualities in World Trade Center: ~3k dead inside the building,
cryptography in U.S.A. and free speech in Czech Republic.