[PATCH 0/3] swsusp: code separation continued

[PATCH 0/3] swsusp: code separation continued

[Rafael J. Wysocki]

Hi,

The following series of patches continues the process of separating the
snapshot-handling code in swsusp from the storage-handling and core
code.

The patches are against the vanilla 2.6.14-rc5-mm1.  They have already been
acked by Pavel.  Please apply.

Greetings,
Rafael

[PATCH 1/3] swsusp: rework swsusp_suspend

[Rafael J. Wysocki]

This patch makes only the functions in swsusp.c call functions in snapshot.c
and not both ways.  Basically, it moves the code without changing its
functionality.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/power.h    |    2 -
 kernel/power/snapshot.c |   14 +---------
 kernel/power/swsusp.c   |   62 +++++++++++++++++++++++++++---------------------
 3 files changed, 39 insertions(+), 39 deletions(-)

Index: linux-2.6.14-rc5-mm1/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/snapshot.c	2005-10-28 23:46:36.000000000 +0200
+++ linux-2.6.14-rc5-mm1/kernel/power/snapshot.c	2005-10-28 23:49:15.000000000 +0200
@@ -89,7 +89,7 @@
 }
 
 
-static int save_highmem(void)
+int save_highmem(void)
 {
 	struct zone *zone;
 	int res = 0;
@@ -121,7 +121,7 @@
 	return 0;
 }
 #else
-static int save_highmem(void) { return 0; }
+int save_highmem(void) { return 0; }
 int restore_highmem(void) { return 0; }
 #endif /* CONFIG_HIGHMEM */
 
@@ -383,11 +383,6 @@
 	unsigned nr_pages;
 
 	pr_debug("swsusp: critical section: \n");
-	if (save_highmem()) {
-		printk(KERN_CRIT "swsusp: Not enough free pages for highmem\n");
-		restore_highmem();
-		return -ENOMEM;
-	}
 
 	drain_local_pages();
 	nr_pages = count_data_pages();
@@ -407,11 +402,6 @@
 		return -ENOMEM;
 	}
 
-	if (!enough_swap(nr_pages)) {
-		printk(KERN_ERR "swsusp: Not enough free swap\n");
-		return -ENOSPC;
-	}
-
 	pagedir_nosave = swsusp_alloc(nr_pages);
 	if (!pagedir_nosave)
 		return -ENOMEM;
Index: linux-2.6.14-rc5-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/swsusp.c	2005-10-28 23:46:36.000000000 +0200
+++ linux-2.6.14-rc5-mm1/kernel/power/swsusp.c	2005-10-28 23:49:15.000000000 +0200
@@ -507,6 +507,26 @@
 }
 
 /**
+ *	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(unsigned long nr_pages)
+{
+	struct sysinfo i;
+
+	si_swapinfo(&i);
+	pr_debug("swsusp: available swap: %lu pages\n", i.freeswap);
+	return i.freeswap > (nr_pages + PAGES_FOR_IO +
+		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
+}
+
+/**
  *	write_suspend_image - Write entire image and metadata.
  *
  */
@@ -514,6 +534,11 @@
 {
 	int error;
 
+	if (!enough_swap(nr_copy_pages)) {
+		printk(KERN_ERR "swsusp: Not enough free swap\n");
+		return -ENOSPC;
+	}
+
 	init_header();
 	if ((error = data_write()))
 		goto FreeData;
@@ -533,27 +558,6 @@
 	goto Done;
 }
 
-/**
- *	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.
- */
-
-int enough_swap(unsigned nr_pages)
-{
-	struct sysinfo i;
-
-	si_swapinfo(&i);
-	pr_debug("swsusp: available swap: %lu pages\n", i.freeswap);
-	return i.freeswap > (nr_pages + PAGES_FOR_IO +
-		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
-}
-
-
 /* It is important _NOT_ to umount filesystems at this point. We want
  * them synced (in case something goes wrong) but we DO not want to mark
  * filesystem clean: it is not. (And it does not matter, if we resume
@@ -576,6 +580,7 @@
 int swsusp_suspend(void)
 {
 	int error;
+
 	if ((error = arch_prepare_suspend()))
 		return error;
 	local_irq_disable();
@@ -587,15 +592,17 @@
 	 */
 	if ((error = device_power_down(PMSG_FREEZE))) {
 		printk(KERN_ERR "Some devices failed to power down, aborting suspend\n");
-		local_irq_enable();
-		return error;
+		goto Enable_irqs;
 	}
 
 	if ((error = swsusp_swap_check())) {
 		printk(KERN_ERR "swsusp: cannot find swap device, try swapon -a.\n");
-		device_power_up();
-		local_irq_enable();
-		return error;
+		goto Power_up;
+	}
+
+	if ((error = save_highmem())) {
+		printk(KERN_ERR "swsusp: Not enough free pages for highmem\n");
+		goto Restore_highmem;
 	}
 
 	save_processor_state();
@@ -603,8 +610,11 @@
 		printk(KERN_ERR "Error %d suspending\n", error);
 	/* Restore control flow magically appears here */
 	restore_processor_state();
+Restore_highmem:
 	restore_highmem();
+Power_up:
 	device_power_up();
+Enable_irqs:
 	local_irq_enable();
 	return error;
 }
Index: linux-2.6.14-rc5-mm1/kernel/power/power.h
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/power.h	2005-10-28 23:46:36.000000000 +0200
+++ linux-2.6.14-rc5-mm1/kernel/power/power.h	2005-10-28 23:49:15.000000000 +0200
@@ -65,8 +65,8 @@
 extern asmlinkage int swsusp_arch_suspend(void);
 extern asmlinkage int swsusp_arch_resume(void);
 
+extern int save_highmem(void);
 extern int restore_highmem(void);
 extern struct pbe * alloc_pagedir(unsigned nr_pages);
 extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
 extern void swsusp_free(void);
-extern int enough_swap(unsigned nr_pages);

[PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

This patch moves the snapshot-handling functions remaining in swsusp.c
to snapshot.c (ie. it moves the code without changing the functionality).

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/power.h    |    2 
 kernel/power/snapshot.c |  121 ++++++++++++++++++++++++++++++++++++++++++++++
 kernel/power/swsusp.c   |  126 ------------------------------------------------
 3 files changed, 125 insertions(+), 124 deletions(-)

Index: linux-2.6.14-rc5-mm1/kernel/power/power.h
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/power.h	2005-10-28 23:49:15.000000000 +0200
+++ linux-2.6.14-rc5-mm1/kernel/power/power.h	2005-10-28 23:50:11.000000000 +0200
@@ -69,4 +69,6 @@
 extern int restore_highmem(void);
 extern struct pbe * alloc_pagedir(unsigned nr_pages);
 extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
+extern int check_pagedir(struct pbe *pblist);
+extern struct pbe * swsusp_pagedir_relocate(struct pbe *pblist);
 extern void swsusp_free(void);
Index: linux-2.6.14-rc5-mm1/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/snapshot.c	2005-10-28 23:49:15.000000000 +0200
+++ linux-2.6.14-rc5-mm1/kernel/power/snapshot.c	2005-10-28 23:50:11.000000000 +0200
@@ -423,3 +423,124 @@
 	printk("swsusp: critical section/: done (%d pages copied)\n", nr_pages);
 	return 0;
 }
+
+/**
+ *	On resume, for storing the PBE list and the image,
+ *	we can only use memory pages that do not conflict with the pages
+ *	which had been used before suspend.
+ *
+ *	We don't know which pages are usable until we allocate them.
+ *
+ *	Allocated but unusable (ie eaten) memory pages are marked so that
+ *	swsusp_free() can release them
+ */
+
+unsigned long get_safe_page(unsigned gfp_mask)
+{
+	unsigned long m;
+
+	do {
+		m = get_zeroed_page(gfp_mask);
+		if (m && PageNosaveFree(virt_to_page(m)))
+			/* This is for swsusp_free() */
+			SetPageNosave(virt_to_page(m));
+	} while (m && PageNosaveFree(virt_to_page(m)));
+	if (m) {
+		/* This is for swsusp_free() */
+		SetPageNosave(virt_to_page(m));
+		SetPageNosaveFree(virt_to_page(m));
+	}
+	return m;
+}
+
+/**
+ *	check_pagedir - We ensure here that pages that the PBEs point to
+ *	won't collide with pages where we're going to restore from the loaded
+ *	pages later
+ */
+
+int check_pagedir(struct pbe *pblist)
+{
+	struct pbe *p;
+
+	/* This is necessary, so that we can free allocated pages
+	 * in case of failure
+	 */
+	for_each_pbe (p, pblist)
+		p->address = 0UL;
+
+	for_each_pbe (p, pblist) {
+		p->address = get_safe_page(GFP_ATOMIC);
+		if (!p->address)
+			return -ENOMEM;
+	}
+	return 0;
+}
+
+/**
+ *	swsusp_pagedir_relocate - It is possible, that some memory pages
+ *	occupied by the list of PBEs collide with pages where we're going to
+ *	restore from the loaded pages later.  We relocate them here.
+ */
+
+struct pbe * swsusp_pagedir_relocate(struct pbe *pblist)
+{
+	struct zone *zone;
+	unsigned long zone_pfn;
+	struct pbe *pbpage, *tail, *p;
+	void *m;
+	int rel = 0;
+
+	if (!pblist) /* a sanity check */
+		return NULL;
+
+	/* Clear page flags */
+
+	for_each_zone (zone) {
+        	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
+        		if (pfn_valid(zone_pfn + zone->zone_start_pfn))
+                		ClearPageNosaveFree(pfn_to_page(zone_pfn +
+					zone->zone_start_pfn));
+	}
+
+	/* Mark orig addresses */
+
+	for_each_pbe (p, pblist)
+		SetPageNosaveFree(virt_to_page(p->orig_address));
+
+	tail = pblist + PB_PAGE_SKIP;
+
+	/* Relocate colliding pages */
+
+	for_each_pb_page (pbpage, pblist) {
+		if (PageNosaveFree(virt_to_page((unsigned long)pbpage))) {
+			m = (void *)get_safe_page(GFP_ATOMIC | __GFP_COLD);
+			if (!m)
+				return NULL;
+			memcpy(m, (void *)pbpage, PAGE_SIZE);
+			if (pbpage == pblist)
+				pblist = (struct pbe *)m;
+			else
+				tail->next = (struct pbe *)m;
+			pbpage = (struct pbe *)m;
+
+			/* We have to link the PBEs again */
+			for (p = pbpage; p < pbpage + PB_PAGE_SKIP; p++)
+				if (p->next) /* needed to save the end */
+					p->next = p + 1;
+
+			rel++;
+		}
+		tail = pbpage + PB_PAGE_SKIP;
+	}
+
+	/* This is for swsusp_free() */
+	for_each_pb_page (pbpage, pblist) {
+		SetPageNosave(virt_to_page(pbpage));
+		SetPageNosaveFree(virt_to_page(pbpage));
+	}
+
+	printk("swsusp: Relocated %d pages\n", rel);
+
+	return pblist;
+}
Index: linux-2.6.14-rc5-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/swsusp.c	2005-10-28 23:49:15.000000000 +0200
+++ linux-2.6.14-rc5-mm1/kernel/power/swsusp.c	2005-10-28 23:50:11.000000000 +0200
@@ -645,130 +645,6 @@
 	return error;
 }
 
