Re: [swsusp] separate snapshot functionality to separate file
From: Pavel Machek
Date: Tue Oct 04 2005 - 15:54:34 EST
Hi!
> > > should be moved to snapshot.c as well, because they are in fact
> > > symmetrical to what's there (they perform the reverse of creating
> > > the snapshot and use analogous data structures). IMO the code
> > > change required would not be so drammatic and all of the functions
> > > that _operate_ on the snapshot would be in the same file.
> >
> > No. read_suspend_image/read_pagedir/data_read is image reading.
>
> Yes, they are, but they use the same data structures and even call the same
> functions that are used in snapshot.c (eg. alloc_pagedir).
Yes, alloc_pagedir is shared, but that is pretty much it. Difference
between "this provides snapshot" and "this does disk reading/writing"
is still usefull.
> > That does not belong to snaphost. The rest is notthat clear, but I have it
> > working in userspace.
>
> Of course it is doable in the userland, but this does not mean it should be
> done in the userland. Personally I don't think so (please see
> below).
Even if you don't agree with putting it to userland (and that's
neccessary -- if we want some features from suspend2), split still
makes sense.
> > Image creation is still done in kernel space, but I think that kernel
> > <-> user interface is going to be cleaner that way, and I do not think
> > pushing it to user is so huge win.
> >
> > Yes, names are not ideal, but that will be followup patch.
>
> Having considered it for a while I think it's too early for the splitting,
> because:
> 1) we have bug fixes pending (viz. the x86-64 resume problem),
It is in arch-dependend code, only. It does not even conflict with split.
> 2) we can simplify swsusp quite a bit thanks to the rework-memory-freeing
> patch (eg. we can get rid of eat_page(), free_eaten_memory() and
> some complicated error paths in the resume code), which I'd prefer to do
> before the code is split,
Well, same cleanup can be done after the split, just as easily.
> 3) some cleanups are due before the splitting (eg. function names, the removal
> of prepare_suspend_image() etc.),
Split does not prevent you from doing the cleanups.
> 4) we could make swsusp consist of two functionally independent parts (ie. such
> that they use different data structures etc.) while it is in the single file
> and _then_ split.
The order of cleanups does not matter.
> IMHO there could be the snapshot-handling part and the storage-handling
> part interfacing via some functions (called by the snapshot-handling part)
> like:
No. It needs to be controlled by storage-handling parts, so that
snapshot-handling parts become nice library.
> 1) prepare_to_save(n, m) - initializing the data structures of the storage-handling
> part and the storage itself,
> 2) save_page(*addr, nr) - with addr being the addres of the page to save and nr
> the number of the page, where:
> - nr = 0 for the first pagedir page,
> - nr = (n - 1) for the last pagedir page,
> - nr = n for the first image page (ie. the page pointed to by the first pagedir
> entry)
> - nr = (n + m - 1) for the last image page (ie. the page pointed to by the
> last pagedir entry)
> 3) finish_saving() - removing the storage-handling part data structures,
> 4) prepare_to_load(*n, *m),
> 5) load_page(*addr, nr) - now addr being the address where to store the page,
> 6) finish_loading()
That is ugly. snapshot needs to be called from storage handling parts,
and then interface can become much simpler:
struct pbe *sys_snapshot();
snapshots a system, then you can save it in any way you want. And
void sys_restore(struct pbe *);
. Simple, eh?
> Then the storage-handling part would only have to save/load individual pages
> and associate the page numbers given by the snapshot-handling part
> with swap offsets or equivalent storage addresses. In that case the
> storage-handling could be moved (at leas partially) to the userland
> _without_ reimplementing the swsusp's data structures in there.
You can't easily do calls from kernel to userland.
> Also, we could use more memory-efficient representation of the PBE, as the
> only component of it that we really _need_ to save is the .orig_address field.
> Namely, the snapshot-handling part could use PBEs consisting of .address,
> .orig_address and .next internally, passing only the .orig_address fields
> to the storage-handling part (the original order of them could be recreated
> based on nr in save_page() and load_page()).
>
> Even if that sounds complicated, I think I can implement it in two weeks,
> so please give me a chance.
Lets do it different way. Give me two weeks to do my cleanups. The
initial code move is big, but things get pretty easy from then on. I
break nothing, your cleanups will still be possible. Preview of some
cleanups follows...
Cleanup comments, remove unneccessary includes.
---
commit 1d0ecfa8c8bf0176533abcd2b3a6fe5d6555a182
tree f5491a678e89b8992904d325bb1972d0f8b539fa
parent 0eb03cc17fa5bed7659835f30e5dac6e7c8f614b
author <pavel@amd.(none)> Mon, 03 Oct 2005 14:24:24 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 14:24:24 +0200
kernel/power/snapshot.c | 25 ++-----------------------
kernel/power/swsusp.c | 9 +--------
2 files changed, 3 insertions(+), 31 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -1,8 +1,7 @@
/*
- * linux/kernel/power/swsusp.c
+ * linux/kernel/power/snapshot.c
*
- * This file is to realize architecture-independent
- * machine suspend feature using pretty near only high-level routines
+ * This file provide system snapshot/restore functionality.
*
* Copyright (C) 1998-2005 Pavel Machek <pavel@xxxxxxx>
*
@@ -15,30 +14,16 @@
#include <linux/mm.h>
#include <linux/suspend.h>
#include <linux/smp_lock.h>
-#include <linux/file.h>
-#include <linux/utsname.h>
-#include <linux/version.h>
#include <linux/delay.h>
-#include <linux/reboot.h>
#include <linux/bitops.h>
-#include <linux/vt_kern.h>
-#include <linux/kbd_kern.h>
-#include <linux/keyboard.h>
#include <linux/spinlock.h>
-#include <linux/genhd.h>
#include <linux/kernel.h>
-#include <linux/major.h>
-#include <linux/swap.h>
#include <linux/pm.h>
#include <linux/device.h>
-#include <linux/buffer_head.h>
-#include <linux/swapops.h>
#include <linux/bootmem.h>
#include <linux/syscalls.h>
#include <linux/console.h>
#include <linux/highmem.h>
-#include <linux/bio.h>
-#include <linux/mount.h>
#include <asm/uaccess.h>
#include <asm/mmu_context.h>
@@ -46,15 +31,9 @@
#include <asm/tlbflush.h>
#include <asm/io.h>
-#include <linux/random.h>
-#include <linux/crypto.h>
-#include <asm/scatterlist.h>
-
#include "power.h"
-
-
#ifdef CONFIG_HIGHMEM
struct highmem_page {
char *data;
diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c
+++ b/kernel/power/swsusp.c
@@ -1,8 +1,7 @@
/*
* linux/kernel/power/swsusp.c
*
- * This file is to realize architecture-independent
- * machine suspend feature using pretty near only high-level routines
+ * This file provides code to write suspend image to swap and read it back.
*
* Copyright (C) 1998-2001 Gabor Kuti <seasons@xxxxxxxxx>
* Copyright (C) 1998,2001-2005 Pavel Machek <pavel@xxxxxxx>
@@ -47,11 +46,7 @@
#include <linux/utsname.h>
#include <linux/version.h>
#include <linux/delay.h>
-#include <linux/reboot.h>
#include <linux/bitops.h>
-#include <linux/vt_kern.h>
-#include <linux/kbd_kern.h>
-#include <linux/keyboard.h>
#include <linux/spinlock.h>
#include <linux/genhd.h>
#include <linux/kernel.h>
@@ -63,10 +58,8 @@
#include <linux/swapops.h>
#include <linux/bootmem.h>
#include <linux/syscalls.h>
-#include <linux/console.h>
#include <linux/highmem.h>
#include <linux/bio.h>
-#include <linux/mount.h>
#include <asm/uaccess.h>
#include <asm/mmu_context.h>
!-------------------------------------------------------------flip-
Coding style cleanups.
---
commit 4bf05b99d3c082bd0a1b5106f0b26daf25a0ead6
tree 314cf06719e7fb4eb03e153dbe2d785de250665e
parent 1d0ecfa8c8bf0176533abcd2b3a6fe5d6555a182
author <pavel@amd.(none)> Mon, 03 Oct 2005 14:34:57 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 14:34:57 +0200
kernel/power/snapshot.c | 14 +++++++-------
1 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -144,10 +144,10 @@ static int pfn_is_nosave(unsigned long p
* isn't part of a free chunk of pages.
*/
-static int saveable(struct zone * zone, unsigned long * zone_pfn)
+static int saveable(struct zone *zone, unsigned long *zone_pfn)
{
unsigned long pfn = *zone_pfn + zone->zone_start_pfn;
- struct page * page;
+ struct page *page;
if (!pfn_valid(pfn))
return 0;
@@ -196,11 +196,11 @@ static void copy_data_pages(void)
/* This is necessary for swsusp_free() */
for_each_pb_page (p, pagedir_nosave)
SetPageNosaveFree(virt_to_page(p));
- for_each_pbe(p, pagedir_nosave)
+ for_each_pbe (p, pagedir_nosave)
SetPageNosaveFree(virt_to_page(p->address));
for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
if (saveable(zone, &zone_pfn)) {
- struct page * page;
+ struct page *page;
page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
BUG_ON(!pbe);
pbe->orig_address = (unsigned long)page_address(page);
@@ -294,7 +294,7 @@ static void *alloc_image_page(void)
* 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)
{
unsigned num;
struct pbe *pblist, *pbe;
@@ -303,7 +303,7 @@ struct pbe * alloc_pagedir(unsigned nr_p
return NULL;
pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
- pblist = (struct pbe *)alloc_image_page();
+ pblist = alloc_image_page();
/* FIXME: rewrite this ugly loop */
for (pbe = pblist, num = PBES_PER_PAGE; pbe && num < nr_pages;
pbe = pbe->next, num += PBES_PER_PAGE) {
@@ -359,7 +359,7 @@ static int enough_free_mem(void)
static int swsusp_alloc(void)
{
- struct pbe * p;
+ struct pbe *p;
pagedir_nosave = NULL;
!-------------------------------------------------------------flip-
One more unneccessary cast killed.
---
commit 8e13f07afcbc380088716ae8b2592d25602cd825
tree e2935bf896b939beb5c336cc79414a5a70793318
parent 4bf05b99d3c082bd0a1b5106f0b26daf25a0ead6
author <pavel@amd.(none)> Mon, 03 Oct 2005 14:36:03 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 14:36:03 +0200
kernel/power/snapshot.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -308,7 +308,7 @@ struct pbe *alloc_pagedir(unsigned nr_pa
for (pbe = pblist, num = PBES_PER_PAGE; pbe && num < nr_pages;
pbe = pbe->next, num += PBES_PER_PAGE) {
pbe += PB_PAGE_SKIP;
- pbe->next = (struct pbe *)alloc_image_page();
+ pbe->next = alloc_image_page();
}
if (!pbe) { /* get_zeroed_page() failed */
free_pagedir(pblist);
!-------------------------------------------------------------flip-
Reduce number of ifdefs somehow.
---
commit 84fc971b826d58ee677b119cf3b6d40bc7617728
tree cc22c57a14c587f4ef4f0046f5f5a46524247da9
parent 8e13f07afcbc380088716ae8b2592d25602cd825
author <pavel@amd.(none)> Mon, 03 Oct 2005 14:54:40 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 14:54:40 +0200
kernel/power/snapshot.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -33,7 +33,6 @@
#include "power.h"
-
#ifdef CONFIG_HIGHMEM
struct highmem_page {
char *data;
@@ -88,12 +87,10 @@ static int save_highmem_zone(struct zone
}
return 0;
}
-#endif /* CONFIG_HIGHMEM */
static int save_highmem(void)
{
-#ifdef CONFIG_HIGHMEM
struct zone *zone;
int res = 0;
@@ -104,13 +101,11 @@ static int save_highmem(void)
if (res)
return res;
}
-#endif
return 0;
}
int restore_highmem(void)
{
-#ifdef CONFIG_HIGHMEM
printk("swsusp: Restoring Highmem\n");
while (highmem_copy) {
struct highmem_page *save = highmem_copy;
@@ -123,9 +118,12 @@ int restore_highmem(void)
free_page((long) save->data);
kfree(save);
}
-#endif
return 0;
}
+#else
+static int save_highmem(void) { return 0; }
+int restore_highmem(void) { return 0; }
+#endif /* CONFIG_HIGHMEM */
static int pfn_is_nosave(unsigned long pfn)
!-------------------------------------------------------------flip-
Reduce use of global variables.
---
commit 7797678810ae12ac92dfecf231ea0f1e27485dfd
tree b4a5834b90c3c9667dfa2d5eb19478d6a6a16405
parent 84fc971b826d58ee677b119cf3b6d40bc7617728
author <pavel@amd.(none)> Mon, 03 Oct 2005 14:59:10 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 14:59:10 +0200
kernel/power/snapshot.c | 23 +++++++++++------------
1 files changed, 11 insertions(+), 12 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -88,7 +88,6 @@ static int save_highmem_zone(struct zone
return 0;
}
-
static int save_highmem(void)
{
struct zone *zone;
@@ -164,37 +163,36 @@ static int saveable(struct zone *zone, u
return 1;
}
-static void count_data_pages(void)
+static int count_data_pages(void)
{
struct zone *zone;
unsigned long zone_pfn;
-
- nr_copy_pages = 0;
+ int pages = 0;
for_each_zone (zone) {
if (is_highmem(zone))
continue;
mark_free_pages(zone);
for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
- nr_copy_pages += saveable(zone, &zone_pfn);
+ pages += saveable(zone, &zone_pfn);
}
+ return pages;
}
-static void copy_data_pages(void)
+static void copy_data_pages(struct pbe *pgdir)
{
struct zone *zone;
unsigned long zone_pfn;
- struct pbe *pbe = pagedir_nosave, *p;
+ struct pbe *pbe = pgdir, *p;
- pr_debug("copy_data_pages(): pages to copy: %d\n", nr_copy_pages);
for_each_zone (zone) {
if (is_highmem(zone))
continue;
mark_free_pages(zone);
/* This is necessary for swsusp_free() */
- for_each_pb_page (p, pagedir_nosave)
+ for_each_pb_page (p, pgdir)
SetPageNosaveFree(virt_to_page(p));
- for_each_pbe (p, pagedir_nosave)
+ for_each_pbe (p, pgdir)
SetPageNosaveFree(virt_to_page(p->address));
for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
if (saveable(zone, &zone_pfn)) {
@@ -396,7 +394,7 @@ static int suspend_prepare_image(void)
}
drain_local_pages();
- count_data_pages();
+ nr_copy_pages = count_data_pages();
printk("swsusp: Need to copy %u pages\n", nr_copy_pages);
pr_debug("swsusp: pages needed: %u + %u + %u, free: %u\n",
@@ -422,7 +420,8 @@ static int suspend_prepare_image(void)
* Kill them.
*/
drain_local_pages();
- copy_data_pages();
+ pr_debug("copy_data_pages(): pages to copy: %d\n", nr_copy_pages);
+ copy_data_pages(pagedir_nosave);
/*
* End of critical section. From now on, we can write to memory,
!-------------------------------------------------------------flip-
More globals removal.
---
commit e1754380014e7306a20eefbc924b9621280d73f5
tree d7f499d6a3911d784706f0c9488b7cbb3bc67f8d
parent 7797678810ae12ac92dfecf231ea0f1e27485dfd
author <pavel@amd.(none)> Mon, 03 Oct 2005 23:48:57 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 23:48:57 +0200
kernel/power/snapshot.c | 25 +++++++++++--------------
1 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
--- a/kernel/power/snapshot.c
+++ b/kernel/power/snapshot.c
@@ -353,24 +353,21 @@ static int enough_free_mem(void)
}
-static int swsusp_alloc(void)
+static struct pbe *swsusp_alloc(int pages)
{
- struct pbe *p;
+ struct pbe *p, *pgdir;
- pagedir_nosave = NULL;
-
- if (MAX_PBES < nr_copy_pages / PBES_PER_PAGE +
- !!(nr_copy_pages % PBES_PER_PAGE))
+ if (MAX_PBES < pages / PBES_PER_PAGE +
+ !!(pages % PBES_PER_PAGE))
return -ENOSPC;
- if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
+ if (!(pgdir = alloc_pagedir(pages))) {
printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
return -ENOMEM;
}
- create_pbe_list(pagedir_save, nr_copy_pages);
- pagedir_nosave = pagedir_save;
+ create_pbe_list(pgdir, pages);
- for_each_pbe (p, pagedir_save) {
+ for_each_pbe (p, pgdir) {
p->address = (unsigned long)alloc_image_page();
if (!p->address) {
printk(KERN_ERR "suspend: Allocating image pages failed.\n");
@@ -379,7 +376,7 @@ static int swsusp_alloc(void)
}
}
- return 0;
+ return pgdir;
}
static int suspend_prepare_image(void)
@@ -412,9 +409,9 @@ static int suspend_prepare_image(void)
return -ENOSPC;
}
- error = swsusp_alloc();
- if (error)
- return error;
+ pagedir_save = pagedir_nosave = swsusp_alloc(nr_copy_pages);
+ if (!pagedir_save)
+ return -ENOSPC;
/* During allocating of suspend pagedir, new cold pages may appear.
* Kill them.
--
if you have sharp zaurus hardware you don't need... you know my address
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/