[6/25] Merge pmdisk and swsusp

[6/25] Merge pmdisk and swsusp

[Patrick Mochel]

ChangeSet 1.1848, 2004/07/17 10:07:17-07:00, mochel@digitalimplant.org

[Power Mgmt] Consolidate code for allocating image pages in pmdisk and swsusp

- Move helpers calc_order(), alloc_pagedir(), alloc_image_pages(),
  enough_free_mem(), and enough_swap() into swsusp.
- Wrap them all with a new function - swsusp_alloc().
- Fix up pmdisk to just call that.
- Fix up suspend_prepare_image() to call that, instead of doing it inline.


 kernel/power/pmdisk.c |  161 +----------------------------------------
 kernel/power/swsusp.c |  196 +++++++++++++++++++++++++++++++++++---------------
 2 files changed, 143 insertions(+), 214 deletions(-)


diff -Nru a/kernel/power/pmdisk.c b/kernel/power/pmdisk.c
--- a/kernel/power/pmdisk.c	2004-07-17 14:51:37 -07:00
+++ b/kernel/power/pmdisk.c	2004-07-17 14:51:37 -07:00
@@ -350,138 +350,9 @@
 }


-/**
- *	free_image_pages - Free each page allocated for snapshot.
- */
-
-static void free_image_pages(void)
-{
-	struct pbe * p;
-	int i;
-
-	for (i = 0, p = pagedir_save; i < nr_copy_pages; i++, p++) {
-		ClearPageNosave(virt_to_page(p->address));
-		free_page(p->address);
-	}
-}
-
-
-/**
- *	free_pagedir - Free the page directory.
- */
-
-static void free_pagedir(void)
-{
-	free_image_pages();
-	free_pages((unsigned long)pagedir_save, pagedir_order);
-}
-
-
-static void calc_order(void)
-{
-	int diff;
-	int order;
-
-	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
-	nr_copy_pages += 1 << order;
-	do {
-		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
-		if (diff) {
-			order += diff;
-			nr_copy_pages += 1 << diff;
-		}
-	} while(diff);
-	pagedir_order = order;
-}
-
-
-/**
- *	alloc_pagedir - Allocate the page directory.
- *
- *	First, determine exactly how many contiguous pages we need,
- *	allocate them, then mark each 'unsavable'.
- */
-
-static int alloc_pagedir(void)
-{
-	calc_order();
-	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
-							     pagedir_order);
-	if(!pagedir_save)
-		return -ENOMEM;
-	memset(pagedir_save,0,(1 << pagedir_order) * PAGE_SIZE);
-	pagedir_nosave = pagedir_save;
-	return 0;
-}
-
-
-/**
- *	alloc_image_pages - Allocate pages for the snapshot.
- *
- */
-
-static int alloc_image_pages(void)
-{
-	struct pbe * p;
-	int i;
-
-	for (i = 0, p = pagedir_save; i < nr_copy_pages; i++, p++) {
-		p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
-		if(!p->address)
-			goto Error;
-		SetPageNosave(virt_to_page(p->address));
-	}
-	return 0;
- Error:
-	do {
-		if (p->address)
-			free_page(p->address);
-		p->address = 0;
-	} while (p-- > pagedir_save);
-	return -ENOMEM;
-}
-
-
-/**
- *	enough_free_mem - Make sure we enough free memory to snapshot.
- *
- *	Returns TRUE or FALSE after checking the number of available
- *	free pages.
- */
-
-static int enough_free_mem(void)
-{
-	if(nr_free_pages() < (nr_copy_pages + PAGES_FOR_IO)) {
-		pr_debug("pmdisk: Not enough free pages: Have %d\n",
-			 nr_free_pages());
-		return 0;
-	}
-	return 1;
-}
-
-
-/**
- *	enough_swap - Make sure we have enough swap to save the image.
- *
- *	Returns TRUE or FALSE after checking the total amount of swap
- *	space avaiable.
- *
- *	FIXME: si_swapinfo(&i) returns all swap devices information.
- *	We should only consider resume_device.
- */
-
-static int enough_swap(void)
-{
-	struct sysinfo i;
-
-	si_swapinfo(&i);
-	if (i.freeswap < (nr_copy_pages + PAGES_FOR_IO))  {
-		pr_debug("pmdisk: Not enough swap. Need %ld\n",i.freeswap);
-		return 0;
-	}
-	return 1;
-}