-/**
- *	On resume, for storing the PBE list and the image,
- *	we can only use memory pages that do not conflict with the pages
- *	which had been used before suspend.
- *
- *	We don't know which pages are usable until we allocate them.
- *
- *	Allocated but unusable (ie eaten) memory pages are marked so that
- *	swsusp_free() can release them
- */
-
-unsigned long get_safe_page(unsigned gfp_mask)
-{
-	unsigned long m;
-
-	do {
-		m = get_zeroed_page(gfp_mask);
-		if (m && PageNosaveFree(virt_to_page(m)))
-			/* This is for swsusp_free() */
-			SetPageNosave(virt_to_page(m));
-	} while (m && PageNosaveFree(virt_to_page(m)));
-	if (m) {
-		/* This is for swsusp_free() */
-		SetPageNosave(virt_to_page(m));
-		SetPageNosaveFree(virt_to_page(m));
-	}
-	return m;
-}
-
-/**
- *	check_pagedir - We ensure here that pages that the PBEs point to
- *	won't collide with pages where we're going to restore from the loaded
- *	pages later
- */
-
-static int check_pagedir(struct pbe *pblist)
-{
-	struct pbe *p;
-
-	/* This is necessary, so that we can free allocated pages
-	 * in case of failure
-	 */
-	for_each_pbe (p, pblist)
-		p->address = 0UL;
-
-	for_each_pbe (p, pblist) {
-		p->address = get_safe_page(GFP_ATOMIC);
-		if (!p->address)
-			return -ENOMEM;
-	}
-	return 0;
-}
-
-/**
- *	swsusp_pagedir_relocate - It is possible, that some memory pages
- *	occupied by the list of PBEs collide with pages where we're going to
- *	restore from the loaded pages later.  We relocate them here.
- */
-
-static struct pbe * swsusp_pagedir_relocate(struct pbe *pblist)
-{
-	struct zone *zone;
-	unsigned long zone_pfn;
-	struct pbe *pbpage, *tail, *p;
-	void *m;
-	int rel = 0;
-
-	if (!pblist) /* a sanity check */
-		return NULL;
-
-	pr_debug("swsusp: Relocating pagedir (%lu pages to check)\n",
-			swsusp_info.pagedir_pages);
-
-	/* Clear page flags */
-
-	for_each_zone (zone) {
-        	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
-        		if (pfn_valid(zone_pfn + zone->zone_start_pfn))
-                		ClearPageNosaveFree(pfn_to_page(zone_pfn +
-					zone->zone_start_pfn));
-	}
-
-	/* Mark orig addresses */
-
-	for_each_pbe (p, pblist)
-		SetPageNosaveFree(virt_to_page(p->orig_address));
-
-	tail = pblist + PB_PAGE_SKIP;
-
-	/* Relocate colliding pages */
-
-	for_each_pb_page (pbpage, pblist) {
-		if (PageNosaveFree(virt_to_page((unsigned long)pbpage))) {
-			m = (void *)get_safe_page(GFP_ATOMIC | __GFP_COLD);
-			if (!m)
-				return NULL;
-			memcpy(m, (void *)pbpage, PAGE_SIZE);
-			if (pbpage == pblist)
-				pblist = (struct pbe *)m;
-			else
-				tail->next = (struct pbe *)m;
-			pbpage = (struct pbe *)m;
-
-			/* We have to link the PBEs again */
-			for (p = pbpage; p < pbpage + PB_PAGE_SKIP; p++)
-				if (p->next) /* needed to save the end */
-					p->next = p + 1;
-
-			rel++;
-		}
-		tail = pbpage + PB_PAGE_SKIP;
-	}
-
-	/* This is for swsusp_free() */
-	for_each_pb_page (pbpage, pblist) {
-		SetPageNosave(virt_to_page(pbpage));
-		SetPageNosaveFree(virt_to_page(pbpage));
-	}
-
-	printk("swsusp: Relocated %d pages\n", rel);
-
-	return pblist;
-}
-
 /*
  *	Using bio to read from swap.
  *	This code requires a bit more work than just using buffer heads
@@ -1015,6 +891,8 @@
 
 	create_pbe_list(p, nr_copy_pages);
 
+	pr_debug("swsusp: Relocating pagedir (%lu pages to check)\n",
+			swsusp_info.pagedir_pages);
 	if (!(pagedir_nosave = swsusp_pagedir_relocate(p)))
 		return -ENOMEM;
 

[PATCH 3/3] swsusp: move swap check out of swsusp_suspend

[Rafael J. Wysocki]

This patch moves the check for available swap out of swsusp_suspend(),
which is necessary for separating the swap-handling functions in swsusp
from the core code.  No functionality changes.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/swsusp.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

Index: linux-2.6.14-rc5-mm1/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-rc5-mm1.orig/kernel/power/swsusp.c	2005-10-30 12:36:58.000000000 +0100
+++ linux-2.6.14-rc5-mm1/kernel/power/swsusp.c	2005-10-30 12:37:12.000000000 +0100
@@ -567,12 +567,15 @@
 {
 	int error;
 
+	if ((error = swsusp_swap_check())) {
+		printk(KERN_ERR "swsusp: cannot find swap device, try swapon -a.\n");
+		return error;
+	}
 	lock_swapdevices();
 	error = write_suspend_image();
 	/* This will unlock ignored swap devices since writing is finished */
 	lock_swapdevices();
 	return error;
-
 }
 
 
@@ -595,11 +598,6 @@
 		goto Enable_irqs;
 	}
 
-	if ((error = swsusp_swap_check())) {
-		printk(KERN_ERR "swsusp: cannot find swap device, try swapon -a.\n");
-		goto Power_up;
-	}
-
 	if ((error = save_highmem())) {
 		printk(KERN_ERR "swsusp: Not enough free pages for highmem\n");
 		goto Restore_highmem;
@@ -612,7 +610,6 @@
 	restore_processor_state();
 Restore_highmem:
 	restore_highmem();
-Power_up:
 	device_power_up();
 Enable_irqs:
 	local_irq_enable();
@@ -781,7 +778,7 @@
 		 * Reset swap signature now.
 		 */
 		error = bio_write_page(0, &swsusp_header);
-	} else { 
+	} else {
 		return -EINVAL;
 	}
 	if (!error)

Re: [PATCH 1/3] swsusp: rework swsusp_suspend

[Ingo Oeser]

[-- Attachment #1: Type: text/plain, Size: 1076 bytes --]

Hi Rafael,

On Sunday 30 October 2005 16:40, Rafael J. Wysocki wrote:
> This patch makes only the functions in swsusp.c call functions in snapshot.c
> and not both ways.  Basically, it moves the code without changing its
> functionality.
 
This is not quite true.

>  #else
> -static int save_highmem(void) { return 0; }
> +int save_highmem(void) { return 0; }
>  int restore_highmem(void) { return 0; }
>  #endif /* CONFIG_HIGHMEM */

Here you change code, which will be optimized completely away to
an empty function, which bloats the kernel.

Please put these two functions into a local header like this:

#ifdef CONFIG_HIGHMEM
int save_highmem(void);
int restore_highmem(void);
#else
static inline int save_highmem(void) { return 0; }
static inline int restore_highmem(void) { return 0; }
#endif


That way no having no highmem means, this code is not used at all
and everything using the return code and expecting != 0 is going
to be optimized away. 

I think everyone CCed will agree here :-)


Many thanks & Regards

