Split swsusp.c into swsusp.c and snapshot.c. Snapshot only cares
provides system snapshot/restore functionality, while swsusp.c will
provide disk i/o. It should enable untangling of the code in future;
swsusp.c parts can mostly be done in userspace.
No code changes.
Signed-off-by: Pavel Machek <[email protected]>
---
commit 747dfd4b9b6abd5df42d6a003cf4ab5dc36a4026
tree 6e6c3ed712122d1aafb6cadcc21fa077aed0fc82
parent f3dae2d929b7f33da6d9f8a70b9a80b705ed20c7
author <pavel@amd.(none)> Mon, 03 Oct 2005 00:45:49 +0200
committer <pavel@amd.(none)> Mon, 03 Oct 2005 00:45:49 +0200
include/linux/suspend.h | 6 +
kernel/power/Makefile | 2
kernel/power/power.h | 17 ++
kernel/power/snapshot.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++
kernel/power/swsusp.c | 432 --------------------------------------------
5 files changed, 492 insertions(+), 429 deletions(-)
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -72,4 +72,10 @@ struct saved_context;
void __save_processor_state(struct saved_context *ctxt);
void __restore_processor_state(struct saved_context *ctxt);
+/*
+ * XXX: We try to keep some more pages free so that I/O operations succeed
+ * without paging. Might this be more?
+ */
+#define PAGES_FOR_IO 512
+
#endif /* _LINUX_SWSUSP_H */
diff --git a/kernel/power/Makefile b/kernel/power/Makefile
--- a/kernel/power/Makefile
+++ b/kernel/power/Makefile
@@ -4,7 +4,7 @@ EXTRA_CFLAGS += -DDEBUG
endif
obj-y := main.o process.o console.o pm.o
-obj-$(CONFIG_SOFTWARE_SUSPEND) += swsusp.o disk.o
+obj-$(CONFIG_SOFTWARE_SUSPEND) += swsusp.o disk.o snapshot.o
obj-$(CONFIG_SUSPEND_SMP) += smp.o
diff --git a/kernel/power/power.h b/kernel/power/power.h
--- a/kernel/power/power.h
+++ b/kernel/power/power.h
@@ -53,3 +53,20 @@ extern void thaw_processes(void);
extern int pm_prepare_console(void);
extern void pm_restore_console(void);
+
+
+/* References to section boundaries */
+extern const void __nosave_begin, __nosave_end;
+
+extern unsigned int nr_copy_pages;
+extern suspend_pagedir_t *pagedir_nosave;
+extern suspend_pagedir_t *pagedir_save;
+
+extern asmlinkage int swsusp_arch_suspend(void);
+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);
+extern void create_pbe_list(struct pbe *pblist, unsigned nr_pages);
+extern int enough_swap(void);
diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c
new file mode 100644
--- /dev/null
+++ b/kernel/power/snapshot.c
@@ -0,0 +1,464 @@
+/*
+ * linux/kernel/power/swsusp.c
+ *
+ * This file is to realize architecture-independent
+ * machine suspend feature using pretty near only high-level routines
+ *
+ * Copyright (C) 1998-2005 Pavel Machek <[email protected]>
+ *
+ * This file is released under the GPLv2, and is based on swsusp.c.
+ *
+ */
+
+
+#include <linux/module.h>
+#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>
+#include <asm/pgtable.h>
+#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;
+ struct page *page;
+ struct highmem_page *next;
+};
+
+static struct highmem_page *highmem_copy;
+
+static int save_highmem_zone(struct zone *zone)
+{
+ unsigned long zone_pfn;
+ mark_free_pages(zone);
+ for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
+ struct page *page;
+ struct highmem_page *save;
+ void *kaddr;
+ unsigned long pfn = zone_pfn + zone->zone_start_pfn;
+
+ if (!(pfn%1000))
+ printk(".");
+ if (!pfn_valid(pfn))
+ continue;
+ page = pfn_to_page(pfn);
+ /*
+ * This condition results from rvmalloc() sans vmalloc_32()
+ * and architectural memory reservations. This should be
+ * corrected eventually when the cases giving rise to this
+ * are better understood.
+ */
+ if (PageReserved(page)) {
+ printk("highmem reserved page?!\n");
+ continue;
+ }
+ BUG_ON(PageNosave(page));
+ if (PageNosaveFree(page))
+ continue;
+ save = kmalloc(sizeof(struct highmem_page), GFP_ATOMIC);
+ if (!save)
+ return -ENOMEM;
+ save->next = highmem_copy;
+ save->page = page;
+ save->data = (void *) get_zeroed_page(GFP_ATOMIC);
+ if (!save->data) {
+ kfree(save);
+ return -ENOMEM;
+ }
+ kaddr = kmap_atomic(page, KM_USER0);
+ memcpy(save->data, kaddr, PAGE_SIZE);
+ kunmap_atomic(kaddr, KM_USER0);
+ highmem_copy = save;
+ }
+ return 0;
+}
+#endif /* CONFIG_HIGHMEM */
+
+
+static int save_highmem(void)
+{
+#ifdef CONFIG_HIGHMEM
+ struct zone *zone;
+ int res = 0;
+
+ pr_debug("swsusp: Saving Highmem\n");
+ for_each_zone (zone) {
+ if (is_highmem(zone))
+ res = save_highmem_zone(zone);
+ 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;
+ void *kaddr;
+ highmem_copy = save->next;
+
+ kaddr = kmap_atomic(save->page, KM_USER0);
+ memcpy(kaddr, save->data, PAGE_SIZE);
+ kunmap_atomic(kaddr, KM_USER0);
+ free_page((long) save->data);
+ kfree(save);
+ }
+#endif
+ return 0;
+}
+
+
+static int pfn_is_nosave(unsigned long pfn)
+{
+ unsigned long nosave_begin_pfn = __pa(&__nosave_begin) >> PAGE_SHIFT;
+ unsigned long nosave_end_pfn = PAGE_ALIGN(__pa(&__nosave_end)) >> PAGE_SHIFT;
+ return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
+}
+
+/**
+ * saveable - Determine whether a page should be cloned or not.
+ * @pfn: The page
+ *
+ * We save a page if it's Reserved, and not in the range of pages
+ * statically defined as 'unsaveable', or if it isn't reserved, and
+ * isn't part of a free chunk of pages.
+ */
+
+static int saveable(struct zone * zone, unsigned long * zone_pfn)
+{
+ unsigned long pfn = *zone_pfn + zone->zone_start_pfn;
+ struct page * page;
+
+ if (!pfn_valid(pfn))
+ return 0;
+
+ page = pfn_to_page(pfn);
+ BUG_ON(PageReserved(page) && PageNosave(page));
+ if (PageNosave(page))
+ return 0;
+ if (PageReserved(page) && pfn_is_nosave(pfn)) {
+ pr_debug("[nosave pfn 0x%lx]", pfn);
+ return 0;
+ }
+ if (PageNosaveFree(page))
+ return 0;
+
+ return 1;
+}
+
+static void count_data_pages(void)
+{
+ struct zone *zone;
+ unsigned long zone_pfn;
+
+ nr_copy_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);
+ }
+}
+
+static void copy_data_pages(void)
+{
+ struct zone *zone;
+ unsigned long zone_pfn;
+ struct pbe *pbe = pagedir_nosave, *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)
+ SetPageNosaveFree(virt_to_page(p));
+ 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;
+ page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
+ BUG_ON(!pbe);
+ pbe->orig_address = (unsigned long)page_address(page);
+ /* copy_page is not usable for copying task structs. */
+ memcpy((void *)pbe->address, (void *)pbe->orig_address, PAGE_SIZE);
+ pbe = pbe->next;
+ }
+ }
+ }
+ BUG_ON(pbe);
+}
+
+
+/**
+ * free_pagedir - free pages allocated with alloc_pagedir()
+ */
+
+void free_pagedir(struct pbe *pblist)
+{
+ struct pbe *pbe;
+
+ while (pblist) {
+ pbe = (pblist + PB_PAGE_SKIP)->next;
+ ClearPageNosave(virt_to_page(pblist));
+ ClearPageNosaveFree(virt_to_page(pblist));
+ free_page((unsigned long)pblist);
+ pblist = pbe;
+ }
+}
+
+/**
+ * fill_pb_page - Create a list of PBEs on a given memory page
+ */
+
+static inline void fill_pb_page(struct pbe *pbpage)
+{
+ struct pbe *p;
+
+ p = pbpage;
+ pbpage += PB_PAGE_SKIP;
+ do
+ p->next = p + 1;
+ while (++p < pbpage);
+}
+
+/**
+ * create_pbe_list - Create a list of PBEs on top of a given chain
+ * of memory pages allocated with alloc_pagedir()
+ */
+
+void create_pbe_list(struct pbe *pblist, unsigned nr_pages)
+{
+ struct pbe *pbpage, *p;
+ unsigned num = PBES_PER_PAGE;
+
+ for_each_pb_page (pbpage, pblist) {
+ if (num >= nr_pages)
+ break;
+
+ fill_pb_page(pbpage);
+ num += PBES_PER_PAGE;
+ }
+ if (pbpage) {
+ for (num -= PBES_PER_PAGE - 1, p = pbpage; num < nr_pages; p++, num++)
+ p->next = p + 1;
+ p->next = NULL;
+ }
+ pr_debug("create_pbe_list(): initialized %d PBEs\n", num);
+}
+
+static void *alloc_image_page(void)
+{
+ void *res = (void *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
+ if (res) {
+ SetPageNosave(virt_to_page(res));
+ SetPageNosaveFree(virt_to_page(res));
+ }
+ return res;
+}
+
+/**
+ * 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.
+ *
+ * On each page we set up a list of struct_pbe elements.
+ */
+
+struct pbe * alloc_pagedir(unsigned nr_pages)
+{
+ unsigned num;
+ struct pbe *pblist, *pbe;
+
+ if (!nr_pages)
+ return NULL;
+
+ pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
+ pblist = (struct pbe *)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) {
+ pbe += PB_PAGE_SKIP;
+ pbe->next = (struct pbe *)alloc_image_page();
+ }
+ if (!pbe) { /* get_zeroed_page() failed */
+ free_pagedir(pblist);
+ pblist = NULL;
+ }
+ return pblist;
+}
+
+/**
+ * Free pages we allocated for suspend. Suspend pages are alocated
+ * before atomic copy, so we need to free them after resume.
+ */
+
+void swsusp_free(void)
+{
+ struct zone *zone;
+ unsigned long zone_pfn;
+
+ for_each_zone(zone) {
+ for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
+ if (pfn_valid(zone_pfn + zone->zone_start_pfn)) {
+ struct page * page;
+ page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
+ if (PageNosave(page) && PageNosaveFree(page)) {
+ ClearPageNosave(page);
+ ClearPageNosaveFree(page);
+ free_page((long) page_address(page));
+ }
+ }
+ }
+}
+
+
+/**
+ * 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)
+{
+ pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
+ return nr_free_pages() > (nr_copy_pages + PAGES_FOR_IO +
+ nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE));
+}
+
+
+static int swsusp_alloc(void)
+{
+ struct pbe * p;
+
+ pagedir_nosave = NULL;
+
+ if (MAX_PBES < nr_copy_pages / PBES_PER_PAGE +
+ !!(nr_copy_pages % PBES_PER_PAGE))
+ return -ENOSPC;
+
+ if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
+ printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
+ return -ENOMEM;
+ }
+ create_pbe_list(pagedir_save, nr_copy_pages);
+ pagedir_nosave = pagedir_save;
+
+ for_each_pbe (p, pagedir_save) {
+ p->address = (unsigned long)alloc_image_page();
+ if (!p->address) {
+ printk(KERN_ERR "suspend: Allocating image pages failed.\n");
+ swsusp_free();
+ return -ENOMEM;
+ }
+ }
+
+ return 0;
+}
+
+static int suspend_prepare_image(void)
+{
+ int error;
+
+ 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();
+ 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",
+ nr_copy_pages,
+ nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE),
+ PAGES_FOR_IO, nr_free_pages());
+
+ if (!enough_free_mem()) {
+ printk(KERN_ERR "swsusp: Not enough free memory\n");
+ return -ENOMEM;
+ }
+
+ if (!enough_swap()) {
+ printk(KERN_ERR "swsusp: Not enough free swap\n");
+ return -ENOSPC;
+ }
+
+ error = swsusp_alloc();
+ if (error)
+ return error;
+
+ /* During allocating of suspend pagedir, new cold pages may appear.
+ * Kill them.
+ */
+ drain_local_pages();
+ copy_data_pages();
+
+ /*
+ * End of critical section. From now on, we can write to memory,
+ * but we should not touch disk. This specially means we must _not_
+ * touch swap space! Except we must write out our image of course.
+ */
+
+ printk("swsusp: critical section/: done (%d pages copied)\n", nr_copy_pages );
+ return 0;
+}
+
+
+asmlinkage int swsusp_save(void)
+{
+ return suspend_prepare_image();
+}
diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c
+++ b/kernel/power/swsusp.c
@@ -5,7 +5,7 @@
* machine suspend feature using pretty near only high-level routines
*
* Copyright (C) 1998-2001 Gabor Kuti <[email protected]>
- * Copyright (C) 1998,2001-2004 Pavel Machek <[email protected]>
+ * Copyright (C) 1998,2001-2005 Pavel Machek <[email protected]>
*
* This file is released under the GPLv2.
*
@@ -84,16 +84,10 @@
#define MAXKEY 32
#define MAXIV 32
-/* References to section boundaries */
-extern const void __nosave_begin, __nosave_end;
-
-/* Variables to be preserved over suspend */
-static int nr_copy_pages_check;
-
extern char resume_file[];
/* Local variables that should not be affected by save */
-static unsigned int nr_copy_pages __nosavedata = 0;
+unsigned int nr_copy_pages __nosavedata = 0;
/* Suspend pagedir is allocated before final copy, therefore it
must be freed after resume
@@ -109,7 +103,7 @@ static unsigned int nr_copy_pages __nosa
MMU hardware.
*/
suspend_pagedir_t *pagedir_nosave __nosavedata = NULL;
-static suspend_pagedir_t *pagedir_save;
+suspend_pagedir_t *pagedir_save;
#define SWSUSP_SIG "S1SUSPEND"
@@ -124,12 +118,6 @@ static struct swsusp_header {
static struct swsusp_info swsusp_info;
/*
- * XXX: We try to keep some more pages free so that I/O operations succeed
- * without paging. Might this be more?
- */
-#define PAGES_FOR_IO 512
-
-/*
* Saving part...
*/
@@ -553,328 +541,6 @@ static int write_suspend_image(void)
}
-#ifdef CONFIG_HIGHMEM
-struct highmem_page {
- char *data;
- struct page *page;
- struct highmem_page *next;
-};
-
-static struct highmem_page *highmem_copy;
-
-static int save_highmem_zone(struct zone *zone)
-{
- unsigned long zone_pfn;
- mark_free_pages(zone);
- for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn) {
- struct page *page;
- struct highmem_page *save;
- void *kaddr;
- unsigned long pfn = zone_pfn + zone->zone_start_pfn;
-
- if (!(pfn%1000))
- printk(".");
- if (!pfn_valid(pfn))
- continue;
- page = pfn_to_page(pfn);
- /*
- * This condition results from rvmalloc() sans vmalloc_32()
- * and architectural memory reservations. This should be
- * corrected eventually when the cases giving rise to this
- * are better understood.
- */
- if (PageReserved(page)) {
- printk("highmem reserved page?!\n");
- continue;
- }
- BUG_ON(PageNosave(page));
- if (PageNosaveFree(page))
- continue;
- save = kmalloc(sizeof(struct highmem_page), GFP_ATOMIC);
- if (!save)
- return -ENOMEM;
- save->next = highmem_copy;
- save->page = page;
- save->data = (void *) get_zeroed_page(GFP_ATOMIC);
- if (!save->data) {
- kfree(save);
- return -ENOMEM;
- }
- kaddr = kmap_atomic(page, KM_USER0);
- memcpy(save->data, kaddr, PAGE_SIZE);
- kunmap_atomic(kaddr, KM_USER0);
- highmem_copy = save;
- }
- return 0;
-}
-#endif /* CONFIG_HIGHMEM */
-
-
-static int save_highmem(void)
-{
-#ifdef CONFIG_HIGHMEM
- struct zone *zone;
- int res = 0;
-
- pr_debug("swsusp: Saving Highmem\n");
- for_each_zone (zone) {
- if (is_highmem(zone))
- res = save_highmem_zone(zone);
- if (res)
- return res;
- }
-#endif
- return 0;
-}
-
-static int restore_highmem(void)
-{
-#ifdef CONFIG_HIGHMEM
- printk("swsusp: Restoring Highmem\n");
- while (highmem_copy) {
- struct highmem_page *save = highmem_copy;
- void *kaddr;
- highmem_copy = save->next;
-
- kaddr = kmap_atomic(save->page, KM_USER0);
- memcpy(kaddr, save->data, PAGE_SIZE);
- kunmap_atomic(kaddr, KM_USER0);
- free_page((long) save->data);
- kfree(save);
- }
-#endif
- return 0;
-}
-
-
-static int pfn_is_nosave(unsigned long pfn)
-{
- unsigned long nosave_begin_pfn = __pa(&__nosave_begin) >> PAGE_SHIFT;
- unsigned long nosave_end_pfn = PAGE_ALIGN(__pa(&__nosave_end)) >> PAGE_SHIFT;
- return (pfn >= nosave_begin_pfn) && (pfn < nosave_end_pfn);
-}
-
-/**
- * saveable - Determine whether a page should be cloned or not.
- * @pfn: The page
- *
- * We save a page if it's Reserved, and not in the range of pages
- * statically defined as 'unsaveable', or if it isn't reserved, and
- * isn't part of a free chunk of pages.
- */
-
-static int saveable(struct zone * zone, unsigned long * zone_pfn)
-{
- unsigned long pfn = *zone_pfn + zone->zone_start_pfn;
- struct page * page;
-
- if (!pfn_valid(pfn))
- return 0;
-
- page = pfn_to_page(pfn);
- BUG_ON(PageReserved(page) && PageNosave(page));
- if (PageNosave(page))
- return 0;
- if (PageReserved(page) && pfn_is_nosave(pfn)) {
- pr_debug("[nosave pfn 0x%lx]", pfn);
- return 0;
- }
- if (PageNosaveFree(page))
- return 0;
-
- return 1;
-}
-
-static void count_data_pages(void)
-{
- struct zone *zone;
- unsigned long zone_pfn;
-
- nr_copy_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);
- }
-}
-
-static void copy_data_pages(void)
-{
- struct zone *zone;
- unsigned long zone_pfn;
- struct pbe *pbe = pagedir_nosave, *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)
- SetPageNosaveFree(virt_to_page(p));
- 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;
- page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
- BUG_ON(!pbe);
- pbe->orig_address = (unsigned long)page_address(page);
- /* copy_page is not usable for copying task structs. */
- memcpy((void *)pbe->address, (void *)pbe->orig_address, PAGE_SIZE);
- pbe = pbe->next;
- }
- }
- }
- BUG_ON(pbe);
-}
-
-
-/**
- * free_pagedir - free pages allocated with alloc_pagedir()
- */
-
-static inline void free_pagedir(struct pbe *pblist)
-{
- struct pbe *pbe;
-
- while (pblist) {
- pbe = (pblist + PB_PAGE_SKIP)->next;
- ClearPageNosave(virt_to_page(pblist));
- ClearPageNosaveFree(virt_to_page(pblist));
- free_page((unsigned long)pblist);
- pblist = pbe;
- }
-}
-
-/**
- * fill_pb_page - Create a list of PBEs on a given memory page
- */
-
-static inline void fill_pb_page(struct pbe *pbpage)
-{
- struct pbe *p;
-
- p = pbpage;
- pbpage += PB_PAGE_SKIP;
- do
- p->next = p + 1;
- while (++p < pbpage);
-}
-
-/**
- * create_pbe_list - Create a list of PBEs on top of a given chain
- * of memory pages allocated with alloc_pagedir()
- */
-
-static void create_pbe_list(struct pbe *pblist, unsigned nr_pages)
-{
- struct pbe *pbpage, *p;
- unsigned num = PBES_PER_PAGE;
-
- for_each_pb_page (pbpage, pblist) {
- if (num >= nr_pages)
- break;
-
- fill_pb_page(pbpage);
- num += PBES_PER_PAGE;
- }
- if (pbpage) {
- for (num -= PBES_PER_PAGE - 1, p = pbpage; num < nr_pages; p++, num++)
- p->next = p + 1;
- p->next = NULL;
- }
- pr_debug("create_pbe_list(): initialized %d PBEs\n", num);
-}
-
-static void *alloc_image_page(void)
-{
- void *res = (void *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
- if (res) {
- SetPageNosave(virt_to_page(res));
- SetPageNosaveFree(virt_to_page(res));
- }
- return res;
-}
-
-/**
- * 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.
- *
- * On each page we set up a list of struct_pbe elements.
- */
-
-static struct pbe * alloc_pagedir(unsigned nr_pages)
-{
- unsigned num;
- struct pbe *pblist, *pbe;
-
- if (!nr_pages)
- return NULL;
-
- pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
- pblist = (struct pbe *)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) {
- pbe += PB_PAGE_SKIP;
- pbe->next = (struct pbe *)alloc_image_page();
- }
- if (!pbe) { /* get_zeroed_page() failed */
- free_pagedir(pblist);
- pblist = NULL;
- }
- return pblist;
-}
-
-/**
- * Free pages we allocated for suspend. Suspend pages are alocated
- * before atomic copy, so we need to free them after resume.
- */
-
-void swsusp_free(void)
-{
- struct zone *zone;
- unsigned long zone_pfn;
-
- for_each_zone(zone) {
- for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_pfn)
- if (pfn_valid(zone_pfn + zone->zone_start_pfn)) {
- struct page * page;
- page = pfn_to_page(zone_pfn + zone->zone_start_pfn);
- if (PageNosave(page) && PageNosaveFree(page)) {
- ClearPageNosave(page);
- ClearPageNosaveFree(page);
- free_page((long) page_address(page));
- }
- }
- }
-}
-
-/**
- * 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)
-{
- pr_debug("swsusp: available memory: %u pages\n", nr_free_pages());
- return nr_free_pages() > (nr_copy_pages + PAGES_FOR_IO +
- nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE));
-}
-
-
/**
* enough_swap - Make sure we have enough swap to save the image.
*
@@ -885,7 +551,7 @@ static int enough_free_mem(void)
* We should only consider resume_device.
*/
-static int enough_swap(void)
+int enough_swap(void)
{
struct sysinfo i;
@@ -895,87 +561,6 @@ static int enough_swap(void)
nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE));
}
-static int swsusp_alloc(void)
-{
- struct pbe * p;
-
- pagedir_nosave = NULL;
-
- if (MAX_PBES < nr_copy_pages / PBES_PER_PAGE +
- !!(nr_copy_pages % PBES_PER_PAGE))
- return -ENOSPC;
-
- if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
- printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
- return -ENOMEM;
- }
- create_pbe_list(pagedir_save, nr_copy_pages);
- pagedir_nosave = pagedir_save;
-
- for_each_pbe (p, pagedir_save) {
- p->address = (unsigned long)alloc_image_page();
- if (!p->address) {
- printk(KERN_ERR "suspend: Allocating image pages failed.\n");
- swsusp_free();
- return -ENOMEM;
- }
- }
-
- return 0;
-}
-
-static int suspend_prepare_image(void)
-{
- int error;
-
- 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();
- count_data_pages();
- printk("swsusp: Need to copy %u pages\n", nr_copy_pages);
- nr_copy_pages_check = nr_copy_pages;
-
- pr_debug("swsusp: pages needed: %u + %u + %u, free: %u\n",
- nr_copy_pages,
- nr_copy_pages/PBES_PER_PAGE + !!(nr_copy_pages%PBES_PER_PAGE),
- PAGES_FOR_IO, nr_free_pages());
-
- if (!enough_free_mem()) {
- printk(KERN_ERR "swsusp: Not enough free memory\n");
- return -ENOMEM;
- }
-
- if (!enough_swap()) {
- printk(KERN_ERR "swsusp: Not enough free swap\n");
- return -ENOSPC;
- }
-
- error = swsusp_alloc();
- if (error)
- return error;
-
- /* During allocating of suspend pagedir, new cold pages may appear.
- * Kill them.
- */
- drain_local_pages();
- copy_data_pages();
-
- /*
- * End of critical section. From now on, we can write to memory,
- * but we should not touch disk. This specially means we must _not_
- * touch swap space! Except we must write out our image of course.
- */
-
- printk("swsusp: critical section/: done (%d pages copied)\n", nr_copy_pages );
- return 0;
-}
-
-
/* 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
@@ -994,14 +579,6 @@ int swsusp_write(void)
}
-extern asmlinkage int swsusp_arch_suspend(void);
-extern asmlinkage int swsusp_arch_resume(void);
-
-
-asmlinkage int swsusp_save(void)
-{
- return suspend_prepare_image();
-}
int swsusp_suspend(void)
{
@@ -1033,7 +610,6 @@ int swsusp_suspend(void)
printk(KERN_ERR "Error %d suspending\n", error);
/* Restore control flow magically appears here */
restore_processor_state();
- BUG_ON (nr_copy_pages_check != nr_copy_pages);
restore_highmem();
device_power_up();
local_irq_enable();
--
if you have sharp zaurus hardware you don't need... you know my address
Hi,
On Monday, 3 of October 2005 01:13, Pavel Machek wrote:
> Split swsusp.c into swsusp.c and snapshot.c. Snapshot only cares
> provides system snapshot/restore functionality, while swsusp.c will
> provide disk i/o. It should enable untangling of the code in future;
> swsusp.c parts can mostly be done in userspace.
>
> No code changes.
I think that the functions:
read_suspend_image()
read_pagedir()
swsusp_pagedir_relocate() (BTW, why there's "swsusp_"?)
check_pagedir() (BTW, misleading name)
data_read()
eat_page()
get_usable_page()
free_eaten_memory()
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.
Greetings,
Rafael
Hi!
> > Split swsusp.c into swsusp.c and snapshot.c. Snapshot only cares
> > provides system snapshot/restore functionality, while swsusp.c will
> > provide disk i/o. It should enable untangling of the code in future;
> > swsusp.c parts can mostly be done in userspace.
> >
> > No code changes.
>
> I think that the functions:
>
> read_suspend_image()
> read_pagedir()
> swsusp_pagedir_relocate() (BTW, why there's "swsusp_"?)
> check_pagedir() (BTW, misleading name)
> data_read()
> eat_page()
> get_usable_page()
> free_eaten_memory()
>
> 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. That
does not belong to snaphost. The rest is notthat clear, but I have it
working in userspace.
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.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Hi,
On Tuesday, 4 of October 2005 01:17, Pavel Machek wrote:
> Hi!
>
> > > Split swsusp.c into swsusp.c and snapshot.c. Snapshot only cares
> > > provides system snapshot/restore functionality, while swsusp.c will
> > > provide disk i/o. It should enable untangling of the code in future;
> > > swsusp.c parts can mostly be done in userspace.
> > >
> > > No code changes.
> >
> > I think that the functions:
> >
> > read_suspend_image()
> > read_pagedir()
> > swsusp_pagedir_relocate() (BTW, why there's "swsusp_"?)
> > check_pagedir() (BTW, misleading name)
> > data_read()
> > eat_page()
> > get_usable_page()
> > free_eaten_memory()
> >
> > 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).
> 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).
> 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),
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,
3) some cleanups are due before the splitting (eg. function names, the removal
of prepare_suspend_image() etc.),
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.
IMHO there could be the snapshot-handling part and the storage-handling
part interfacing via some functions (called by the snapshot-handling part)
like:
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()
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.
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.
Greetings,
Rafael
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 <[email protected]>
*
@@ -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 <[email protected]>
* Copyright (C) 1998,2001-2005 Pavel Machek <[email protected]>
@@ -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
Hi.
On Wed, 2005-10-05 at 06:53, Pavel Machek wrote:
> > > 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.
Not necessary, but desirable in your eyes. I can see that you can make
it work if you're only talking about implementing eye candy, but if
you're serious about adding the substantial improvements from Suspend2
(support for multiple swap partitions, swap files, block sizes != 4096,
asynchronous I/O, readhead where I/O must be synchronous, support for
writing to a network or a generic file (again with block size != 4096)
etc, - let alone support for saving a full image of memory - this is
just going to get uglier and uglier. We can see this already because
you've already dropped swap support, obviously because it's too hard
from userspace.
The tidy up you're proposing is a nice step. But it seems to me to be
the only good thing and really useful thing to come of this so far.
Pavel, at the PM summit, we agreed to work toward getting Suspend2
merged. I've been working since then on cleaning up the code, splitting
the patches up nicely and so on. In the meantime, you seem to have gone
off on a completely different tangent, going right against what we
agreed then. Can I get you to at least try to come back from that? I'd
be more than willing to help you with cherry picking some changes and
getting them in ahead of the rest of the code. Would you consider
working together to do that? In particular, I'm thinking that I could
send you the refrigerator patches as I have them at the moment, and a
patch to remove the reliance on PageReserved, at least for a start. Any
willingness on your part to give that a try?
Regards,
Nigel
--
Hi,
On Tuesday, 4 of October 2005 22:53, Pavel Machek wrote:
> Hi!
>
]-- snip --[
> > > 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.
I do agree with that and yes the split makes sense. The only problem is
I don't think the line of splitting, so to speak, is not very well defined.
I mean there are some functions that belong to the "grey zone"
and there really shouldn't be any.
> > > 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.
Well, that depends on which solution we'll end up with. I hope the last one,
ie. allocation of the resume page tables in the init code, in which case
it won't conflict with the split, indeed.
> > 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.
No, it doesn't, but the flow of changes would be easier to follow if the
cleanups were made first (ie. cleanup -> smaller and simpler code -> split).
> > 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.
>From the final code point of view, it doesn't, but by applying the cleanups
after the split we would make the whole thing practically irreversible.
I mean if we find out at some point that the split is not so great idea,
whatever the reason, we'll have to undo the cleanups to revert it and
apply the cleanups again. By applying the cleanups _first_ we won't have
to undo them in either case.
> > 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.
You are right, I have confused the sides. I should have said like that:
The snapshot-handling part makes some functions available to the
storage-handling part. The storage-handling part initializes suspend
or resume by calling specific function from the snapshot-handling part.
Then, it sends pages of data to the the snapshot-handling part or
receives pages of data from it (as many pages as needed). Finally,
it callls a function to finalize the process.
The whole point is that the storage-handling part need not care for
what data are contained in these pages as long as it is able to send or
receive them is a specific order. On the other hand, the snapshot-handling
part need not care for what happens to the pages of data send to the
storage-handling parts as long as it can receive them back in the same
order in which they have been sent.
]-- snip --[
>
> 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?
Well, aren't there any problems with handling kernel addresses from the user
space and vice versa?
Anyway, I think on resume we should send data from the user space to the
kernel and let the kernel arrange them in memory instead of placing them in
memory directly from the used space. By symmetry, on suspend we should send
data from the kernel to the user space instead of allowing the users space
to read memory at will. IMO the arrangement of the data in memory should
not be visible to the user space at all.
]-- snip --[
> 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...
OK
Still I'm afraid in the future we'll be moving some functions between
snapshot.c and swsusp.c back and forth ...
> Cleanup comments, remove unneccessary includes.
Sure all of this should be applied.
Greetings,
Rafael
Hi!
> > 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.
>
> No, it doesn't, but the flow of changes would be easier to follow if the
> cleanups were made first (ie. cleanup -> smaller and simpler code ->
> split).
I wanted to have a "this changes nothing" patch first. Cleanups in
front would be trickier to do because period of "settle down" is
needed before split. We now had quite a long "settle down" period, so
I've seen opportunity to do the split now.
> > No. It needs to be controlled by storage-handling parts, so that
> > snapshot-handling parts become nice library.
>
> You are right, I have confused the sides. I should have said like that:
> The snapshot-handling part makes some functions available to the
...
> part need not care for what happens to the pages of data send to the
> storage-handling parts as long as it can receive them back in the same
> order in which they have been sent.
Nicely said.
> > 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?
>
> Well, aren't there any problems with handling kernel addresses from the user
> space and vice versa?
Nothing we could not handle. Kernel needs to use get_user, while
userspace needs to seek/read/write on /dev/kmem (when accessing "the
other" addresses).
> Anyway, I think on resume we should send data from the user space to the
> kernel and let the kernel arrange them in memory instead of placing them in
> memory directly from the used space. By symmetry, on suspend we should send
> data from the kernel to the user space instead of allowing the users space
> to read memory at will. IMO the arrangement of the data in memory should
> not be visible to the user space at all.
I thought about that -- user/kernel interface would certainly be nicer
-- but I do not think it is feasible without writing a lot of code.
[I agree that assymetry I have in there is ugly, but I don't see a way
to do alloc_pagedir() in userspace, and I'd like to keep page
relocation in userspace.]
> Still I'm afraid in the future we'll be moving some functions between
> snapshot.c and swsusp.c back and forth ...
We may have to move function or two, but I think nothing too dramatic
will happen.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Hi,
On Wednesday, 5 of October 2005 02:06, Pavel Machek wrote:
> Hi!
>
> > > 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.
> >
> > No, it doesn't, but the flow of changes would be easier to follow if the
> > cleanups were made first (ie. cleanup -> smaller and simpler code ->
> > split).
>
> I wanted to have a "this changes nothing" patch first. Cleanups in
> front would be trickier to do because period of "settle down" is
> needed before split. We now had quite a long "settle down" period, so
> I've seen opportunity to do the split now.
OK, but if we decide to move some functions from one file to another,
we'll have to wait for another "settle down" period, I think.
> > > No. It needs to be controlled by storage-handling parts, so that
> > > snapshot-handling parts become nice library.
> >
> > You are right, I have confused the sides. I should have said like that:
> > The snapshot-handling part makes some functions available to the
> ...
> > part need not care for what happens to the pages of data send to the
> > storage-handling parts as long as it can receive them back in the same
> > order in which they have been sent.
>
> Nicely said.
Thanks, and I belive it can be made that way.
> > > 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?
> >
> > Well, aren't there any problems with handling kernel addresses from the user
> > space and vice versa?
>
> Nothing we could not handle. Kernel needs to use get_user, while
> userspace needs to seek/read/write on /dev/kmem (when accessing "the
> other" addresses).
Shouldn't we avoid using /dev/kmem? Especially that some people wanted it
to go already.
> > Anyway, I think on resume we should send data from the user space to the
> > kernel and let the kernel arrange them in memory instead of placing them in
> > memory directly from the used space. By symmetry, on suspend we should send
> > data from the kernel to the user space instead of allowing the users space
> > to read memory at will. IMO the arrangement of the data in memory should
> > not be visible to the user space at all.
>
> I thought about that -- user/kernel interface would certainly be nicer
> -- but I do not think it is feasible without writing a lot of code.
I thought of two in-kernel subsystems with a well defined interface
between them first, such that one of them could be replaced with a
user space implementation (partially or as a whole) in the future.
I think I can come up with a proof-of-concept patch if that helps.
> [I agree that assymetry I have in there is ugly, but I don't see a way
> to do alloc_pagedir() in userspace, and I'd like to keep page
> relocation in userspace.]
I'd think the page relocation is easier in the kernel, as we have access to
zones and we can use swsusp_free() to clean things up if something goes
wrong (of course that's after some changes).
Greetings,
Rafael
Hi!
> OK, but if we decide to move some functions from one file to another,
> we'll have to wait for another "settle down" period, I think.
Yes...
> > > Well, aren't there any problems with handling kernel addresses from the user
> > > space and vice versa?
> >
> > Nothing we could not handle. Kernel needs to use get_user, while
> > userspace needs to seek/read/write on /dev/kmem (when accessing "the
> > other" addresses).
>
> Shouldn't we avoid using /dev/kmem? Especially that some people wanted it
> to go already.
Well, I believe it is okay for suspend.
> > > Anyway, I think on resume we should send data from the user space to the
> > > kernel and let the kernel arrange them in memory instead of placing them in
> > > memory directly from the used space. By symmetry, on suspend we should send
> > > data from the kernel to the user space instead of allowing the users space
> > > to read memory at will. IMO the arrangement of the data in memory should
> > > not be visible to the user space at all.
> >
> > I thought about that -- user/kernel interface would certainly be nicer
> > -- but I do not think it is feasible without writing a lot of code.
>
> I thought of two in-kernel subsystems with a well defined interface
> between them first, such that one of them could be replaced with a
> user space implementation (partially or as a whole) in the future.
>
> I think I can come up with a proof-of-concept patch if that helps.
Yes, it would help. (But notice that I have proof-of-concenpt ready in
sw3 tree :-).
> > [I agree that assymetry I have in there is ugly, but I don't see a way
> > to do alloc_pagedir() in userspace, and I'd like to keep page
> > relocation in userspace.]
>
> I'd think the page relocation is easier in the kernel, as we have access to
> zones and we can use swsusp_free() to clean things up if something goes
> wrong (of course that's after some changes).
It is quite a lot of ugly code, and being in kernel does not really
help. swsusp_free() will need to be exported, but we need it for
normal (non-error) code path, too, so that should not be big problem.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Hi!
> Not necessary, but desirable in your eyes. I can see that you can make
> it work if you're only talking about implementing eye candy, but if
> you're serious about adding the substantial improvements from Suspend2
> (support for multiple swap partitions, swap files, block sizes != 4096,
> asynchronous I/O, readhead where I/O must be synchronous, support for
> writing to a network or a generic file (again with block size != 4096)
> etc, - let alone support for saving a full image of memory - this is
> just going to get uglier and uglier. We can see this already because
> you've already dropped swap support, obviously because it's too hard
> from userspace.
swap support needs "allocate me swap page" ioctl/syscall/whatever;
that can be arranged. Then you can have as many swap partition, swap
files and normal files as you want, and block size will not really
matter. Network and generic file writing should be possible now.
Full image of memory... No, I can't do that, but Rafael already has
some patches that get "close enough" -- they keep system responsive
after resume, without major uglyness/rewrite.
> The tidy up you're proposing is a nice step. But it seems to me to be
> the only good thing and really useful thing to come of this so far.
Thanks. There are more nice cleanups coming, and then we may add
userspace interface.
> Pavel, at the PM summit, we agreed to work toward getting Suspend2
> merged. I've been working since then on cleaning up the code, splitting
> the patches up nicely and so on. In the meantime, you seem to have gone
> off on a completely different tangent, going right against what we
> agreed then. Can I get you to at least try to come back from that?
> I'd
Sorry about that. At pm summit, I did not know if uswsusp was
feasible. Now I'm pretty sure it is (code works and is stable).
> be more than willing to help you with cherry picking some changes and
> getting them in ahead of the rest of the code. Would you consider
> working together to do that? In particular, I'm thinking that I could
> send you the refrigerator patches as I have them at the moment, and a
> patch to remove the reliance on PageReserved, at least for a start. Any
> willingness on your part to give that a try?
I'd certainly like to understand mysqld refrigerator failure, and have
it fixed. (But that should be few lines.) Other than that, I do not
think refrigerator is broken.
PageReserved... lets see if it is small enough.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Pavel Machek wrote:
>> Pavel, at the PM summit, we agreed to work toward getting Suspend2
>> merged. I've been working since then on cleaning up the code, splitting
>> the patches up nicely and so on. In the meantime, you seem to have gone
>> off on a completely different tangent, going right against what we
>> agreed then.
> Sorry about that. At pm summit, I did not know if uswsusp was
> feasible. Now I'm pretty sure it is (code works and is stable).
Ok, excuse me for butting in.
I would just like to give the point of view of a user.
I have been using suspend2 probably at least once a day for about a year
now, and I love it. I have had zero cases of data corruption, and it's
fast, effective, and reliable. I can't say the same about the in-kernel
swsusp. When I tried it (once), a few months ago:
- It was dog slow because it doesn't use compression
- Even though it's dog slow, it doesn't save all RAM
- Therefore the machine is dog slow after resume
- It doesn't have a decent UI
- There is no way to abort suspend once it's started. (Whatever others
may say, this /is/ useful, especially when you've forgotten something
and you're in a hurry and don't have two more minutes to waste waiting
for a suspend/resume cycle.)
These points /do/ matter to users: after all, if we all had time to
waste we'd never use suspend or S3, we'd just reboot all the time...
I have been waiting for swsusp2 to be merged ever since I started using
it. When I read about the discussion at the PM summit, I hoped that this
would finally happen. Now I see that it's not, and instead work is going
to continue on what is - or at least seemed to be when I tried it - an
inferior implementation. From my point of view as a user, this seems
silly. There may be all the technical reasons in the world to dislike
suspend2; on these, I defer to everyone else, since I'm no kernel
hacker. But from the point of view of a user, well, suspend2 is much better.
So, instead of working on getting swsusp, which is still far behind in
terms of functionality, up to the level of suspend2, why not work
together on merging swsusp2, which is fast, stable and provides what
users want and need?
Cheers,
Lorenzo
--
http://www.colitti.com/lorenzo/
Hi Lorenzo et al.
On Thu, 2005-10-06 at 07:21, Lorenzo Colitti wrote:
> Pavel Machek wrote:
> >> Pavel, at the PM summit, we agreed to work toward getting Suspend2
> >> merged. I've been working since then on cleaning up the code, splitting
> >> the patches up nicely and so on. In the meantime, you seem to have gone
> >> off on a completely different tangent, going right against what we
> >> agreed then.
> > Sorry about that. At pm summit, I did not know if uswsusp was
> > feasible. Now I'm pretty sure it is (code works and is stable).
>
> Ok, excuse me for butting in.
>
> I would just like to give the point of view of a user.
>
> I have been using suspend2 probably at least once a day for about a year
> now, and I love it. I have had zero cases of data corruption, and it's
> fast, effective, and reliable. I can't say the same about the in-kernel
> swsusp. When I tried it (once), a few months ago:
>
> - It was dog slow because it doesn't use compression
> - Even though it's dog slow, it doesn't save all RAM
> - Therefore the machine is dog slow after resume
> - It doesn't have a decent UI
> - There is no way to abort suspend once it's started. (Whatever others
> may say, this /is/ useful, especially when you've forgotten something
> and you're in a hurry and don't have two more minutes to waste waiting
> for a suspend/resume cycle.)
>
> These points /do/ matter to users: after all, if we all had time to
> waste we'd never use suspend or S3, we'd just reboot all the time...
>
> I have been waiting for swsusp2 to be merged ever since I started using
> it. When I read about the discussion at the PM summit, I hoped that this
> would finally happen. Now I see that it's not, and instead work is going
> to continue on what is - or at least seemed to be when I tried it - an
> inferior implementation. From my point of view as a user, this seems
> silly. There may be all the technical reasons in the world to dislike
> suspend2; on these, I defer to everyone else, since I'm no kernel
> hacker. But from the point of view of a user, well, suspend2 is much better.
>
> So, instead of working on getting swsusp, which is still far behind in
> terms of functionality, up to the level of suspend2, why not work
> together on merging swsusp2, which is fast, stable and provides what
> users want and need?
Thanks for the support.
Just to clarify a little, the main reason (from my point of view) that
Suspend2 hasn't been merged prior to now is that I haven't asked for
that. I have sought code reviews a couple of times, and have really
appreciated the feedback. But so far I haven't sought for the code to be
merged. This has been for a number of reasons:
First, I've been seeking to make it as mature and bug free as I can
prior to the merge so that the merge itself creates as few problems as
possible. Part of this has involved spending a long time cleaning up
code, improving the commenting and so on.
Second, getting it in a form that Andrew and Linus can use involves
preparing a git tree for them to pull from, and this requires splitting
the patches up into discrete functions and pairs of functions. Doing
this and writing descriptions for each patch has also taken a long time,
especially because Suspend is by no means the only thing I do with my
life (although my wife sometimes feels otherwise!).
So, then, at least part of the blame for Suspend2 not being merged yet
must lie with me, sorry. I am seeking to address this, and trying not to
be too much of a perfectionist, but it will take a little bit longer
yet.
Of course even when I think I'm ready, it doesn't mean others will
agree, so don't expect it to happen the instant I become satisfied :).
Regards,
Nigel
>
> Cheers,
> Lorenzo
Hi!
> >>Pavel, at the PM summit, we agreed to work toward getting Suspend2
> >>merged. I've been working since then on cleaning up the code, splitting
> >>the patches up nicely and so on. In the meantime, you seem to have gone
> >>off on a completely different tangent, going right against what we
> >>agreed then.
> >Sorry about that. At pm summit, I did not know if uswsusp was
> >feasible. Now I'm pretty sure it is (code works and is stable).
>
> Ok, excuse me for butting in.
>
> I would just like to give the point of view of a user.
>
> I have been using suspend2 probably at least once a day for about a year
> now, and I love it. I have had zero cases of data corruption, and it's
> fast, effective, and reliable. I can't say the same about the in-kernel
> swsusp. When I tried it (once), a few months ago:
>
> - It was dog slow because it doesn't use compression
> - Even though it's dog slow, it doesn't save all RAM
> - Therefore the machine is dog slow after resume
> - It doesn't have a decent UI
> - There is no way to abort suspend once it's started. (Whatever others
> may say, this /is/ useful, especially when you've forgotten something
> and you're in a hurry and don't have two more minutes to waste waiting
> for a suspend/resume cycle.)
With uswsusp (aka swsusp3), you can do all this in userland. Stop
whining, start hacking... Code is at kernel.org/git/.../linux-sw3.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Pavel Machek wrote:
>> - It was dog slow because it doesn't use compression
>> - Even though it's dog slow, it doesn't save all RAM
>> - Therefore the machine is dog slow after resume
>> - It doesn't have a decent UI
>> - There is no way to abort suspend once it's started. [...]
>
> With uswsusp (aka swsusp3), you can do all this in userland. Stop
> whining, start hacking... Code is at kernel.org/git/.../linux-sw3.
But that was exactly my point: there's no need to hack!
The code is there. It's well tested, fast, stable, and does what users
need. It's called suspend2. Why work on yet another implementation
instead of just merging that?
Cheers,
Lorenzo
--
http://www.colitti.com/lorenzo/
On Čt 06-10-05 00:54:19, Lorenzo Colitti wrote:
> Pavel Machek wrote:
> >>- It was dog slow because it doesn't use compression
> >>- Even though it's dog slow, it doesn't save all RAM
> >> - Therefore the machine is dog slow after resume
> >>- It doesn't have a decent UI
> >>- There is no way to abort suspend once it's started. [...]
> >
> >With uswsusp (aka swsusp3), you can do all this in userland. Stop
> >whining, start hacking... Code is at kernel.org/git/.../linux-sw3.
>
> But that was exactly my point: there's no need to hack!
>
> The code is there. It's well tested, fast, stable, and does what users
> need. It's called suspend2. Why work on yet another implementation
> instead of just merging that?
Most of that code does not belong it kernel. It can't be "just
merged". If we had nice "vi" implementation in kernel, we'd have to
drop it and start again in userland. This is similar.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
On Čt 06-10-05 00:57:27, Pavel Machek wrote:
> On Čt 06-10-05 00:54:19, Lorenzo Colitti wrote:
> > Pavel Machek wrote:
> > >>- It was dog slow because it doesn't use compression
> > >>- Even though it's dog slow, it doesn't save all RAM
> > >> - Therefore the machine is dog slow after resume
> > >>- It doesn't have a decent UI
> > >>- There is no way to abort suspend once it's started. [...]
> > >
> > >With uswsusp (aka swsusp3), you can do all this in userland. Stop
> > >whining, start hacking... Code is at kernel.org/git/.../linux-sw3.
> >
> > But that was exactly my point: there's no need to hack!
> >
> > The code is there. It's well tested, fast, stable, and does what users
> > need. It's called suspend2. Why work on yet another implementation
> > instead of just merging that?
>
> Most of that code does not belong it kernel. It can't be "just
> merged". If we had nice "vi" implementation in kernel, we'd have to
> drop it and start again in userland. This is similar.
To say it more nicely.
"Cool, so you have done 100% of work and now it is stable, fast and
tested. You only need to do 200% more work to get it merged".
Merging into kernel is not easy, sorry.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Pavel Machek wrote:
> "Cool, so you have done 100% of work and now it is stable, fast and
> tested. You only need to do 200% more work to get it merged".
>
> Merging into kernel is not easy, sorry.
Of course. I was just saying that it might be better for users if you,
Nigel and the other parties involved collaborated on getting suspend2
merged instead of working on 2 or 3 separate implementations. My gut
feeling is that it would be quicker to do that than to nail the sucky
low-level bugs that are bound to come up when developing the new uswsusp
framework.
And now, since there's nothing I can actually do to help this - you
can't learn to be a kernel hacker in two weeks - I'll shut up. But I
just wanted to remind everyone involved that from a user's perspective,
it would be great if suspend2 were merged.
It works, it's fast, it's stable, and it does what users want. That's a
strong combination. It would be really good if you could work together
to merge it.
Cheers,
Lorenzo
--
http://www.colitti.com/lorenzo/
Lorenzo Colitti wrote:
> Pavel Machek wrote:
>
>> "Cool, so you have done 100% of work and now it is stable, fast and
>> tested. You only need to do 200% more work to get it merged".
>>
>> Merging into kernel is not easy, sorry.
> >
> It works, it's fast, it's stable, and it does what users want. That's a
> strong combination. It would be really good if you could work together
> to merge it.
>
>
> Cheers,
> Lorenzo
>
Hello,
I've read this thread...
And I want to join this request...
It might be that suspend2 is not the best solution, but
maybe if you merge it and then work together in order to
maximize the use of user space at next version, it will be
slower process but at least the interfaces suspend2 provides
will be available to users.
I understand the wish to minimize kernel mode usage... But
first you should support a decent suspend disk/suspend ram
support in Linux without forcing the user to hack his
configuration.
At my culture we say: "The enemy of the good is the best"...
Best Regards,
Alon Bar-Lev.
Hi,
On Wednesday, 5 of October 2005 10:33, Pavel Machek wrote:
> Hi!
>
> > OK, but if we decide to move some functions from one file to another,
> > we'll have to wait for another "settle down" period, I think.
>
> Yes...
Then I'd propose that we wait for the next "settle down" period with the
split and apply all of the bugfixes and cleanups now.
I don't think we'll gain anything by splitting the files immediately and
there are some cleanups not in-line with the split (eg. the Nigel's patch
eliminating the references to PG_reserved).
Greetings,
Rafael
Hi!
> > > OK, but if we decide to move some functions from one file to another,
> > > we'll have to wait for another "settle down" period, I think.
> >
> > Yes...
>
> Then I'd propose that we wait for the next "settle down" period with the
> split and apply all of the bugfixes and cleanups now.
Nigel's cleanup is not ready yet, and yours is oneliner. I applied
that oneliner locally. I already have cleanups depending on the
split. Of course I can redo them, but perhaps it is easier to just
redo that one line.
Pavel
--
if you have sharp zaurus hardware you don't need... you know my address
Hi,
On Thursday, 6 of October 2005 12:42, Pavel Machek wrote:
> Hi!
>
> > > > OK, but if we decide to move some functions from one file to another,
> > > > we'll have to wait for another "settle down" period, I think.
> > >
> > > Yes...
> >
> > Then I'd propose that we wait for the next "settle down" period with the
> > split and apply all of the bugfixes and cleanups now.
>
> Nigel's cleanup is not ready yet, and yours is oneliner. I applied
> that oneliner locally. I already have cleanups depending on the
> split. Of course I can redo them, but perhaps it is easier to just
> redo that one line.
OK
Just to make sure:
- the rework-image-freeing patch goes first,
- my x86-64 patch goes second,
- the splitting patch goes next,
and any subsequent cleanups are expected to go on top of it?
Rafael
Hi!
> >>"Cool, so you have done 100% of work and now it is stable, fast and
> >>tested. You only need to do 200% more work to get it merged".
> >>
> >>Merging into kernel is not easy, sorry.
> >>
> >It works, it's fast, it's stable, and it does what users want.
> >That's a strong combination. It would be really good if you could
> >work together to merge it.
> I've read this thread...
> And I want to join this request...
>
> It might be that suspend2 is not the best solution, but
> maybe if you merge it and then work together in order to
> maximize the use of user space at next version, it will be
Merge 10000 lines of code, only to drop them in next version?
Better not.
Pavel
--
64 bytes from 195.113.31.123: icmp_seq=28 ttl=51 time=448769.1 ms
Hi Pavel.
On Thu, 2005-10-06 at 18:20, Pavel Machek wrote:
> Hi!
>
> > >>"Cool, so you have done 100% of work and now it is stable, fast and
> > >>tested. You only need to do 200% more work to get it merged".
> > >>
> > >>Merging into kernel is not easy, sorry.
> > >>
> > >It works, it's fast, it's stable, and it does what users want.
> > >That's a strong combination. It would be really good if you could
> > >work together to merge it.
>
> > I've read this thread...
> > And I want to join this request...
> >
> > It might be that suspend2 is not the best solution, but
> > maybe if you merge it and then work together in order to
> > maximize the use of user space at next version, it will be
>
> Merge 10000 lines of code, only to drop them in next version?
You're assuming your code would get merged.
More than that, though, let me be bold enough to say that your version
has quite a lot of development to do in order to begin to approach the
level of Suspend2. If and when it achieves that, I'll happily stop
maintaining Suspend2 - after all, I've always said I'm just a user who
wants to suspend. In the meantime, though, I'm happy to carry that can
while you're developing your userspace implementation. That way, users
can have the best of both worlds - a stable, reliable, fast and mature
implementation in kernel space while you're getting userspace working,
and then whatever advantages you bring once your version becomes an
improved version of Suspend2.
Regards,
Nigel