+extern int swsusp_alloc(void);
+extern void free_suspend_pagedir(unsigned long);

 /**
  *	pmdisk_suspend - Atomically snapshot the system.
@@ -503,33 +374,11 @@
 		return error;

 	drain_local_pages();
-
-	pagedir_nosave = NULL;
 	pr_debug("pmdisk: Counting pages to copy.\n" );
 	count_pages();
-
-	pr_debug("pmdisk: (pages needed: %d + %d free: %d)\n",
-		 nr_copy_pages,PAGES_FOR_IO,nr_free_pages());
-
-	if (!enough_free_mem())
-		return -ENOMEM;
-
-	if (!enough_swap())
-		return -ENOSPC;
-
-	if ((error = alloc_pagedir())) {
-		pr_debug("pmdisk: Allocating pagedir failed.\n");
-		return error;
-	}
-	if ((error = alloc_image_pages())) {
-		pr_debug("pmdisk: Allocating image pages failed.\n");
-		free_pagedir();
-		return error;
-	}
-
-	nr_copy_pages_check = nr_copy_pages;
-	pagedir_order_check = pagedir_order;

+	error = swsusp_alloc();
+
 	/* During allocating of suspend pagedir, new cold pages may appear.
 	 * Kill them
 	 */
@@ -931,7 +780,7 @@
 int pmdisk_free(void)
 {
 	pr_debug( "Freeing prev allocated pagedir\n" );
-	free_pagedir();
+	free_suspend_pagedir((unsigned long)pagedir_save);
 	return 0;
 }

diff -Nru a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c	2004-07-17 14:51:37 -07:00
+++ b/kernel/power/swsusp.c	2004-07-17 14:51:37 -07:00
@@ -594,7 +594,7 @@
 	}
 }

-static void free_suspend_pagedir(unsigned long this_pagedir)
+void free_suspend_pagedir(unsigned long this_pagedir)
 {
 	struct zone *zone;
 	for_each_zone(zone) {
@@ -604,36 +604,6 @@
 	free_pages(this_pagedir, pagedir_order);
 }

-static suspend_pagedir_t *create_suspend_pagedir(int nr_copy_pages)
-{
-	int i;
-	suspend_pagedir_t *pagedir;
-	struct pbe *p;
-	struct page *page;
-
-	pagedir_order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
-
-	p = pagedir = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD, pagedir_order);
-	if (!pagedir)
-		return NULL;
-
-	page = virt_to_page(pagedir);
-	for(i=0; i < 1<<pagedir_order; i++)
-		SetPageNosave(page++);
-
-	while(nr_copy_pages--) {
-		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++;
-	}
-	return pagedir;
-}
-
 static int prepare_suspend_processes(void)
 {
 	sys_sync();	/* Syncing needs pdflushd, so do it before stopping processes */
@@ -658,12 +628,145 @@
 	printk("|\n");
 }