Ingo Oeser


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> This patch moves the snapshot-handling functions remaining in swsusp.c
> to snapshot.c (ie. it moves the code without changing the functionality).
>

I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where
it is, and define "must not collide" as a part of interface. That will keep snapshot.c
smaller/simpler, and I plan to
eventually put responsibility for relocation to userspace.

That should simplify error handling at least: data structures
needed for relocation can be kept in userspace memory, and therefore
we do not risk memory leak in case something goes wrong.


				Pavel

-- 
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms         

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Sunday, 30 of October 2005 20:52, Pavel Machek wrote:
> Hi!
> 
> > This patch moves the snapshot-handling functions remaining in swsusp.c
> > to snapshot.c (ie. it moves the code without changing the functionality).
> >
> I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where
> it is, and define "must not collide" as a part of interface.

That's doable, but frankly I don't like the idea.

> That will keep snapshot.c smaller/simpler, and I plan to
> eventually put responsibility for relocation to userspace.

Please note that the relocating code uses the page flags to mark the allocated
pages as well as to avoid the pages that should not be used.  In my opinion
no userspace process should be allowed to fiddle with the page flags.

Moreover, get_safe_page() is called directly by the arch code on x86-64,
so it has to stay in the kernel and hence it should be in snapshot.c.
OTOH the relocating code is nothing more than "if the page is not safe,
use get_safe_page() to allocate one" kind of thing, so I don't see a point
in taking it out of the kernel (in the future) too.

> That should simplify error handling at least: data structures
> needed for relocation can be kept in userspace memory,

Well, after the patches that are already in -mm we don't use any additional
data structures for this purpose, so that's not a problem, I think. ;-)

> and therefore we do not risk memory leak in case something goes wrong.

We don't.  All memory allocated with either get_image_page() or
get_safe_page() will eventually be released by swsusp_free(), no matter
what happens.

Greetings,
Rafael

Re: [PATCH 1/3] swsusp: rework swsusp_suspend

[Rafael J. Wysocki]

Hi,

On Sunday, 30 of October 2005 18:54, Ingo Oeser wrote:
> Hi Rafael,
> 
> On Sunday 30 October 2005 16:40, Rafael J. Wysocki wrote:
> > This patch makes only the functions in swsusp.c call functions in snapshot.c
> > and not both ways.  Basically, it moves the code without changing its
> > functionality.
>  
> This is not quite true.
> 
> >  #else
> > -static int save_highmem(void) { return 0; }
> > +int save_highmem(void) { return 0; }
> >  int restore_highmem(void) { return 0; }
> >  #endif /* CONFIG_HIGHMEM */
> 
> Here you change code, which will be optimized completely away to
> an empty function, which bloats the kernel.
> 
> Please put these two functions into a local header like this:
> 
> #ifdef CONFIG_HIGHMEM
> int save_highmem(void);
> int restore_highmem(void);
> #else
> static inline int save_highmem(void) { return 0; }
> static inline int restore_highmem(void) { return 0; }
> #endif
> 
> 
> That way no having no highmem means, this code is not used at all
> and everything using the return code and expecting != 0 is going
> to be optimized away. 
> 
> I think everyone CCed will agree here :-)

Of course you're right, I'll do that.

Thanks a lot for the comment.

Greetings,
Rafael

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> > > This patch moves the snapshot-handling functions remaining in swsusp.c
> > > to snapshot.c (ie. it moves the code without changing the functionality).
> > >
> > I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where
> > it is, and define "must not collide" as a part of interface.
> 
> That's doable, but frankly I don't like the idea.
> 
> > That will keep snapshot.c smaller/simpler, and I plan to
> > eventually put responsibility for relocation to userspace.
> 
> Please note that the relocating code uses the page flags to mark the allocated
> pages as well as to avoid the pages that should not be used.  In my opinion
> no userspace process should be allowed to fiddle with the page
> flags.

Of course, userspace would have to use separate data structure. [Hash table?]

> Moreover, get_safe_page() is called directly by the arch code on x86-64,
> so it has to stay in the kernel and hence it should be in snapshot.c.
> OTOH the relocating code is nothing more than "if the page is not safe,
> use get_safe_page() to allocate one" kind of thing, so I don't see a point
> in taking it out of the kernel (in the future) too.

Well... for resume. If userspace does the allocation, it is:

userspace reads image
userspace relocates it
sys_atomic_restore(image)
if something goes wrong, userspace is clearly responsible for freeing
it.

How would you propose kernel<->user interface?

userspace reads pagedir
sys_these_pages_are_forbidden(pagedir)
userspace reads rest
sys_atomic_restore(image)
if something goes wrong, userspace must dealocate pages _and_ clear
forbidden flags?

> > That should simplify error handling at least: data structures
> > needed for relocation can be kept in userspace memory,
> 
> Well, after the patches that are already in -mm we don't use any additional
> data structures for this purpose, so that's not a problem, I
> think. ;-)

Until someone will want to get page flag bits back ;-), ok.

								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Sunday, 30 of October 2005 22:28, Pavel Machek wrote:
> Hi!
}-- snip --{
> > Please note that the relocating code uses the page flags to mark the allocated
> > pages as well as to avoid the pages that should not be used.  In my opinion
> > no userspace process should be allowed to fiddle with the page
> > flags.
> 
> Of course, userspace would have to use separate data structure. [Hash table?]

IMO a bitmap could be used.  Anyway in that case the x86-64 arch code
would need to have access either to this structure or to the image metadata,
because it must figure out which pages are not safe.  I don't see any simple
way of making this work ...

> > Moreover, get_safe_page() is called directly by the arch code on x86-64,
> > so it has to stay in the kernel and hence it should be in snapshot.c.
> > OTOH the relocating code is nothing more than "if the page is not safe,
> > use get_safe_page() to allocate one" kind of thing, so I don't see a point
> > in taking it out of the kernel (in the future) too.
> 
> Well... for resume. If userspace does the allocation, it is:
> 
> userspace reads image
> userspace relocates it
> sys_atomic_restore(image)
> if something goes wrong, userspace is clearly responsible for freeing
> it.
> 
> How would you propose kernel<->user interface?
> 
> userspace reads pagedir
> sys_these_pages_are_forbidden(pagedir)
> userspace reads rest
> sys_atomic_restore(image)
> if something goes wrong, userspace must dealocate pages _and_ clear
> forbidden flags?

Well, you have taken these things out of context.  Namely, the userspace
process cannot freeze the other tasks, suspend devices etc., so it has to
call the kernel for these purposes anyway.  Of course if something goes
wrong it has to call the kernel to revert these steps too.  Similarly it
can call the kernel to allocate the image memory and to free it in case
something's wrong.  For example, if the userspace initiates the resume:

- if (image not found)
	exit
- sys_freeze_processes /* this one will be tricky ;-) */
- sys_create_pagedir
- while (image data) {
	sys_put_this_stuff_where_appropriate(image data);
	/* Here the kernel will do the relocation etc. if necessary */
	if (something's wrong)
		goto Cleanup; }
- sys_atomic_restore /* suspend devices, disable IRQs, restore */
Cleanup: /* certainly something's gone wrong */
- sys_destroy_pagedir /* that's it */
- sys_resume_devices
- sys_thaw_processes

> > > That should simplify error handling at least: data structures
> > > needed for relocation can be kept in userspace memory,
> > 
> > Well, after the patches that are already in -mm we don't use any additional
> > data structures for this purpose, so that's not a problem, I
> > think. ;-)
> 
> Until someone will want to get page flag bits back ;-), ok.

In that case we'll have to redesign the snapshot part top-down anyway.

Greetings,
Rafael

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> > > Please note that the relocating code uses the page flags to mark the allocated
> > > pages as well as to avoid the pages that should not be used.  In my opinion
> > > no userspace process should be allowed to fiddle with the page
> > > flags.
> > 
> > Of course, userspace would have to use separate data structure. [Hash table?]
> 
> IMO a bitmap could be used.  Anyway in that case the x86-64 arch code
> would need to have access either to this structure or to the image metadata,
> because it must figure out which pages are not safe.  I don't see any simple
> way of making this work ...

Can you elaborate? resume is certainly going to get list of pbes...

> > > Moreover, get_safe_page() is called directly by the arch code on x86-64,
> > > so it has to stay in the kernel and hence it should be in snapshot.c.
> > > OTOH the relocating code is nothing more than "if the page is not safe,
> > > use get_safe_page() to allocate one" kind of thing, so I don't see a point
> > > in taking it out of the kernel (in the future) too.
> > 
> > Well... for resume. If userspace does the allocation, it is:
> > 
> > userspace reads image
> > userspace relocates it
> > sys_atomic_restore(image)
> > if something goes wrong, userspace is clearly responsible for freeing
> > it.
> > 
> > How would you propose kernel<->user interface?
> > 
> > userspace reads pagedir
> > sys_these_pages_are_forbidden(pagedir)
> > userspace reads rest
> > sys_atomic_restore(image)
> > if something goes wrong, userspace must dealocate pages _and_ clear
> > forbidden flags?
> 
> Well, you have taken these things out of context.  Namely, the userspace
> process cannot freeze the other tasks, suspend devices etc., so it
> has to

Yes, process freezing probably needs to be separate. Suspending
devices can well be part of atomic_snapshot operation; userspace does
not need to care.

> call the kernel for these purposes anyway.  Of course if something goes
> wrong it has to call the kernel to revert these steps too.  Similarly it
> can call the kernel to allocate the image memory and to free it in case
> something's wrong.  For example, if the userspace initiates the resume:
> 
> - if (image not found)
> 	exit
> - sys_freeze_processes /* this one will be tricky ;-) */

Why, I have it implemented? Just do not freeze the process calling you.

> - sys_create_pagedir

Ugly...

> - while (image data) {
> 	sys_put_this_stuff_where_appropriate(image data);
> 	/* Here the kernel will do the relocation etc. if necessary */
> 	if (something's wrong)
> 		goto Cleanup; }
> - sys_atomic_restore /* suspend devices, disable IRQs, restore */

Exactly. I'd like to go a

> Cleanup: /* certainly something's gone wrong */
> - sys_destroy_pagedir /* that's it */
> - sys_resume_devices

You should not need to do this one. resuming devices is going to be
integrated in atomic_restore, because suspending devices is there, too.

Here's how it looks... additionaly, I have ioctl for getting one
usable page. It is true that I did not solve error paths, yet; I'll
certainly need some way to free memory, too. 

							Pavel

int
do_resume(void)
{
	kmem = open("/dev/kmem", O_RDWR | O_LARGEFILE);
	image_fd = open(image, O_RDWR);

	if (kmem < 0) {
		fprintf(stderr, "Could not open /dev/kmem: %m\n");
		return 1;
	}

	memset(&swsusp_info, 0, sizeof(swsusp_info));
	read(image_fd, &swsusp_info, sizeof(swsusp_info));
	resume.nr_copy_pages = swsusp_info.nr_copy_pages;

	if (strcmp("swsusp3", swsusp_info.signature))
		exit(0);
	if (lseek(image_fd, 0, SEEK_SET) != 0) {
		printf("Could not seek to kill sig: %m\n");
		exit(1);
	}
	if (write(image_fd, &zeros, sizeof(swsusp_info)) != sizeof(swsusp_info)) {
		printf("Could not write to kill sig: %m\n");
		exit(1);
	}
	if (fsync(image_fd)) {
		printf("Could not fsync to kill sig: %m\n");
		exit(1);
	}
	printf("Got image, %d pages, signature [%s]\n", resume.nr_copy_pages, swsusp_info.signature);

	alloc_pagedir(resume.nr_copy_pages);
	printf("Verifying allocated pagedir: %d pages\n", walk_chain(&resume, NULL));
	printf("swsusp: Reading pagedir ");
	walk_pages_chain(&resume, (void *) read_pagedir_one);
	printf("ok\n");

	/* Need to be done twice; so that forbidden_pages comes into effect */
	alloc_pagedir(resume.nr_copy_pages);
	printf("Verifying allocated pagedir: %d pages\n", walk_chain(&resume, NULL));
	printf("swsusp: Reading pagedir ");
	walk_pages_chain(&resume, (void *) read_pagedir_one);
	printf("ok\n");

	printf("Verifying allocated pagedir: %d pages\n", walk_chain(&resume, NULL));

	/* FIXME: Need to relocate pages */
	mod = swsusp_info.nr_copy_pages / 100;
	if (!mod)
		mod = 1;
	printf("swsusp: Reading image data (%d pages):     ",
			swsusp_info.nr_copy_pages);
	walk_chain(&resume, data_read_one);
	printf("\b\b\b\bdone\n");

	if (ioctl(kmem, IOCTL_FREEZE, 0)) {
		fprintf(stderr, "Could not freeze system: %m\n");
		return 1;
	}

	if (ioctl(kmem, IOCTL_ATOMIC_RESTORE, &resume)) {
		fprintf(stderr, "Could not restore system: %m\n");
	}
	/* Ouch, at this point we'll appear in ATOMIC_SNAPSHOT syscall, if
	   things went ok... */

	return 0;
}

-- 
Thanks, Sharp!

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Monday, 31 of October 2005 00:04, Pavel Machek wrote:
> Hi!
> 
> > > > Please note that the relocating code uses the page flags to mark the allocated
> > > > pages as well as to avoid the pages that should not be used.  In my opinion
> > > > no userspace process should be allowed to fiddle with the page
> > > > flags.
> > > 
> > > Of course, userspace would have to use separate data structure. [Hash table?]
> > 
> > IMO a bitmap could be used.  Anyway in that case the x86-64 arch code
> > would need to have access either to this structure or to the image metadata,
> > because it must figure out which pages are not safe.  I don't see any simple
> > way of making this work ...
> 
> Can you elaborate? resume is certainly going to get list of pbes...

OK
On x86-64 we have to allocate a few safe pages to put the temporary page
tables on them.  In principle I can imagine the following code for this:

do {
	get a page;
	walk the list of pbes to verify that the page is safe;
	if (the page is not safe)
		keep track of it;
} while (the page is not safe)

but I'd rather not like to propose Andi to merge it. ;-)  Currently the x86-64 arch
code uses the same method of marking non-safe pages that is used by
the rest of swsusp for efficiency and I think it should stay this way.

}-- snip --{
> > 
> > Well, you have taken these things out of context.  Namely, the userspace
> > process cannot freeze the other tasks, suspend devices etc., so it
> > has to
> 
> Yes, process freezing probably needs to be separate. Suspending
> devices can well be part of atomic_snapshot operation; userspace does
> not need to care.
> 
> > call the kernel for these purposes anyway.  Of course if something goes
> > wrong it has to call the kernel to revert these steps too.  Similarly it
> > can call the kernel to allocate the image memory and to free it in case
> > something's wrong.  For example, if the userspace initiates the resume:
> > 
> > - if (image not found)
> > 	exit
> > - sys_freeze_processes /* this one will be tricky ;-) */
> 
> Why, I have it implemented? Just do not freeze the process calling you.

"tricky" != "impossible" ;-)

> > - sys_create_pagedir
> 
> Ugly...

Oh, it can be done on-the-fly in
sys_put_this_stuff_where_appropriate(image data) (at the expense of one
redundant check per call).

> > - while (image data) {
> > 	sys_put_this_stuff_where_appropriate(image data);
> > 	/* Here the kernel will do the relocation etc. if necessary */
> > 	if (something's wrong)
> > 		goto Cleanup; }
> > - sys_atomic_restore /* suspend devices, disable IRQs, restore */
> 
> Exactly. I'd like to go a
> 
> > Cleanup: /* certainly something's gone wrong */
> > - sys_destroy_pagedir /* that's it */
> > - sys_resume_devices
> 
> You should not need to do this one. resuming devices is going to be
> integrated in atomic_restore, because suspending devices is there, too.

Yes, but I need to thaw processes anyway, so I can release memory as well.
OTOH, if sys_atomic_restore fails because of the lack of memory, the memory
should be freed _before_ resuming devices, since otherwise subsequent
failures are almost certain to appear (I've seen what happens in that case).
Now, if the memory is allocated by the kernel, I can easily put an
emergency memory-freeing call in sys_atomic_restore (in that case
sys_destroy_pagedir will be redundant, but so what?).

> Here's how it looks... additionaly, I have ioctl for getting one
> usable page. It is true that I did not solve error paths, yet; I'll
> certainly need some way to free memory, too.

IMHO, these are important issues.
 
Greetings,
Rafael

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Sunday, 30 of October 2005 20:52, Pavel Machek wrote:
> Hi!
> 
> > This patch moves the snapshot-handling functions remaining in swsusp.c
> > to snapshot.c (ie. it moves the code without changing the functionality).
> >
> 
> I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where
> it is, and define "must not collide" as a part of interface. That will keep snapshot.c
> smaller/simpler,

Speaking of simplifications and having seen your code I hope you will agree with
the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code,
and replaces swsusp_pagedir_relocate with a simpler mechanism).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/power.h    |    6 +-
 kernel/power/snapshot.c |  121 +++++++++++++++++++++++++++++++++--------
 kernel/power/swsusp.c   |  139 ++----------------------------------------------
 3 files changed, 109 insertions(+), 157 deletions(-)

Index: linux-2.6.14-git3/kernel/power/power.h
===================================================================
--- linux-2.6.14-git3.orig/kernel/power/power.h	2005-10-31 20:27:30.000000000 +0100
+++ linux-2.6.14-git3/kernel/power/power.h	2005-10-31 20:29:31.000000000 +0100
@@ -66,7 +66,11 @@
 extern asmlinkage int swsusp_arch_resume(void);
 
 extern int restore_highmem(void);
-extern struct pbe * alloc_pagedir(unsigned nr_pages);
+extern void free_pagedir(struct pbe *pblist);
+extern struct pbe *alloc_pagedir(unsigned nr_pages, int need_safe);
 extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
+extern int alloc_data_pages(struct pbe *pblist, int need_safe);
 extern void swsusp_free(void);
 extern int enough_swap(unsigned nr_pages);
+extern void mark_unsafe_pages(struct pbe *pblist);
+extern void copy_page_backup_list(struct pbe *dst, struct pbe *src);
Index: linux-2.6.14-git3/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-git3.orig/kernel/power/snapshot.c	2005-10-31 20:27:30.000000000 +0100
+++ linux-2.6.14-git3/kernel/power/snapshot.c	2005-10-31 20:29:31.000000000 +0100
@@ -212,12 +212,44 @@
 	BUG_ON(pbe);
 }
 