-static int suspend_prepare_image(void)
+
+static void calc_order(void)
+{
+	int diff;
+	int order;
+
+	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
+	nr_copy_pages += 1 << order;
+	do {
+		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
+		if (diff) {
+			order += diff;
+			nr_copy_pages += 1 << diff;
+		}
+	} while(diff);
+	pagedir_order = order;
+}
+
+
+/**
+ *	alloc_pagedir - Allocate the page directory.
+ *
+ *	First, determine exactly how many contiguous pages we need,
+ *	allocate them, then mark each 'unsavable'.
+ */
+
+static int alloc_pagedir(void)
+{
+	calc_order();
+	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
+							     pagedir_order);
+	if(!pagedir_save)
+		return -ENOMEM;
+	memset(pagedir_save,0,(1 << pagedir_order) * PAGE_SIZE);
+	pagedir_nosave = pagedir_save;
+	return 0;
+}
+
+
+/**
+ *	alloc_image_pages - Allocate pages for the snapshot.
+ *
+ */
+
+static int alloc_image_pages(void)
+{
+	struct pbe * p;
+	int i;
+
+	for (i = 0, p = pagedir_save; i < nr_copy_pages; i++, p++) {
+		p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
+		if(!p->address)
+			goto Error;
+		SetPageNosave(virt_to_page(p->address));
+	}
+	return 0;
+ Error:
+	do {
+		if (p->address)
+			free_page(p->address);
+		p->address = 0;
+	} while (p-- > pagedir_save);
+	return -ENOMEM;
+}
+
+
+/**
+ *	enough_free_mem - Make sure we enough free memory to snapshot.
+ *
+ *	Returns TRUE or FALSE after checking the number of available
+ *	free pages.
+ */
+
+static int enough_free_mem(void)
+{
+	if(nr_free_pages() < (nr_copy_pages + PAGES_FOR_IO)) {
+		pr_debug("pmdisk: Not enough free pages: Have %d\n",
+			 nr_free_pages());
+		return 0;
+	}
+	return 1;
+}
+
+
+/**
+ *	enough_swap - Make sure we have enough swap to save the image.
+ *
+ *	Returns TRUE or FALSE after checking the total amount of swap
+ *	space avaiable.
+ *
+ *	FIXME: si_swapinfo(&i) returns all swap devices information.
+ *	We should only consider resume_device.
+ */
+
+static int enough_swap(void)
 {
 	struct sysinfo i;
-	unsigned int nr_needed_pages = 0;
+
+	si_swapinfo(&i);
+	if (i.freeswap < (nr_copy_pages + PAGES_FOR_IO))  {
+		pr_debug("pmdisk: Not enough swap. Need %ld\n",i.freeswap);
+		return 0;
+	}
+	return 1;
+}
+
+int swsusp_alloc(void)
+{
+	int error;
+
+	pr_debug("suspend: (pages needed: %d + %d free: %d)\n",
+		 nr_copy_pages,PAGES_FOR_IO,nr_free_pages());

 	pagedir_nosave = NULL;
+	if (!enough_free_mem())
+		return -ENOMEM;
+
+	if (!enough_swap())
+		return -ENOSPC;
+
+	if ((error = alloc_pagedir())) {
+		pr_debug("suspend: Allocating pagedir failed.\n");
+		return error;
+	}
+	if ((error = alloc_image_pages())) {
+		pr_debug("suspend: Allocating image pages failed.\n");
+		free_suspend_pagedir((unsigned long)pagedir_save);
+		return error;
+	}
+
+	nr_copy_pages_check = nr_copy_pages;
+	pagedir_order_check = pagedir_order;
+	return 0;
+}
+
+static int suspend_prepare_image(void)
+{
+	unsigned int nr_needed_pages = 0;
+
 	printk( "/critical section: ");
 #ifdef CONFIG_HIGHMEM
 	printk( "handling highmem" );
@@ -678,32 +781,9 @@
 	drain_local_pages();
 	nr_copy_pages = count_and_copy_data_pages(NULL);
 	nr_needed_pages = nr_copy_pages + PAGES_FOR_IO;
-
-	printk(" (pages needed: %d+%d=%d free: %d)\n",nr_copy_pages,PAGES_FOR_IO,nr_needed_pages,nr_free_pages());
-	if(nr_free_pages() < nr_needed_pages) {
-		printk(KERN_CRIT "%sCouldn't get enough free pages, on %d pages short\n",
-		       name_suspend, nr_needed_pages-nr_free_pages());
-		root_swap = 0xFFFF;
-		return -ENOMEM;
-	}
-	si_swapinfo(&i);	/* FIXME: si_swapinfo(&i) returns all swap devices information.
-				   We should only consider resume_device. */
-	if (i.freeswap < nr_needed_pages)  {
-		printk(KERN_CRIT "%sThere's not enough swap space available, on %ld pages short\n",
-		       name_suspend, nr_needed_pages-i.freeswap);
-		return -ENOSPC;
-	}
-
-	PRINTK( "Alloc pagedir\n" );
-	pagedir_save = pagedir_nosave = create_suspend_pagedir(nr_copy_pages);
-	if (!pagedir_nosave) {
-		/* Pagedir is big, one-chunk allocation. It is easily possible for this allocation to fail */
-		printk(KERN_CRIT "%sCouldn't allocate continuous pagedir\n", name_suspend);
-		return -ENOMEM;
-	}
-	nr_copy_pages_check = nr_copy_pages;
-	pagedir_order_check = pagedir_order;

+	swsusp_alloc();
+
 	drain_local_pages();	/* During allocating of suspend pagedir, new cold pages may appear. Kill them */
 	if (nr_copy_pages != count_and_copy_data_pages(pagedir_nosave))	/* copy */
 		BUG();

Re: [6/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> +static void calc_order(void)
> +{
> +	int diff;
> +	int order;
> +
> +	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
> +	nr_copy_pages += 1 << order;
> +	do {
> +		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
> +		if (diff) {
> +			order += diff;
> +			nr_copy_pages += 1 << diff;
> +		}
> +	} while(diff);
> +	pagedir_order = order;
> +}

This code is "interesting". Perhaps at least comment would be good
here?
									Pavel
-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [6/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> - Move helpers calc_order(), alloc_pagedir(), alloc_image_pages(),
>   enough_free_mem(), and enough_swap() into swsusp.


> +/**
> + *	enough_free_mem - Make sure we enough free memory to snapshot.
> + *
> + *	Returns TRUE or FALSE after checking the number of available
> + *	free pages.
> + */
> +
> +static int enough_free_mem(void)
> +{
> +	if(nr_free_pages() < (nr_copy_pages + PAGES_FOR_IO)) {
> +		pr_debug("pmdisk: Not enough free pages: Have %d\n",
> +			 nr_free_pages());
> +		return 0;
> +	}
> +	return 1;
> +}

> +	if (!enough_free_mem())
> +		return -ENOMEM;
> +
> +	if (!enough_swap())
> +		return -ENOSPC;
> +
> +	if ((error = alloc_pagedir())) {
> +		pr_debug("suspend: Allocating pagedir failed.\n");
> +		return error;
> +	}

Perhaps enough_free_* should return 0 / -ERROR to keep it consistent
with rest of code, no need to explain TRUE/FALSE etc?

-- 
Horseback riding is like software...
...vgf orggre jura vgf serr.

Re: [6/25] Merge pmdisk and swsusp

[Patrick Mochel]

Sorry about the delay; all the conferences are finally over (for now)

On Mon, 19 Jul 2004, Pavel Machek wrote:

> Hi!
>
> > +static void calc_order(void)
> > +{
> > +	int diff;
> > +	int order;
> > +
> > +	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
> > +	nr_copy_pages += 1 << order;
> > +	do {
> > +		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
> > +		if (diff) {
> > +			order += diff;
> > +			nr_copy_pages += 1 << diff;
> > +		}
> > +	} while(diff);
> > +	pagedir_order = order;
> > +}
>
> This code is "interesting". Perhaps at least comment would be good
> here?

Sure, patch below.



	Pat

diff -Nru a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c	2004-08-01 20:18:35 -07:00
+++ b/kernel/power/swsusp.c	2004-08-01 20:18:35 -07:00
@@ -659,13 +659,38 @@
 }


+/**
+ *	calc_order - Determine the order of allocation needed for pagedir_save.
+ *
+ *	This looks tricky, but is just subtle. Please fix it some time.
+ *	Since there are %nr_copy_pages worth of pages in the snapshot, we need
+ *	to allocate enough contiguous space to hold
+ *		(%nr_copy_pages * sizeof(struct pbe)),
+ *	which has the saved/orig locations of the page..
+ *
+ *	SUSPEND_PD_PAGES() tells us how many pages we need to hold those
+ *	structures, then we call get_bitmask_order(), which will tell us the
+ *	last bit set in the number, starting with 1. (If we need 30 pages, that
+ *	is 0x0000001e in hex. The last bit is the 5th, which is the order we
+ *	would use to allocate 32 contiguous pages).
+ *
+ *	Since we also need to save those pages, we add the number of pages that
+ *	we need to nr_copy_pages, and in case of an overflow, do the
+ *	calculation again to update the number of pages needed.
+ *
+ *	With this model, we will tend to waste a lot of memory if we just cross
+ *	an order boundary. Plus, the higher the order of allocation that we try
+ *	to do, the more likely we are to fail in a low-memory situtation
+ *	(though	we're unlikely to get this far in such a case, since swsusp
+ *	requires half of memory to be free anyway).
+ */
+
+
 static void calc_order(void)
 {
-	int diff;
-	int order;
+	int diff = 0;
+	int order = 0;

-	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
-	nr_copy_pages += 1 << order;
 	do {
 		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
 		if (diff) {
@@ -687,7 +712,7 @@
 static int alloc_pagedir(void)
 {
 	calc_order();
-	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
+	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
 							     pagedir_order);
 	if(!pagedir_save)
 		return -ENOMEM;

Re: [6/25] Merge pmdisk and swsusp

[Patrick Mochel]

On Mon, 19 Jul 2004, Pavel Machek wrote:

> > +/**
> > + *	enough_free_mem - Make sure we enough free memory to snapshot.
> > + *
> > + *	Returns TRUE or FALSE after checking the number of available
> > + *	free pages.
> > + */
> > +
> > +static int enough_free_mem(void)
> > +{
> > +	if(nr_free_pages() < (nr_copy_pages + PAGES_FOR_IO)) {
> > +		pr_debug("pmdisk: Not enough free pages: Have %d\n",
> > +			 nr_free_pages());
> > +		return 0;
> > +	}
> > +	return 1;
> > +}

> Perhaps enough_free_* should return 0 / -ERROR to keep it consistent
> with rest of code, no need to explain TRUE/FALSE etc?

Well, then they wouldn't read like plain language..

Besides, they're superfluous and racy anyway - the amount of memory and
swap space free could change at any time, so we should just remove them
and make sure our error handlin is correct later down the line when
allocations fail..


	Pat

Re: [6/25] Merge pmdisk and swsusp

[Mika Penttilä]

Patrick Mochel wrote:

>Sorry about the delay; all the conferences are finally over (for now)
>
>On Mon, 19 Jul 2004, Pavel Machek wrote:
>
>  
>
>>Hi!
>>
>>    
>>
>>>+static void calc_order(void)
>>>+{
>>>+	int diff;
>>>+	int order;
>>>+
>>>+	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
>>>+	nr_copy_pages += 1 << order;
>>>+	do {
>>>+		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
>>>+		if (diff) {
>>>+			order += diff;
>>>+			nr_copy_pages += 1 << diff;
>>>+		}
>>>+	} while(diff);
>>>+	pagedir_order = order;
>>>+}
>>>      
>>>
>>This code is "interesting". Perhaps at least comment would be good
>>here?
>>    
>>
>
>Sure, patch below.
>
>
>
>	Pat
>
>diff -Nru a/kernel/power/swsusp.c b/kernel/power/swsusp.c
>--- a/kernel/power/swsusp.c	2004-08-01 20:18:35 -07:00
>+++ b/kernel/power/swsusp.c	2004-08-01 20:18:35 -07:00
>@@ -659,13 +659,38 @@
> }
>
>
>+/**
>+ *	calc_order - Determine the order of allocation needed for pagedir_save.
>+ *
>+ *	This looks tricky, but is just subtle. Please fix it some time.
>+ *	Since there are %nr_copy_pages worth of pages in the snapshot, we need
>+ *	to allocate enough contiguous space to hold
>+ *		(%nr_copy_pages * sizeof(struct pbe)),
>+ *	which has the saved/orig locations of the page..
>+ *
>+ *	SUSPEND_PD_PAGES() tells us how many pages we need to hold those
>+ *	structures, then we call get_bitmask_order(), which will tell us the
>+ *	last bit set in the number, starting with 1. (If we need 30 pages, that
>+ *	is 0x0000001e in hex. The last bit is the 5th, which is the order we
>+ *	would use to allocate 32 contiguous pages).
>+ *
>+ *	Since we also need to save those pages, we add the number of pages that
>+ *	we need to nr_copy_pages, and in case of an overflow, do the
>+ *	calculation again to update the number of pages needed.
>+ *
>+ *	With this model, we will tend to waste a lot of memory if we just cross
>+ *	an order boundary. Plus, the higher the order of allocation that we try
>+ *	to do, the more likely we are to fail in a low-memory situtation
>+ *	(though	we're unlikely to get this far in such a case, since swsusp
>+ *	requires half of memory to be free anyway).
>+ */
>+
>+
> static void calc_order(void)
> {
>-	int diff;
>-	int order;
>+	int diff = 0;
>+	int order = 0;
>
>-	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
>-	nr_copy_pages += 1 << order;
> 	do {
> 		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
> 		if (diff) {
>@@ -687,7 +712,7 @@
> static int alloc_pagedir(void)
> {
> 	calc_order();
>-	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
>+	pagedir_save = (suspend_pagedir_t *)__get_free_pages(GFP_ATOMIC | __GFP_COLD,
> 							     pagedir_order);
> 	if(!pagedir_save)
> 		return -ENOMEM;
>-
>  
>

Why alloc twice for the saved pages, once in calc_order() and then in 
alloc_image_pages() ?


--Mika

Re: [6/25] Merge pmdisk and swsusp

[Patrick Mochel]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 324 bytes --]


On Mon, 2 Aug 2004, [ISO-8859-1] Mika Penttilä wrote:

> Why alloc twice for the saved pages, once in calc_order() and then in
> alloc_image_pages() ?

There is only one allocation each for both the pagedir (in
alloc_pagedir()) and the image pages (in alloc_image_pages()). So, I'm not
sure what you mean..

Thanks,


	Pat

Re: [6/25] Merge pmdisk and swsusp

[Mika Penttilä]

Patrick Mochel wrote:

>On Mon, 2 Aug 2004, [ISO-8859-1] Mika Penttil? wrote:
>
>  
>
>>Why alloc twice for the saved pages, once in calc_order() and then in
>>alloc_image_pages() ?
>>    
>>
>
>There is only one allocation each for both the pagedir (in
>alloc_pagedir()) and the image pages (in alloc_image_pages()). So, I'm not
>sure what you mean..
>
>Thanks,
>
>
>	Pat
>-
>  
>

Ah ok. It was the alloc pagedir space for the pagedir pages itself that 
confused me. This part was missing from swsusp before, I think.

--Mika

Re: [6/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> Sorry about the delay; all the conferences are finally over (for
> now)

:-). Well, I'm back from canoes for now, so I'm at least able to read
the mail...

> > > +static void calc_order(void)
> > > +{
> > > +	int diff;
> > > +	int order;
> > > +
> > > +	order = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages));
> > > +	nr_copy_pages += 1 << order;
> > > +	do {
> > > +		diff = get_bitmask_order(SUSPEND_PD_PAGES(nr_copy_pages)) - order;
> > > +		if (diff) {
> > > +			order += diff;
> > > +			nr_copy_pages += 1 << diff;
> > > +		}
> > > +	} while(diff);
> > > +	pagedir_order = order;
> > > +}
> >
> > This code is "interesting". Perhaps at least comment would be good
> > here?
> 
> Sure, patch below.

Looks good.
									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!

Re: [6/25] Merge pmdisk and swsusp

[Pavel Machek]

Hi!

> > > +/**
> > > + *	enough_free_mem - Make sure we enough free memory to snapshot.
> > > + *
> > > + *	Returns TRUE or FALSE after checking the number of available
> > > + *	free pages.
> > > + */
> > > +
> > > +static int enough_free_mem(void)
> > > +{
> > > +	if(nr_free_pages() < (nr_copy_pages + PAGES_FOR_IO)) {
> > > +		pr_debug("pmdisk: Not enough free pages: Have %d\n",
> > > +			 nr_free_pages());
> > > +		return 0;
> > > +	}
> > > +	return 1;
> > > +}
> 
> > Perhaps enough_free_* should return 0 / -ERROR to keep it consistent
> > with rest of code, no need to explain TRUE/FALSE etc?
> 
> Well, then they wouldn't read like plain language..

Hmm, it would probably need to be called check_free_mem...

> Besides, they're superfluous and racy anyway - the amount of memory and
> swap space free could change at any time, so we should just remove them
> and make sure our error handlin is correct later down the line when
> allocations fail..

I do not think this is doable. ... ... .. 

Yes, it probably is doable. We should mark ourselves as some kind of
"memory cleaning thread" so we get errors instead of deadlock when we
run out of memory while writing pages, and then we just need to handle
that errors, carefully. Ok.
									Pavel
-- 
People were complaining that M$ turns users into beta-testers...
...jr ghea gurz vagb qrirybcref, naq gurl frrz gb yvxr vg gung jnl!