+/**
+ *	@safe_needed - on resume, for storing the PBE list and the image,
+ *	we can only use memory pages that do not conflict with the pages
+ *	which had been used before suspend.
+ *
+ *	The unsafe pages are marked with the help of SetPageNosaveFree()
+ *	in mark_unsafe_pages()
+ *
+ *	Allocated but unusable (ie eaten) memory pages should be marked
+ *	so that swsusp_free() can release them
+ */
+
+static inline unsigned long snapshot_get_page(gfp_t gfp_mask, int safe_needed)
+{
+	unsigned long m;
+
+	if (safe_needed)
+		do {
+			m = get_zeroed_page(gfp_mask);
+			if (m && PageNosaveFree(virt_to_page(m)))
+				/* This is for swsusp_free() */
+				SetPageNosave(virt_to_page(m));
+		} while (m && PageNosaveFree(virt_to_page(m)));
+	else
+		m = get_zeroed_page(gfp_mask);
+	if (m) {
+		/* This is for swsusp_free() */
+		SetPageNosave(virt_to_page(m));
+		SetPageNosaveFree(virt_to_page(m));
+	}
+	return m;
+}
 
 /**
  *	free_pagedir - free pages allocated with alloc_pagedir()
  */
 
-static void free_pagedir(struct pbe *pblist)
+void free_pagedir(struct pbe *pblist)
 {
 	struct pbe *pbe;
 
@@ -270,22 +302,14 @@
 	pr_debug("create_pbe_list(): initialized %d PBEs\n", num);
 }
 
-static void *alloc_image_page(void)
+static inline void *alloc_image_page(int safe_needed)
 {
-	void *res = (void *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
-	if (res) {
-		SetPageNosave(virt_to_page(res));
-		SetPageNosaveFree(virt_to_page(res));
-	}
-	return res;
+	return (void *)snapshot_get_page(GFP_ATOMIC | __GFP_COLD, safe_needed);
 }
 
 /**
  *	alloc_pagedir - Allocate the page directory.
  *
- *	First, determine exactly how many pages we need and
- *	allocate them.
- *
  *	We arrange the pages in a chain: each page is an array of PBES_PER_PAGE
  *	struct pbe elements (pbes) and the last element in the page points
  *	to the next page.
@@ -293,7 +317,7 @@
  *	On each page we set up a list of struct_pbe elements.
  */
 
-struct pbe *alloc_pagedir(unsigned nr_pages)
+struct pbe *alloc_pagedir(unsigned nr_pages, int safe_needed)
 {
 	unsigned num;
 	struct pbe *pblist, *pbe;
@@ -302,12 +326,12 @@
 		return NULL;
 
 	pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
-	pblist = alloc_image_page();
+	pblist = alloc_image_page(safe_needed);
 	/* FIXME: rewrite this ugly loop */
 	for (pbe = pblist, num = PBES_PER_PAGE; pbe && num < nr_pages;
         		pbe = pbe->next, num += PBES_PER_PAGE) {
 		pbe += PB_PAGE_SKIP;
-		pbe->next = alloc_image_page();
+		pbe->next = alloc_image_page(safe_needed);
 	}
 	if (!pbe) { /* get_zeroed_page() failed */
 		free_pagedir(pblist);
@@ -316,6 +340,18 @@
 	return pblist;
 }
 
+int alloc_data_pages(struct pbe *pblist, int safe_needed)
+{
+	struct pbe *p;
+
+	for_each_pbe (p, pblist) {
+		p->address = (unsigned long)alloc_image_page(safe_needed);
+		if (!p->address)
+			return -ENOMEM;
+	}
+	return 0;
+}
+
 /**
  * Free pages we allocated for suspend. Suspend pages are alocated
  * before atomic copy, so we need to free them after resume.
@@ -355,24 +391,20 @@
 		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
 }
 
-
 static struct pbe *swsusp_alloc(unsigned nr_pages)
 {
-	struct pbe *pblist, *p;
+	struct pbe *pblist;
 
-	if (!(pblist = alloc_pagedir(nr_pages))) {
+	if (!(pblist = alloc_pagedir(nr_pages, 0))) {
 		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
 		return NULL;
 	}
 	create_pbe_list(pblist, nr_pages);
 
-	for_each_pbe (p, pblist) {
-		p->address = (unsigned long)alloc_image_page();
-		if (!p->address) {
-			printk(KERN_ERR "suspend: Allocating image pages failed.\n");
-			swsusp_free();
-			return NULL;
-		}
+	if (alloc_data_pages(pblist, 0)) {
+		printk(KERN_ERR "suspend: Allocating image pages failed.\n");
+		swsusp_free();
+		return NULL;
 	}
 
 	return pblist;
@@ -433,3 +465,44 @@
 	printk("swsusp: critical section/: done (%d pages copied)\n", nr_pages);
 	return 0;
 }
+
+/* Resume-related functions */
+
+void mark_unsafe_pages(struct pbe *pblist)
+{
+	struct zone *zone;
+	unsigned long zone_pfn;
+	struct pbe *p;
+
+	if (!pblist) /* a sanity check */
+		return;
+
+	/* Clear page flags */
+	for_each_zone (zone) {
+        	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
+        		if (pfn_valid(zone_pfn + zone->zone_start_pfn))
+                		ClearPageNosaveFree(pfn_to_page(zone_pfn +
+					zone->zone_start_pfn));
+	}
+
+	/* Mark orig addresses */
+	for_each_pbe (p, pblist)
+		SetPageNosaveFree(virt_to_page(p->orig_address));
+
+}
+
+void copy_page_backup_list(struct pbe *dst, struct pbe *src)
+{
+	/* We assume both lists contain the same number of elements */
+	while (src) {
+		dst->orig_address = src->orig_address;
+		dst->swap_address = src->swap_address;
+		dst = dst->next;
+		src = src->next;
+	}
+}
+
+unsigned long get_safe_page(gfp_t gfp_mask)
+{
+	return (unsigned long)snapshot_get_page(gfp_mask, 1);
+}
Index: linux-2.6.14-git3/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-git3.orig/kernel/power/swsusp.c	2005-10-31 20:27:30.000000000 +0100
+++ linux-2.6.14-git3/kernel/power/swsusp.c	2005-10-31 20:29:31.000000000 +0100
@@ -635,130 +635,6 @@
 	return error;
 }
 
-/**
- *	On resume, for storing the PBE list and the image,
- *	we can only use memory pages that do not conflict with the pages
- *	which had been used before suspend.
- *
- *	We don't know which pages are usable until we allocate them.
- *
- *	Allocated but unusable (ie eaten) memory pages are marked so that
- *	swsusp_free() can release them
- */
-
-unsigned long get_safe_page(gfp_t gfp_mask)
-{
-	unsigned long m;
-
-	do {
-		m = get_zeroed_page(gfp_mask);
-		if (m && PageNosaveFree(virt_to_page(m)))
-			/* This is for swsusp_free() */
-			SetPageNosave(virt_to_page(m));
-	} while (m && PageNosaveFree(virt_to_page(m)));
-	if (m) {
-		/* This is for swsusp_free() */
-		SetPageNosave(virt_to_page(m));
-		SetPageNosaveFree(virt_to_page(m));
-	}
-	return m;
-}
-
-/**
- *	check_pagedir - We ensure here that pages that the PBEs point to
- *	won't collide with pages where we're going to restore from the loaded
- *	pages later
- */
-
-static int check_pagedir(struct pbe *pblist)
-{
-	struct pbe *p;
-
-	/* This is necessary, so that we can free allocated pages
-	 * in case of failure
-	 */
-	for_each_pbe (p, pblist)
-		p->address = 0UL;
-
-	for_each_pbe (p, pblist) {
-		p->address = get_safe_page(GFP_ATOMIC);
-		if (!p->address)
-			return -ENOMEM;
-	}
-	return 0;
-}
-
-/**
- *	swsusp_pagedir_relocate - It is possible, that some memory pages
- *	occupied by the list of PBEs collide with pages where we're going to
- *	restore from the loaded pages later.  We relocate them here.
- */
-
-static struct pbe * swsusp_pagedir_relocate(struct pbe *pblist)
-{
-	struct zone *zone;
-	unsigned long zone_pfn;
-	struct pbe *pbpage, *tail, *p;
-	void *m;
-	int rel = 0;
-
-	if (!pblist) /* a sanity check */
-		return NULL;
-
-	pr_debug("swsusp: Relocating pagedir (%lu pages to check)\n",
-			swsusp_info.pagedir_pages);
-
-	/* Clear page flags */
-
-	for_each_zone (zone) {
-        	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
-        		if (pfn_valid(zone_pfn + zone->zone_start_pfn))
-                		ClearPageNosaveFree(pfn_to_page(zone_pfn +
-					zone->zone_start_pfn));
-	}
-
-	/* Mark orig addresses */
-
-	for_each_pbe (p, pblist)
-		SetPageNosaveFree(virt_to_page(p->orig_address));
-
-	tail = pblist + PB_PAGE_SKIP;
-
-	/* Relocate colliding pages */
-
-	for_each_pb_page (pbpage, pblist) {
-		if (PageNosaveFree(virt_to_page((unsigned long)pbpage))) {
-			m = (void *)get_safe_page(GFP_ATOMIC | __GFP_COLD);
-			if (!m)
-				return NULL;
-			memcpy(m, (void *)pbpage, PAGE_SIZE);
-			if (pbpage == pblist)
-				pblist = (struct pbe *)m;
-			else
-				tail->next = (struct pbe *)m;
-			pbpage = (struct pbe *)m;
-
-			/* We have to link the PBEs again */
-			for (p = pbpage; p < pbpage + PB_PAGE_SKIP; p++)
-				if (p->next) /* needed to save the end */
-					p->next = p + 1;
-
-			rel++;
-		}
-		tail = pbpage + PB_PAGE_SKIP;
-	}
-
-	/* This is for swsusp_free() */
-	for_each_pb_page (pbpage, pblist) {
-		SetPageNosave(virt_to_page(pbpage));
-		SetPageNosaveFree(virt_to_page(pbpage));
-	}
-
-	printk("swsusp: Relocated %d pages\n", rel);
-
-	return pblist;
-}
-
 /*
  *	Using bio to read from swap.
  *	This code requires a bit more work than just using buffer heads
@@ -905,9 +781,6 @@
 
 /**
  *	data_read - Read image pages from swap.
- *
- *	You do not need to check for overlaps, check_pagedir()
- *	already did that.
  */
 
 static int data_read(struct pbe *pblist)
@@ -997,20 +870,22 @@
 	int error = 0;
 	struct pbe *p;
 
-	if (!(p = alloc_pagedir(nr_copy_pages)))
+	if (!(p = alloc_pagedir(nr_copy_pages, 0)))
 		return -ENOMEM;
 
 	if ((error = read_pagedir(p)))
 		return error;
-
 	create_pbe_list(p, nr_copy_pages);
-
-	if (!(pagedir_nosave = swsusp_pagedir_relocate(p)))
+	mark_unsafe_pages(p);
+	if (!(pagedir_nosave = alloc_pagedir(nr_copy_pages, 1)))
 		return -ENOMEM;
+	create_pbe_list(pagedir_nosave, nr_copy_pages);
+	copy_page_backup_list(pagedir_nosave, p);
+	free_pagedir(p);
 
 	/* Allocate memory for the image and read the data from swap */
 
-	error = check_pagedir(pagedir_nosave);
+	error = alloc_data_pages(pagedir_nosave, 1);
 
 	if (!error)
 		error = data_read(pagedir_nosave);

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> > Can you elaborate? resume is certainly going to get list of pbes...
> 
> OK
> On x86-64 we have to allocate a few safe pages to put the temporary page
> tables on them.  In principle I can imagine the following code for this:
> 
> do {
> 	get a page;
> 	walk the list of pbes to verify that the page is safe;
> 	if (the page is not safe)
> 		keep track of it;
> } while (the page is not safe)
> 
> but I'd rather not like to propose Andi to merge it. ;-)  Currently the x86-64 arch
> code uses the same method of marking non-safe pages that is used by
> the rest of swsusp for efficiency and I think it should stay this
> way.

Ok, I see.

> > > - sys_create_pagedir
> > 
> > Ugly...
> 
> Oh, it can be done on-the-fly in
> sys_put_this_stuff_where_appropriate(image data) (at the expense of one
> redundant check per call).

Yes, but it is still ugly, as you keep some context across the
syscalls.

> > > Cleanup: /* certainly something's gone wrong */
> > > - sys_destroy_pagedir /* that's it */
> > > - sys_resume_devices
> > 
> > You should not need to do this one. resuming devices is going to be
> > integrated in atomic_restore, because suspending devices is there, too.
> 
> Yes, but I need to thaw processes anyway, so I can release memory as well.
> OTOH, if sys_atomic_restore fails because of the lack of memory, the memory
> should be freed _before_ resuming devices, since otherwise subsequent
> failures are almost certain to appear (I've seen what happens in that case).
> Now, if the memory is allocated by the kernel, I can easily put an
> emergency memory-freeing call in sys_atomic_restore (in that case
> sys_destroy_pagedir will be redundant, but so what?).

Ugh, I'd say "don't care about this one too much". If resume is
failing, we have bad problems anyway.

> > Here's how it looks... additionaly, I have ioctl for getting one
> > usable page. It is true that I did not solve error paths, yet; I'll
> > certainly need some way to free memory, too.
> 
> IMHO, these are important issues.

Yes, but I do not expect any problems while actually coding that...

								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> > > This patch moves the snapshot-handling functions remaining in swsusp.c
> > > to snapshot.c (ie. it moves the code without changing the functionality).
> > >
> > 
> > I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where
> > it is, and define "must not collide" as a part of interface. That will keep snapshot.c
> > smaller/simpler,
> 
> Speaking of simplifications and having seen your code I hope you will agree with
> the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code,
> and replaces swsusp_pagedir_relocate with a simpler mechanism).

...and also moves stuff around in a way

a) I don't like

and

b) is almost impossible to review

:-). Can you keep "relocate" code in swsusp.c, just making it simpler?

> @@ -997,20 +870,22 @@
>  	int error = 0;
>  	struct pbe *p;
>  
> -	if (!(p = alloc_pagedir(nr_copy_pages)))
> +	if (!(p = alloc_pagedir(nr_copy_pages, 0)))
>  		return -ENOMEM;
>  
>  	if ((error = read_pagedir(p)))
>  		return error;
> -
>  	create_pbe_list(p, nr_copy_pages);
> -
> -	if (!(pagedir_nosave = swsusp_pagedir_relocate(p)))
> +	mark_unsafe_pages(p);
> +	if (!(pagedir_nosave = alloc_pagedir(nr_copy_pages, 1)))
>  		return -ENOMEM;

Okay, this is probably better approach than copying pagedir around...

								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Monday, 31 of October 2005 23:02, Pavel Machek wrote:
> Hi!
> 
> > > > This patch moves the snapshot-handling functions remaining in swsusp.c
> > > > to snapshot.c (ie. it moves the code without changing the functionality).
> > > >
> > > 
> > > I'm sorry, but I acked this one too quickly. I'd prefer to keep "relocate" code where
> > > it is, and define "must not collide" as a part of interface. That will keep snapshot.c
> > > smaller/simpler,
> > 
> > Speaking of simplifications and having seen your code I hope you will agree with
> > the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code,
> > and replaces swsusp_pagedir_relocate with a simpler mechanism).
> 
> ...and also moves stuff around in a way
> 
> a) I don't like
> 
> and
> 
> b) is almost impossible to review

OK, I'll try to split it into two patches to make it cleaner.

> :-). Can you keep "relocate" code in swsusp.c, just making it simpler?

If you mean mark_unsafe_pages() and copy_backup_list(), no problem.
The rest is still there.

> 
> > @@ -997,20 +870,22 @@
> >  	int error = 0;
> >  	struct pbe *p;
> >  
> > -	if (!(p = alloc_pagedir(nr_copy_pages)))
> > +	if (!(p = alloc_pagedir(nr_copy_pages, 0)))
> >  		return -ENOMEM;
> >  
> >  	if ((error = read_pagedir(p)))
> >  		return error;
> > -
> >  	create_pbe_list(p, nr_copy_pages);
> > -
> > -	if (!(pagedir_nosave = swsusp_pagedir_relocate(p)))
> > +	mark_unsafe_pages(p);
> > +	if (!(pagedir_nosave = alloc_pagedir(nr_copy_pages, 1)))
> >  		return -ENOMEM;
> 
> Okay, this is probably better approach than copying pagedir around...

It's nice you agree here. :-)

Greetings,
Rafael

[PATCH 1/2] swsusp: reduce code duplication (was: Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c)

[Rafael J. Wysocki]

Hi,

On Tuesday, 1 of November 2005 13:57, Rafael J. Wysocki wrote:
}-- snip --{ 
> > > Speaking of simplifications and having seen your code I hope you will agree with
> > > the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code,
> > > and replaces swsusp_pagedir_relocate with a simpler mechanism).
> > 
> > ...and also moves stuff around in a way
> > 
> > a) I don't like
> > 
> > and
> > 
> > b) is almost impossible to review
> 
> OK, I'll try to split it into two patches to make it cleaner.

The first patch is appended, the next one will be in the reply to this message.

The changes made by the appended patch are necessary for the relocation
simplification in the next patch.  Still, the changes allow us to drop
check_pagedir() and make get_safe_page() be a one-line wrapper around
alloc_image_page() (get_safe_page() goes to snapshot.c, because
alloc_image_page() is static and it does not make sense to export it).

Greetings,
Rafael


Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/power.h    |    3 +-
 kernel/power/snapshot.c |   62 +++++++++++++++++++++++++++++++++++++-----------
 kernel/power/swsusp.c   |   57 +-------------------------------------------
 3 files changed, 52 insertions(+), 70 deletions(-)

Index: linux-2.6.14-git4/kernel/power/power.h
===================================================================
--- linux-2.6.14-git4.orig/kernel/power/power.h	2005-11-01 18:18:31.000000000 +0100
+++ linux-2.6.14-git4/kernel/power/power.h	2005-11-01 18:20:28.000000000 +0100
@@ -66,7 +66,8 @@
 extern asmlinkage int swsusp_arch_resume(void);
 
 extern int restore_highmem(void);
-extern struct pbe * alloc_pagedir(unsigned nr_pages);
+extern struct pbe *alloc_pagedir(unsigned nr_pages, gfp_t gfp_mask, int safe_needed);
 extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
 extern void swsusp_free(void);
+extern int alloc_data_pages(struct pbe *pblist, gfp_t gfp_mask, int safe_needed);
 extern int enough_swap(unsigned nr_pages);
Index: linux-2.6.14-git4/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-git4.orig/kernel/power/snapshot.c	2005-11-01 18:18:31.000000000 +0100
+++ linux-2.6.14-git4/kernel/power/snapshot.c	2005-11-01 18:20:28.000000000 +0100
@@ -270,9 +270,30 @@
 	pr_debug("create_pbe_list(): initialized %d PBEs\n", num);
 }
 
-static void *alloc_image_page(void)
+/**
+ *	@safe_needed - on resume, for storing the PBE list and the image,
+ *	we can only use memory pages that do not conflict with the pages
+ *	which had been used before suspend.
+ *
+ *	The unsafe pages are marked with the PG_nosave_free flag
+ *
+ *	Allocated but unusable (ie eaten) memory pages should be marked
+ *	so that swsusp_free() can release them
+ */
+
+static inline void *alloc_image_page(gfp_t gfp_mask, int safe_needed)
 {
-	void *res = (void *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
+	void *res;
+
+	if (safe_needed)
+		do {
+			res = (void *)get_zeroed_page(gfp_mask);
+			if (res && PageNosaveFree(virt_to_page(res)))
+				/* This is for swsusp_free() */
+				SetPageNosave(virt_to_page(res));
+		} while (res && PageNosaveFree(virt_to_page(res)));
+	else
+		res = (void *)get_zeroed_page(gfp_mask);
 	if (res) {
 		SetPageNosave(virt_to_page(res));
 		SetPageNosaveFree(virt_to_page(res));
@@ -280,6 +301,11 @@
 	return res;
 }
 
+unsigned long get_safe_page(gfp_t gfp_mask)
+{
+	return (unsigned long)alloc_image_page(gfp_mask, 1);
+}
+
 /**
  *	alloc_pagedir - Allocate the page directory.
  *
@@ -293,7 +319,7 @@
  *	On each page we set up a list of struct_pbe elements.
  */
 
-struct pbe *alloc_pagedir(unsigned nr_pages)
+struct pbe *alloc_pagedir(unsigned nr_pages, gfp_t gfp_mask, int safe_needed)
 {
 	unsigned num;
 	struct pbe *pblist, *pbe;
@@ -302,12 +328,12 @@
 		return NULL;
 
 	pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
-	pblist = alloc_image_page();
+	pblist = alloc_image_page(gfp_mask, safe_needed);
 	/* FIXME: rewrite this ugly loop */
 	for (pbe = pblist, num = PBES_PER_PAGE; pbe && num < nr_pages;
         		pbe = pbe->next, num += PBES_PER_PAGE) {
 		pbe += PB_PAGE_SKIP;
-		pbe->next = alloc_image_page();
+		pbe->next = alloc_image_page(gfp_mask, safe_needed);
 	}
 	if (!pbe) { /* get_zeroed_page() failed */
 		free_pagedir(pblist);
@@ -355,24 +381,32 @@
 		(nr_pages + PBES_PER_PAGE - 1) / PBES_PER_PAGE);
 }
 
+int alloc_data_pages(struct pbe *pblist, gfp_t gfp_mask, int safe_needed)
+{
+	struct pbe *p;
+
+	for_each_pbe (p, pblist) {
+		p->address = (unsigned long)alloc_image_page(gfp_mask, safe_needed);
+		if (!p->address)
+			return -ENOMEM;
+	}
+	return 0;
+}
 
 static struct pbe *swsusp_alloc(unsigned nr_pages)
 {
-	struct pbe *pblist, *p;
+	struct pbe *pblist;
 
-	if (!(pblist = alloc_pagedir(nr_pages))) {
+	if (!(pblist = alloc_pagedir(nr_pages, GFP_ATOMIC | __GFP_COLD, 0))) {
 		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
 		return NULL;
 	}
 	create_pbe_list(pblist, nr_pages);
 
-	for_each_pbe (p, pblist) {
-		p->address = (unsigned long)alloc_image_page();
-		if (!p->address) {
-			printk(KERN_ERR "suspend: Allocating image pages failed.\n");
-			swsusp_free();
-			return NULL;
-		}
+	if (alloc_data_pages(pblist, GFP_ATOMIC | __GFP_COLD, 0)) {
+		printk(KERN_ERR "suspend: Allocating image pages failed.\n");
+		swsusp_free();
+		return NULL;
 	}
 
 	return pblist;
Index: linux-2.6.14-git4/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-git4.orig/kernel/power/swsusp.c	2005-11-01 18:18:31.000000000 +0100
+++ linux-2.6.14-git4/kernel/power/swsusp.c	2005-11-01 18:20:28.000000000 +0100
@@ -636,59 +636,6 @@
 }
 
 /**
- *	On resume, for storing the PBE list and the image,
- *	we can only use memory pages that do not conflict with the pages
- *	which had been used before suspend.
- *
- *	We don't know which pages are usable until we allocate them.
- *
- *	Allocated but unusable (ie eaten) memory pages are marked so that
- *	swsusp_free() can release them
- */
-
-unsigned long get_safe_page(gfp_t gfp_mask)
-{
-	unsigned long m;
-
-	do {
-		m = get_zeroed_page(gfp_mask);
-		if (m && PageNosaveFree(virt_to_page(m)))
-			/* This is for swsusp_free() */
-			SetPageNosave(virt_to_page(m));
-	} while (m && PageNosaveFree(virt_to_page(m)));
-	if (m) {
-		/* This is for swsusp_free() */
-		SetPageNosave(virt_to_page(m));
-		SetPageNosaveFree(virt_to_page(m));
-	}
-	return m;
-}
-
-/**
- *	check_pagedir - We ensure here that pages that the PBEs point to
- *	won't collide with pages where we're going to restore from the loaded
- *	pages later
- */
-
-static int check_pagedir(struct pbe *pblist)
-{
-	struct pbe *p;
-
-	/* This is necessary, so that we can free allocated pages
-	 * in case of failure
-	 */
-	for_each_pbe (p, pblist)
-		p->address = 0UL;
-
-	for_each_pbe (p, pblist) {
-		p->address = get_safe_page(GFP_ATOMIC);
-		if (!p->address)
-			return -ENOMEM;
-	}
-	return 0;
-}
-
-/**
  *	swsusp_pagedir_relocate - It is possible, that some memory pages
  *	occupied by the list of PBEs collide with pages where we're going to
  *	restore from the loaded pages later.  We relocate them here.
@@ -997,7 +944,7 @@
 	int error = 0;
 	struct pbe *p;
 
-	if (!(p = alloc_pagedir(nr_copy_pages)))
+	if (!(p = alloc_pagedir(nr_copy_pages, GFP_ATOMIC, 0)))
 		return -ENOMEM;
 
 	if ((error = read_pagedir(p)))
@@ -1010,7 +957,7 @@
 
 	/* Allocate memory for the image and read the data from swap */
 
-	error = check_pagedir(pagedir_nosave);
+	error = alloc_data_pages(pagedir_nosave, GFP_ATOMIC, 1);
 
 	if (!error)
 		error = data_read(pagedir_nosave);

[PATCH 2/2] swsusp: simplify pagedir relocation

[Rafael J. Wysocki]

The appended patch simplifies the relocation of the page backup list
(aka pagedir) during resume.

Signed-off-by: Rafael J. Wysocki <rjw@sisk.pl>

 kernel/power/power.h    |    1 
 kernel/power/snapshot.c |    2 -
 kernel/power/swsusp.c   |   78 ++++++++++++++++--------------------------------
 3 files changed, 28 insertions(+), 53 deletions(-)

Index: linux-2.6.14-git4/kernel/power/swsusp.c
===================================================================
--- linux-2.6.14-git4.orig/kernel/power/swsusp.c	2005-11-01 18:20:28.000000000 +0100
+++ linux-2.6.14-git4/kernel/power/swsusp.c	2005-11-01 18:22:12.000000000 +0100
@@ -636,74 +636,43 @@
 }
 
 /**
- *	swsusp_pagedir_relocate - It is possible, that some memory pages
- *	occupied by the list of PBEs collide with pages where we're going to
- *	restore from the loaded pages later.  We relocate them here.
+ *	mark_unsafe_pages - mark the pages that cannot be used for storing
+ *	the image during resume, because they conflict with the pages that
+ *	had been used before suspend
  */
 
-static struct pbe * swsusp_pagedir_relocate(struct pbe *pblist)
+static void mark_unsafe_pages(struct pbe *pblist)
 {
 	struct zone *zone;
 	unsigned long zone_pfn;
-	struct pbe *pbpage, *tail, *p;
-	void *m;
-	int rel = 0;
+	struct pbe *p;
 
 	if (!pblist) /* a sanity check */
-		return NULL;
-
-	pr_debug("swsusp: Relocating pagedir (%lu pages to check)\n",
-			swsusp_info.pagedir_pages);
+		return;
 
 	/* Clear page flags */
-
 	for_each_zone (zone) {
-        	for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
-        		if (pfn_valid(zone_pfn + zone->zone_start_pfn))
-                		ClearPageNosaveFree(pfn_to_page(zone_pfn +
+		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
+			if (pfn_valid(zone_pfn + zone->zone_start_pfn))
+				ClearPageNosaveFree(pfn_to_page(zone_pfn +
 					zone->zone_start_pfn));
 	}
 
 	/* Mark orig addresses */
-
 	for_each_pbe (p, pblist)
 		SetPageNosaveFree(virt_to_page(p->orig_address));
 
-	tail = pblist + PB_PAGE_SKIP;
-
-	/* Relocate colliding pages */
-
-	for_each_pb_page (pbpage, pblist) {
-		if (PageNosaveFree(virt_to_page((unsigned long)pbpage))) {
-			m = (void *)get_safe_page(GFP_ATOMIC | __GFP_COLD);
-			if (!m)
-				return NULL;
-			memcpy(m, (void *)pbpage, PAGE_SIZE);
-			if (pbpage == pblist)
-				pblist = (struct pbe *)m;
-			else
-				tail->next = (struct pbe *)m;
-			pbpage = (struct pbe *)m;
-
-			/* We have to link the PBEs again */
-			for (p = pbpage; p < pbpage + PB_PAGE_SKIP; p++)
-				if (p->next) /* needed to save the end */
-					p->next = p + 1;
-
-			rel++;
-		}
-		tail = pbpage + PB_PAGE_SKIP;
-	}
+}
 
-	/* This is for swsusp_free() */
-	for_each_pb_page (pbpage, pblist) {
-		SetPageNosave(virt_to_page(pbpage));
-		SetPageNosaveFree(virt_to_page(pbpage));
+static void copy_page_backup_list(struct pbe *dst, struct pbe *src)
+{
+	/* We assume both lists contain the same number of elements */
+	while (src) {
+		dst->orig_address = src->orig_address;
+		dst->swap_address = src->swap_address;
+		dst = dst->next;
+		src = src->next;
 	}
-
-	printk("swsusp: Relocated %d pages\n", rel);
-
-	return pblist;
 }
 
 /*
@@ -949,10 +918,15 @@
 
 	if ((error = read_pagedir(p)))
 		return error;
-
 	create_pbe_list(p, nr_copy_pages);
-
-	if (!(pagedir_nosave = swsusp_pagedir_relocate(p)))
+	mark_unsafe_pages(p);
+	pagedir_nosave = alloc_pagedir(nr_copy_pages, GFP_ATOMIC, 1);
+	if (pagedir_nosave) {
+		create_pbe_list(pagedir_nosave, nr_copy_pages);
+		copy_page_backup_list(pagedir_nosave, p);
+	}
+	free_pagedir(p);
+	if (!pagedir_nosave)
 		return -ENOMEM;
 
 	/* Allocate memory for the image and read the data from swap */
Index: linux-2.6.14-git4/kernel/power/power.h
===================================================================
--- linux-2.6.14-git4.orig/kernel/power/power.h	2005-11-01 18:20:28.000000000 +0100
+++ linux-2.6.14-git4/kernel/power/power.h	2005-11-01 18:22:12.000000000 +0100
@@ -66,6 +66,7 @@
 extern asmlinkage int swsusp_arch_resume(void);
 
 extern int restore_highmem(void);
+extern void free_pagedir(struct pbe *pblist);
 extern struct pbe *alloc_pagedir(unsigned nr_pages, gfp_t gfp_mask, int safe_needed);
 extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
 extern void swsusp_free(void);
Index: linux-2.6.14-git4/kernel/power/snapshot.c
===================================================================
--- linux-2.6.14-git4.orig/kernel/power/snapshot.c	2005-11-01 18:20:28.000000000 +0100
+++ linux-2.6.14-git4/kernel/power/snapshot.c	2005-11-01 18:22:12.000000000 +0100
@@ -217,7 +217,7 @@
  *	free_pagedir - free pages allocated with alloc_pagedir()
  */
 
-static void free_pagedir(struct pbe *pblist)
+void free_pagedir(struct pbe *pblist)
 {
 	struct pbe *pbe;
 

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Monday, 31 of October 2005 22:59, Pavel Machek wrote:
}-- snip --{
> 
> > > > - sys_create_pagedir
> > > 
> > > Ugly...
> > 
> > Oh, it can be done on-the-fly in
> > sys_put_this_stuff_where_appropriate(image data) (at the expense of one
> > redundant check per call).
> 
> Yes, but it is still ugly, as you keep some context across the
> syscalls.

That depends on how you implement the interface.  If you insist on using
ioctls then yes, it's ugly.  However, if it is a file in sysfs, for example,
then you have well-defined open(), close(), read() and write() operations
and it is assumed you will keep some context accross eg. write()s.
 
> > > > Cleanup: /* certainly something's gone wrong */
> > > > - sys_destroy_pagedir /* that's it */
> > > > - sys_resume_devices
> > > 
> > > You should not need to do this one. resuming devices is going to be
> > > integrated in atomic_restore, because suspending devices is there, too.
> > 
> > Yes, but I need to thaw processes anyway, so I can release memory as well.
> > OTOH, if sys_atomic_restore fails because of the lack of memory, the memory
> > should be freed _before_ resuming devices, since otherwise subsequent
> > failures are almost certain to appear (I've seen what happens in that case).
> > Now, if the memory is allocated by the kernel, I can easily put an
> > emergency memory-freeing call in sys_atomic_restore (in that case
> > sys_destroy_pagedir will be redundant, but so what?).
> 
> Ugh, I'd say "don't care about this one too much". If resume is
> failing, we have bad problems anyway.

We loose the saved system state, but the kernel that has just booted is
supposed to continue, so we should make it possible.  Alternatively,
we can do something like a panic and force the user to reboot,
in which case we can forget about the error paths, freeing memory
etc. altogether.

Greetings,
Rafael

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> > > Oh, it can be done on-the-fly in
> > > sys_put_this_stuff_where_appropriate(image data) (at the expense of one
> > > redundant check per call).
> > 
> > Yes, but it is still ugly, as you keep some context across the
> > syscalls.
> 
> That depends on how you implement the interface.  If you insist on using
> ioctls then yes, it's ugly.  However, if it is a file in sysfs, for example,
> then you have well-defined open(), close(), read() and write() operations
> and it is assumed you will keep some context accross eg. write()s.

I was trying to keep kernel code simple. Yes, if we do it sysfs based,
that's probably not a problem. I'm not sure if nice sysfs interface
can be done without excessive ammount of code.
								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 1/2] swsusp: reduce code duplication (was: Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c)

[Pavel Machek]

Hi!

> > > > Speaking of simplifications and having seen your code I hope you will agree with
> > > > the appended patch against vanilla 2.6.14-git3 (it reduces the duplication of code,
> > > > and replaces swsusp_pagedir_relocate with a simpler mechanism).
> > > 
> > > ...and also moves stuff around in a way
> > > 
> > > a) I don't like
> > > 
> > > and
> > > 
> > > b) is almost impossible to review
> > 
> > OK, I'll try to split it into two patches to make it cleaner.
> 
> The first patch is appended, the next one will be in the reply to this message.
> 
> The changes made by the appended patch are necessary for the relocation
> simplification in the next patch.  Still, the changes allow us to drop
> check_pagedir() and make get_safe_page() be a one-line wrapper around
> alloc_image_page() (get_safe_page() goes to snapshot.c, because
> alloc_image_page() is static and it does not make sense to export
> it).

ACK.
								Pavel
-- 
Thanks, Sharp!

Re: [PATCH 2/2] swsusp: simplify pagedir relocation

[Pavel Machek]

> The appended patch simplifies the relocation of the page backup list
> (aka pagedir) during resume.

ACK and thanks for patience.

								Pavel

-- 
Thanks, Sharp!

Re: [PATCH 2/2] swsusp: simplify pagedir relocation

[Rafael J. Wysocki]

On Tuesday, 1 of November 2005 22:11, Pavel Machek wrote:
> 
> > The appended patch simplifies the relocation of the page backup list
> > (aka pagedir) during resume.
> 
> ACK and thanks for patience.

No big deal.

Greetings,
Rafael

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Rafael J. Wysocki]

Hi,

On Tuesday, 1 of November 2005 22:04, Pavel Machek wrote:
> Hi!
> 
> > > > Oh, it can be done on-the-fly in
> > > > sys_put_this_stuff_where_appropriate(image data) (at the expense of one
> > > > redundant check per call).
> > > 
> > > Yes, but it is still ugly, as you keep some context across the
> > > syscalls.
> > 
> > That depends on how you implement the interface.  If you insist on using
> > ioctls then yes, it's ugly.  However, if it is a file in sysfs, for example,
> > then you have well-defined open(), close(), read() and write() operations
> > and it is assumed you will keep some context accross eg. write()s.
> 
> I was trying to keep kernel code simple. Yes, if we do it sysfs based,
> that's probably not a problem. I'm not sure if nice sysfs interface
> can be done without excessive ammount of code.

I'm not sure either, but I'm going to try.

Still first I'd like to make swsusp free only as much memory as needed
and not as much as possible which should improve its performance
quite a bit.

Greetings,
Rafael

Re: [PATCH 2/3] swsusp: move snapshot-handling functions to snapshot.c

[Pavel Machek]

Hi!

> > > That depends on how you implement the interface.  If you insist on using
> > > ioctls then yes, it's ugly.  However, if it is a file in sysfs, for example,
> > > then you have well-defined open(), close(), read() and write() operations
> > > and it is assumed you will keep some context accross eg. write()s.
> > 
> > I was trying to keep kernel code simple. Yes, if we do it sysfs based,
> > that's probably not a problem. I'm not sure if nice sysfs interface
> > can be done without excessive ammount of code.
> 
> I'm not sure either, but I'm going to try.

Ok, I'm looking forward. We still have my ugly ioctls as a fallback :-).

> Still first I'd like to make swsusp free only as much memory as needed
> and not as much as possible which should improve its performance
> quite a bit.

Yes, I'd like that patch in. It will make many people happy.
								Pavel
-- 
Thanks, Sharp!