Remove page flags for software suspend

I think we can just move the flags completely into the kernel/power directory? This centralizes all your handling of pageflags into snapshot.c so that you need no external definitions anymore.

Index: linux-2.6.20-mm1/include/linux/mmzone.h
===================================================================

include/linux/mmzone.h +1 -0

@@ -295,6 +295,7 @@ struct zone {
 	unsigned long		spanned_pages;	/* total size, including holes */
 	unsigned long		present_pages;	/* amount of memory (excluding holes) */
 
+	unsigned long		*suspend_flags;
 	/*
 	 * rarely used fields:
 	 */

Index: linux-2.6.20-mm1/include/linux/page-flags.h
===================================================================

include/linux/page-flags.h +0 -12

@@ -82,13 +82,11 @@
 #define PG_private		11	/* If pagecache, has fs-private data */
 
 #define PG_writeback		12	/* Page is under writeback */
-#define PG_nosave		13	/* Used for system suspend/resume */
 #define PG_compound		14	/* Part of a compound page */
 #define PG_swapcache		15	/* Swap page: swp_entry_t in private */
 
 #define PG_mappedtodisk		16	/* Has blocks allocated on-disk */
 #define PG_reclaim		17	/* To be reclaimed asap */
-#define PG_nosave_free		18	/* Used for system suspend/resume */
 #define PG_buddy		19	/* Page is free, on buddy lists */
 
 #define PG_mlocked		20	/* Page is mlocked */

@@ -192,16 +190,6 @@ static inline void SetPageUptodate(struc
 #define TestClearPageWriteback(page) test_and_clear_bit(PG_writeback,	\
 							&(page)->flags)
 
-#define PageNosave(page)	test_bit(PG_nosave, &(page)->flags)
-#define SetPageNosave(page)	set_bit(PG_nosave, &(page)->flags)
-#define TestSetPageNosave(page)	test_and_set_bit(PG_nosave, &(page)->flags)
-#define ClearPageNosave(page)		clear_bit(PG_nosave, &(page)->flags)
-#define TestClearPageNosave(page)	test_and_clear_bit(PG_nosave, &(page)->flags)
-
-#define PageNosaveFree(page)	test_bit(PG_nosave_free, &(page)->flags)
-#define SetPageNosaveFree(page)	set_bit(PG_nosave_free, &(page)->flags)
-#define ClearPageNosaveFree(page)		clear_bit(PG_nosave_free, &(page)->flags)
-
 #define PageBuddy(page)		test_bit(PG_buddy, &(page)->flags)
 #define __SetPageBuddy(page)	__set_bit(PG_buddy, &(page)->flags)
 #define __ClearPageBuddy(page)	__clear_bit(PG_buddy, &(page)->flags)

Index: linux-2.6.20-mm1/include/linux/suspend.h
===================================================================

include/linux/suspend.h +12 -1

@@ -21,7 +22,6 @@ struct pbe {
 
 /* mm/page_alloc.c */
 extern void drain_local_pages(void);
-extern void mark_free_pages(struct zone *zone);
 
 #ifdef CONFIG_PM
 /* kernel/power/swsusp.c */

@@ -42,6 +42,18 @@ static inline int software_suspend(void)
 }
 #endif /* CONFIG_PM */
 
+#ifdef CONFIG_SOFTWARE_SUSPEND
+int suspend_flags_init(struct zone *zone, unsigned long zone_size_pages);
+void mark_free_pages(struct zone *zone);
+#else
+static inline int suspend_flags_init(struct zone *zone, unsigned long zone_size_pages)
+{
+	return 0;
+}
+
+static inline void mark_free_pages(struct zone *zone) {}
+#endif
+
 void save_processor_state(void);
 void restore_processor_state(void);
 struct saved_context;

Index: linux-2.6.20-mm1/mm/page_alloc.c
===================================================================

mm/page_alloc.c +3 -34

@@ -767,40 +767,6 @@ static void __drain_pages(unsigned int c
 }
 
 #ifdef CONFIG_PM
-
-void mark_free_pages(struct zone *zone)
-{
-	unsigned long pfn, max_zone_pfn;
-	unsigned long flags;
-	int order;
-	struct list_head *curr;
-
-	if (!zone->spanned_pages)
-		return;
-
-	spin_lock_irqsave(&zone->lock, flags);
-
-	max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages;
-	for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++)
-		if (pfn_valid(pfn)) {
-			struct page *page = pfn_to_page(pfn);
-
-			if (!PageNosave(page))
-				ClearPageNosaveFree(page);
-		}
-
-	for (order = MAX_ORDER - 1; order >= 0; --order)
-		list_for_each(curr, &zone->free_area[order].free_list) {
-			unsigned long i;
-
-			pfn = page_to_pfn(list_entry(curr, struct page, lru));
-			for (i = 0; i < (1UL << order); i++)
-				SetPageNosaveFree(pfn_to_page(pfn + i));
-		}
-
-	spin_unlock_irqrestore(&zone->lock, flags);
-}
-
 /*
  * Spill all of this CPU's per-cpu pages back into the buddy allocator.
  */

@@ -2354,6 +2320,9 @@ __meminit int init_currently_empty_zone(
 	ret = zone_wait_table_init(zone, size);
 	if (ret)
 		return ret;
+	ret = suspend_flags_init(zone, size);
+	if (ret)
+		return ret;
 	pgdat->nr_zones = zone_idx(zone) + 1;
 
 	zone->zone_start_pfn = zone_start_pfn;

Index: linux-2.6.20-mm1/kernel/power/snapshot.c
===================================================================

kernel/power/snapshot.c +12 -0

@@ -34,6 +34,126 @@
 
 #include "power.h"
 
+static inline int PageNosave(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;
+
+	return test_bit(offset * 2, zone->suspend_flags);
+}
+
+static inline void SetPageNosave(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;

kernel/power/snapshot.c +91 -0

+}
+
+static inline void ClearPageNosave(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;
+
+	clear_bit(offset * 2, zone->suspend_flags);
+}
+
+static inline int TestClearPageNosave(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;
+
+	return test_and_clear_bit(offset * 2, zone->suspend_flags);
+}
+
+
+static inline int PageNosaveFree(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;
+
+	return test_bit(offset * 2 + 1, zone->suspend_flags);
+}
+
+static inline void SetPageNosaveFree(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;
+
+	set_bit(offset * 2 + 1, zone->suspend_flags);
+}
+
+static inline void ClearPageNosaveFree(struct page *page)
+{
+	struct zone *zone = page_zone(page);
+	unsigned long offset = page_to_pfn(page) - zone->zone_start_pfn;
+
+	clear_bit(offset * 2 + 1, zone->suspend_flags);
+}
+
+int suspend_flags_init(struct zone *zone, unsigned long zone_size_pages)
+{
+	struct pglist_data *pgdat = zone->zone_pgdat;
+	size_t alloc_size;
+
+	/*
+	 * We need two bits per page in the zone. One for PageNosave and the other
+	 * for PageNosaveFree.
+	 */
+	alloc_size = BITS_TO_LONGS(zone_size_pages * 2);
+ 	if (system_state == SYSTEM_BOOTING) {
+		zone->suspend_flags = (unsigned long *)
+			alloc_bootmem_node(pgdat, alloc_size);
+	} else
+		zone->suspend_flags = (unsigned long *)vmalloc(alloc_size);
+	if (!zone->suspend_flags)
+		return -ENOMEM;
+
+	bitmap_zero(zone->suspend_flags, 2 * zone_size_pages);
+	return 0;
+}
+
+void mark_free_pages(struct zone *zone)
+{
+	unsigned long pfn, max_zone_pfn;
+	unsigned long flags;
+	int order;
+	struct list_head *curr;
+
+	if (!zone->spanned_pages)
+		return;
+
+	spin_lock_irqsave(&zone->lock, flags);
+
+	max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages;
+	for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++)
+		if (pfn_valid(pfn)) {
+			struct page *page = pfn_to_page(pfn);
+
+			if (!PageNosave(page))
+				ClearPageNosaveFree(page);
+		}
+
+	for (order = MAX_ORDER - 1; order >= 0; --order)
+		list_for_each(curr, &zone->free_area[order].free_list) {
+			unsigned long i;
+
+			pfn = page_to_pfn(list_entry(curr, struct page, lru));

Quoted 3 lines

I think we can just move the flags completely into the kernel/power directory? This centralizes all your handling of pageflags into snapshot.c so that you need no external definitions anymore.

Yes, I think we can do it this way, but can we generally assume that the offset for eg. test_bit() won't be taken modulo 32 (or 64)?

I... actually do not like that patch. It adds code... at little or no benefit.

Quoted 2 lines · Pavel Machek

I... actually do not like that patch. It adds code... at little or no benefit.

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

Quoted 3 lines · Rafael J. Wysocki

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

Ummm.. There are other parties who would like to use these flags!

I think my patch localizes the suspend material properly. In fact there is *no* reason for the page flags to be visible outside of snapshot.c.

What is the problem with the patch?

How long will it take you to remove the flags on your own?

Yes, I know that. On the other hand, we have terminally broken CPU hotplug code in the kernel that I'd like to get fixed _first_.

PageNosaveFree is only needed at the suspend time, so we need not allocate it in advance.

Quoted 2 lines · Rafael J. Wysocki

Yes, I know that. On the other hand, we have terminally broken CPU hotplug code in the kernel that I'd like to get fixed _first_.

The cpu hotplug code has been terminal for years.

Quoted 2 lines

PageNosaveFree is only needed at the suspend time, so we need not allocate it in advance.

Well it should be simple to change the patch to allocate the bitmaps later.

Well, yes, I think so. Still, there may be another way of doing it and I need some time to have a look.

BTW, have you tested the patch?

Quoted 4 lines

Well, yes, I think so. Still, there may be another way of doing it and I need some time to have a look.

BTW, have you tested the patch?

Nope. Sorry, have no use for software suspend.

/sys/power/state... I guess that is not too much to ask?

Quoted 3 lines · Rafael J. Wysocki

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

Ummm.. There are other parties who would like to use these flags!

Quoted 6 lines · Christoph Lameter

Quoted 2 lines · Pavel Machek

I... actually do not like that patch. It adds code... at little or no benefit.

Quoted 1 line

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

Quoted 8 lines · Rafael J. Wysocki

Quoted 6 lines · Christoph Lameter

Quoted 2 lines · Pavel Machek

I... actually do not like that patch. It adds code... at little or no benefit.

Quoted 8 lines

Quoted 3 lines

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

I need one bit for lockless pagecache ;)

Anyway, I guess if you want something done you have to do it yourself.

This patch still needs work (and I don't know if it even works, because I can't make swsusp resume even on a vanilla kernel). But this is my WIP for removing swsusp page flags.

This patch adds a simple extent based nosave region tracker, and rearranges some of the snapshot code to be a bit simpler and more amenable to having dynamically allocated flags (they aren't actually dynamically allocated in this patch, however).

Quoted 22 lines · Nick Piggin

Quoted 8 lines · Rafael J. Wysocki

Quoted 6 lines · Christoph Lameter

Quoted 2 lines · Pavel Machek

I... actually do not like that patch. It adds code... at little or no benefit.

Quoted 8 lines

Quoted 3 lines

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

I need one bit for lockless pagecache ;)

Anyway, I guess if you want something done you have to do it yourself.

Quoted 8 lines

This patch still needs work (and I don't know if it even works, because I can't make swsusp resume even on a vanilla kernel). But this is my WIP for removing swsusp page flags.

This patch adds a simple extent based nosave region tracker, and rearranges some of the snapshot code to be a bit simpler and more amenable to having dynamically allocated flags (they aren't actually dynamically allocated in this patch, however).

Thanks for the patch.

Probably I'd like to do some things in a different way, I'll think about that later today.

I hope I'll have a working patch that removes the "offending" page flags after the weekend.

Quoted 22 lines · Nick Piggin

Quoted 8 lines · Rafael J. Wysocki

Quoted 6 lines · Christoph Lameter

Quoted 2 lines · Pavel Machek

I... actually do not like that patch. It adds code... at little or no benefit.

Quoted 8 lines

Quoted 3 lines

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

I need one bit for lockless pagecache ;)

Anyway, I guess if you want something done you have to do it yourself.

Quoted 8 lines

This patch still needs work (and I don't know if it even works, because I can't make swsusp resume even on a vanilla kernel). But this is my WIP for removing swsusp page flags.

This patch adds a simple extent based nosave region tracker, and rearranges some of the snapshot code to be a bit simpler and more amenable to having dynamically allocated flags (they aren't actually dynamically allocated in this patch, however).

Thanks for the patch.

Probably I'd like to do some things in a different way, I'll think about that later today.

I hope I'll have a working patch that removes the "offending" page flags after the weekend.

Make swsusp stop using SetPageNosave(), SetPageNosaveFree() and friends directly.

This way the amount of changes made in the next patch is smaller.

---
 include/linux/suspend.h |   33 +++++++++++++++++++++++++++++++++
 kernel/power/snapshot.c |   48 +++++++++++++++++++++++++-----------------------
 mm/page_alloc.c         |    6 +++---
 3 files changed, 61 insertions(+), 26 deletions(-)

Index: linux-2.6.21-rc2/include/linux/suspend.h
===================================================================

include/linux/suspend.h +22 -0

@@ -8,6 +8,7 @@
 #include <linux/notifier.h>
 #include <linux/init.h>
 #include <linux/pm.h>
+#include <linux/mm.h>
 
 /* struct pbe is used for creating lists of pages that should be restored
  * atomically during the resume from disk, because the page frames they have

@@ -49,6 +50,38 @@ void __save_processor_state(struct saved
 void __restore_processor_state(struct saved_context *ctxt);
 unsigned long get_safe_page(gfp_t gfp_mask);
 
+/* Page management functions for the software suspend (swsusp) */
+
+static inline void swsusp_set_page_forbidden(struct page *page)
+{
+	SetPageNosave(page);
+}
+
+static inline int swsusp_page_is_forbidden(struct page *page)
+{
+	return PageNosave(page);
+}
+
+static inline void swsusp_unset_page_forbidden(struct page *page)
+{
+	ClearPageNosave(page);
+}
+
+static inline void swsusp_set_page_free(struct page *page)
+{
+	SetPageNosaveFree(page);
+}

}

/**
+ * This structure represents a range of page frames the contents of which
+ * should not be saved during the suspend.
+ */

kernel/power/snapshot.c +31 -0

+
+struct nosave_region {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
+static LIST_HEAD(nosave_regions);
+
+/**
+ *	register_nosave_region - register a range of page frames the contents
+ *	of which should not be saved during the suspend (to be used in the early
+ *	initializatoion code)
+ */
+
+void __init
+register_nosave_region(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct nosave_region *region;
+
+	if (start_pfn >= end_pfn)
+		return;
+
+	if (!list_empty(&nosave_regions)) {
+		/* Try to extend the previous region (they should be sorted) */
+		region = list_entry(nosave_regions.prev,
+					struct nosave_region, list);
+		if (region->end_pfn == start_pfn) {
+			region->end_pfn = end_pfn;
+			goto Report;
+		}

Quoted 44 lines · Rafael J. Wysocki

}

/**
+ * This structure represents a range of page frames the contents of which
+ * should not be saved during the suspend.
+ */

kernel/power/snapshot.c +38 -0

+
+struct nosave_region {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
+static LIST_HEAD(nosave_regions);
+
+/**
+ *	register_nosave_region - register a range of page frames the contents
+ *	of which should not be saved during the suspend (to be used in the early
+ *	initializatoion code)
+ */
+
+void __init
+register_nosave_region(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct nosave_region *region;
+
+	if (start_pfn >= end_pfn)
+		return;
+
+	if (!list_empty(&nosave_regions)) {
+		/* Try to extend the previous region (they should be sorted) */
+		region = list_entry(nosave_regions.prev,
+					struct nosave_region, list);
+		if (region->end_pfn == start_pfn) {
+			region->end_pfn = end_pfn;
+			goto Report;
+		}
+	}
+	/* This allocation cannot fail */
+	region = alloc_bootmem_low(sizeof(struct nosave_region));
+	region->start_pfn = start_pfn;
+	region->end_pfn = end_pfn;
+	list_add_tail(&region->list, &nosave_regions);
+ Report:

kernel/power/snapshot.c static unsigned long memory_bm_next_pfn( · line 635

Quoted 3 lines

kernel/power/snapshot.c +3 -0

+	printk("swsusp: Registered nosave memory region: %016lx - %016lx\n",
+		start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
+}

I wonder why you reimplemented this and put it in snapshot.c, rather than use my version which was nicely in its own file, had appropriate locking, etc.?

Quoted 47 lines · Nick Piggin

Quoted 44 lines · Rafael J. Wysocki

}

/**
+ * This structure represents a range of page frames the contents of which
+ * should not be saved during the suspend.
+ */

kernel/power/snapshot.c +38 -0

+
+struct nosave_region {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
+static LIST_HEAD(nosave_regions);
+
+/**
+ *	register_nosave_region - register a range of page frames the contents
+ *	of which should not be saved during the suspend (to be used in the early
+ *	initializatoion code)
+ */
+
+void __init
+register_nosave_region(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct nosave_region *region;
+
+	if (start_pfn >= end_pfn)
+		return;
+
+	if (!list_empty(&nosave_regions)) {
+		/* Try to extend the previous region (they should be sorted) */
+		region = list_entry(nosave_regions.prev,
+					struct nosave_region, list);
+		if (region->end_pfn == start_pfn) {
+			region->end_pfn = end_pfn;
+			goto Report;
+		}
+	}
+	/* This allocation cannot fail */
+	region = alloc_bootmem_low(sizeof(struct nosave_region));
+	region->start_pfn = start_pfn;
+	region->end_pfn = end_pfn;
+	list_add_tail(&region->list, &nosave_regions);
+ Report:

Quoted 8 lines

kernel/power/snapshot.c static unsigned long memory_bm_next_pfn( · line 635

Quoted 3 lines

kernel/power/snapshot.c +3 -0

+	printk("swsusp: Registered nosave memory region: %016lx - %016lx\n",
+		start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
+}

I wonder why you reimplemented this and put it in snapshot.c, rather than use my version which was nicely in its own file, had appropriate locking, etc.?

Well, the locking is not necessary and we only need a list for that. Also,

mark_nosave_pages() refers to a function that's invisible outside snapshot.c and I didn't think it was a good idea to separate mark_nosave_pages() from register_nosave_region().

Quoted 59 lines · Rafael J. Wysocki

Quoted 55 lines · Nick Piggin

Quoted 37 lines · Rafael J. Wysocki

}

/**
+ * This structure represents a range of page frames the contents of which
+ * should not be saved during the suspend.
+ */

kernel/power/snapshot.c +31 -0

+
+struct nosave_region {
+	struct list_head list;
+	unsigned long start_pfn;
+	unsigned long end_pfn;
+};
+
+static LIST_HEAD(nosave_regions);
+
+/**
+ *	register_nosave_region - register a range of page frames the contents
+ *	of which should not be saved during the suspend (to be used in the early
+ *	initializatoion code)
+ */
+
+void __init
+register_nosave_region(unsigned long start_pfn, unsigned long end_pfn)
+{
+	struct nosave_region *region;
+
+	if (start_pfn >= end_pfn)
+		return;
+
+	if (!list_empty(&nosave_regions)) {
+		/* Try to extend the previous region (they should be sorted) */
+		region = list_entry(nosave_regions.prev,
+					struct nosave_region, list);
+		if (region->end_pfn == start_pfn) {
+			region->end_pfn = end_pfn;
+			goto Report;
+		}

kernel/power/snapshot.c static unsigned long memory_bm_next_pfn( · line 635

Quoted 10 lines

kernel/power/snapshot.c +10 -0

+	}
+	/* This allocation cannot fail */
+	region = alloc_bootmem_low(sizeof(struct nosave_region));
+	region->start_pfn = start_pfn;
+	region->end_pfn = end_pfn;
+	list_add_tail(&region->list, &nosave_regions);
+ Report:
+	printk("swsusp: Registered nosave memory region: %016lx - %016lx\n",
+		start_pfn << PAGE_SHIFT, end_pfn << PAGE_SHIFT);
+}

I wonder why you reimplemented this and put it in snapshot.c, rather than use my version which was nicely in its own file, had appropriate locking, etc.?

Quoted 1 line

Well, the locking is not necessary and we only need a list for that. Also,

I wouldn't say that. You're creating an interface here that is going to be used outside swsusp. Users of that interface may not need locking now, but that could cause problems down the line.

Sure you don't _need_ an rbtree, but our implementation makes it so simple that there isn't much downside.

Quoted 3 lines

mark_nosave_pages() refers to a function that's invisible outside snapshot.c and I didn't think it was a good idea to separate mark_nosave_pages() from register_nosave_region().

But that's because you even use mark_nosave_pages in your implementation. Mine uses the nosave regions directly.

Quoted 3 lines · Nick Piggin

I wouldn't say that. You're creating an interface here that is going to be used outside swsusp. Users of that interface may not need locking now, but that could cause problems down the line.

I think we can add the locking when it's necessary. For now, IMHO, it could be confusing to someone who doesn't know the locking is not needed.

Quoted 2 lines

Sure you don't _need_ an rbtree, but our implementation makes it so simple that there isn't much downside.

Not much, but the code is more complicated.

Quoted 6 lines

Quoted 3 lines

mark_nosave_pages() refers to a function that's invisible outside snapshot.c and I didn't think it was a good idea to separate mark_nosave_pages() from register_nosave_region().

But that's because you even use mark_nosave_pages in your implementation. Mine uses the nosave regions directly.

Well, I think we need two bits per page anyway, to mark free pages and pages allocated by swsusp, so using the nosave regions directly won't save us much.

Quoted 8 lines · Rafael J. Wysocki

Quoted 3 lines · Nick Piggin

I wouldn't say that. You're creating an interface here that is going to be used outside swsusp. Users of that interface may not need locking now, but that could cause problems down the line.

Quoted 2 lines

I think we can add the locking when it's necessary. For now, IMHO, it could be confusing to someone who doesn't know the locking is not needed.

I don't know why it would confuse them. We just define the API to guarantee the correct locking, and that means the locking _is_ needed.

You don't have to care what the callers are doing. That's the beauty of a sane API.

Quoted 5 lines

Quoted 2 lines

Sure you don't _need_ an rbtree, but our implementation makes it so simple that there isn't much downside.

Not much, but the code is more complicated.

But it's in its own file and has a contained API, so it is very easy to review, test and verify.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

mark_nosave_pages() refers to a function that's invisible outside snapshot.c and I didn't think it was a good idea to separate mark_nosave_pages() from register_nosave_region().

But that's because you even use mark_nosave_pages in your implementation. Mine uses the nosave regions directly.

Quoted 3 lines

Well, I think we need two bits per page anyway, to mark free pages and pages allocated by swsusp, so using the nosave regions directly won't save us much.

Well I think it is a cleaner though.

Quoted 13 lines · Nick Piggin

Quoted 10 lines · Rafael J. Wysocki

Quoted 3 lines · Nick Piggin

I wouldn't say that. You're creating an interface here that is going to be used outside swsusp. Users of that interface may not need locking now, but that could cause problems down the line.

I think we can add the locking when it's necessary. For now, IMHO, it could be confusing to someone who doesn't know the locking is not needed.

Quoted 2 lines

I don't know why it would confuse them. We just define the API to guarantee the correct locking, and that means the locking _is_ needed.

Even if there are no users that actually need the locking and probably never will be?

For now, register_nosave_region() is to be called by architecture initialization code _only_ and there's no reason whatsoever why any architecture would need to call it concurrently from many places.

Quoted 2 lines

You don't have to care what the callers are doing. That's the beauty of a sane API.

Well, I don't think adding unneded infrastructure is a good thing.

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

But that's because you even use mark_nosave_pages in your implementation. Mine uses the nosave regions directly.

Quoted 5 lines

Quoted 3 lines

Well, I think we need two bits per page anyway, to mark free pages and pages allocated by swsusp, so using the nosave regions directly won't save us much.

Well I think it is a cleaner though.

This is a matter of opinion, too ...

Quoted 15 lines · Rafael J. Wysocki

Quoted 13 lines · Nick Piggin

Quoted 10 lines · Rafael J. Wysocki

Quoted 3 lines · Nick Piggin

I wouldn't say that. You're creating an interface here that is going to be used outside swsusp. Users of that interface may not need locking now, but that could cause problems down the line.

I think we can add the locking when it's necessary. For now, IMHO, it could be confusing to someone who doesn't know the locking is not needed.

Quoted 6 lines

Quoted 2 lines

I don't know why it would confuse them. We just define the API to guarantee the correct locking, and that means the locking _is_ needed.

Even if there are no users that actually need the locking and probably never will be?

Probably is the keyword.

Why would you *not* make this a sane API? Surely performance isn't the reason? Nor complexity.

Quoted 10 lines

For now, register_nosave_region() is to be called by architecture initialization code _only_ and there's no reason whatsoever why any architecture would need to call it concurrently from many places.

Quoted 2 lines

You don't have to care what the callers are doing. That's the beauty of a sane API.

Well, I don't think adding unneded infrastructure is a good thing.

But defining good APIs is a very good thing. And with my good API, the lock is not unneeded.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 2 lines

But that's because you even use mark_nosave_pages in your implementation. Mine uses the nosave regions directly.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Well, I think we need two bits per page anyway, to mark free pages and pages allocated by swsusp, so using the nosave regions directly won't save us much.

Well I think it is a cleaner though.

This is a matter of opinion, too ...

Well, as I'm not volunteering to maintain swsusp, if your opinion is that your way is cleaner, I can't argue ;) So long as it stops wasting those page flags then I'm happy.

However the register_nosave API really should use locking, I think. There is absolutely no downside, AFAIKS.

Okay, the next three messages contain patches that should do the trick.

They have been tested on x86_64, but not very thoroughly.

Quoted 3 lines · Rafael J. Wysocki

Okay, the next three messages contain patches that should do the trick.

They have been tested on x86_64, but not very thoroughly.

Looks nice, but I'm having some trouble with it. Solved too though :)

Thing is that I need to call register_nosave_region for a region reserved for the IOMMU. Because the region is reserved so early during boot I cannot call register_nosave_region at that time. However, I also can't call register_nosave_region during a late initcall because at that point bootmem can no longer be allocated. I could of course put a hook somewhere into the arch code to do the marking, but I'd prefer not to.

The easiest solution I came up with is below. Of course, the suspend patches for powerpc64 are still very much work in progress and I might end up changing the whole reservation scheme after some feedback... If nobody else needs this then don't think about it now.

However, would that patch be acceptable to you? What about error handling? Printing a message and setting a "suspend not permitted" variable would be great but I don't think such a variable exists.

Also, maybe passing in a gfp mask would be better (and we could use 0 to mean bootmem too, I'd think)

Actually... I'd never have noticed this if register_nosave_region merged regions. I have these two regions:

[    0.000000] swsusp: Registered nosave memory region: 0000000080000000 - 0000000100000000
[...]
[   19.406116] swsusp: Registered nosave memory region: 000000007f000000 - 0000000080000000

But they aren't merged, if they were the latter call wouldn't need to do any allocations. Not that I'd want to rely on these positions!

With this patch and appropriate changes to my suspend code, it works.

Quoted 4 lines

The easiest solution I came up with is below. Of course, the suspend patches for powerpc64 are still very much work in progress and I might end up changing the whole reservation scheme after some feedback... If nobody else needs this then don't think about it now.

Well, it may be needed for other things too.

I think we should pass a mask. BTW, can you please check if the appended patch is sufficient?

Quoted 1 line

With this patch and appropriate changes to my suspend code, it works.

OK, thanks for testing!

Quoted 6 lines · Rafael J. Wysocki

Quoted 4 lines

The easiest solution I came up with is below. Of course, the suspend patches for powerpc64 are still very much work in progress and I might end up changing the whole reservation scheme after some feedback... If nobody else needs this then don't think about it now.

Well, it may be needed for other things too.

Yeah, but it's probably better to wait for them :)

Quoted 2 lines

I think we should pass a mask. BTW, can you please check if the appended patch is sufficient?

Unfortunately I won't be able to actually try this on hardware until the 20th or so.

Quoted 3 lines

Quoted 1 line

With this patch and appropriate changes to my suspend code, it works.

OK, thanks for testing!

Forgot to mention, patches are at
http://johannes.sipsolutions.net/patches/ look for the latest powerpc-suspend-* patchset.

Quoted 8 lines

kernel/power/snapshot.c +8 -0

+	if (system_state == SYSTEM_BOOTING) {
+		/* This allocation cannot fail */
+		region = alloc_bootmem_low(sizeof(struct nosave_region));
+	} else {
+		region = kzalloc(sizeof(struct nosave_region), GFP_ATOMIC);
+		if (!region) {
+			printk(KERN_WARNING "swsusp: Not enough memory "
+				"to register a nosave region!\n");

kernel/power/snapshot.c register_nosave_region(unsigned long sta · line 638

Quoted 4 lines

kernel/power/snapshot.c +4 -0

+			WARN_ON(1);
+			return;
+		}
+	}

I don't think that'll be sufficient, system_state = SYSTEM_BOOTING is done only in init/main.c:init_post which is done after after calling the initcalls (they are called in do_basic_setup)

Quoted 2 lines

Unfortunately I won't be able to actually try this on hardware until the 20th or so.

OK, it's not an urgent thing. ;-)

Well, I don't think so. If I understand the definition of system_state correctly, it is initially equal to SYSTEM_BOOTING. Then, it's changed to SYSTEM_RUNNING in init/main.c after the bootmem has been freed.

Anyway, the patch works on x86_64. :-)

Quoted 4 lines · Rafael J. Wysocki

Quoted 2 lines

Unfortunately I won't be able to actually try this on hardware until the 20th or so.

OK, it's not an urgent thing. ;-)

True :)

Quoted 3 lines

Well, I don't think so. If I understand the definition of system_state correctly, it is initially equal to SYSTEM_BOOTING. Then, it's changed to SYSTEM_RUNNING in init/main.c after the bootmem has been freed.

No, I think you're confusing bootmem with initmem right now.

If you actually look at the code then free_all_bootmem is called as part of mem_init() on powerpc, which is called from start_kernel() a long time before initcalls are done and system state is set.

Put it this way. By the time initcalls are done, I can no longer use bootmem. I tested this and it panics. But if you look at the code in init/main.c, system_state is only changed after initcalls are done.

In that case your patch seems to be the simplest one and I think it should go along with some code that will actually use it.

That's a no-no. ATOMIC alocations can fail, and no, WARN_ON is not enough. It is not a bug, they just fail.

Quoted 2 lines

That's a no-no. ATOMIC alocations can fail, and no, WARN_ON is not enough. It is not a bug, they just fail.

But like I said in my post, there's no way we can disable suspend to disk when they do, right now anyway. Also, this can't be called any later than a late initcall or such since it's __init, and thus there shouldn't be memory pressure yet that would cause this to fail.

In any case, I'd be much happier with having a "disable suspend" variable so we could print a big warning and set that flag.

Quoted 2 lines · Pavel Machek

That's a no-no. ATOMIC alocations can fail, and no, WARN_ON is not enough. It is not a bug, they just fail.

But like I said in my post, there's no way we can disable suspend to disk when they do, right now anyway. Also, this can't be called any later than a late initcall or such since it's __init, and thus there shouldn't be memory pressure yet that would cause this to fail.

In any case, I'd be much happier with having a "disable suspend" variable so we could print a big warning and set that flag.

Quoted 7 lines

Quoted 2 lines

That's a no-no. ATOMIC alocations can fail, and no, WARN_ON is not enough. It is not a bug, they just fail.

But like I said in my post, there's no way we can disable suspend to disk when they do, right now anyway. Also, this can't be called any later than a late initcall or such since it's __init, and thus there shouldn't be memory pressure yet that would cause this to fail.

Exactly. If an atomic allocation fails at this stage, there is a bug IMHO (although not necessarily in our code).

Okay, the next three messages contain patches that should do the trick.

They have been tested on x86_64, but not very thoroughly.

Quoted 4 lines

Okay, the next three messages contain patches that should do the trick.

They have been tested on x86_64, but not very thoroughly.

Works on my powerbook as well. Never mind that usb is broken again with suspend to disk. And my own patches break both str and std right now.

But these (on top of wireless-dev which is currently about 2.6.21-rc2) work fine as long as I assume they don't break usb ;)

Quoted 2 lines

Works on my powerbook as well. Never mind that usb is broken again with suspend to disk. And my own patches break both str and std right now.

Ouch.

Quoted 2 lines

But these (on top of wireless-dev which is currently about 2.6.21-rc2) work fine as long as I assume they don't break usb ;)

Well, on my boxes they don't. ;-)

Okay, the next three messages contain patches that should do the trick.

They have been tested on x86_64, but not very thoroughly.

Quoted 3 lines · Rafael J. Wysocki

Okay, the next three messages contain patches that should do the trick.

They have been tested on x86_64, but not very thoroughly.

They look good to me, but what do I know about swsusp :-) I'll stick them in my laptop's kernel (boring i386-UP) and see what happens.

I did notice you don't stick KERN_ prio markers in your printk()s not sure what to think of that (or if its common practise and I'm just not aware of it).

Thanks for doing this.

Let's make sure that no more backdoor page flags get allocated without going through the linux-mm list to work out whether we really need it or can live without it...

I need one bit for lockless pagecache ;)

Quoted 3 lines · Nick Piggin

Let's make sure that no more backdoor page flags get allocated without going through the linux-mm list to work out whether we really need it or can live without it...

Quoted 1 line · Nick Piggin

I need one bit for lockless pagecache ;)

Is that still your PageNoNewRefs thing?

What was wrong with my atomic_cmpxchg suggestion?

Quoted 8 lines · Rafael J. Wysocki

Quoted 6 lines · Christoph Lameter

Quoted 2 lines · Pavel Machek

I... actually do not like that patch. It adds code... at little or no benefit.

Quoted 8 lines

Quoted 3 lines

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

As I have already said for a couple of times, I think we can and I'm going to do it, but right now I'm a bit busy with other things that I consider as more urgent.

I need one bit for lockless pagecache ;)

Anyway, I guess if you want something done you have to do it yourself.

This patch still needs work (and I don't know if it even works, because I can't make swsusp resume even on a vanilla kernel).

But this is my WIP for removing swsusp page flags.

This patch adds a simple extent based nosave region tracker, and rearranges some of the snapshot code to be a bit simpler and more amenable to having dynamically allocated flags (they aren't actually dynamically allocated in this patch, however).

Quoted 4 lines

Anyway, I guess if you want something done you have to do it yourself.

This patch still needs work (and I don't know if it even works, because I can't make swsusp resume even on a vanilla kernel).

That's interesting, BTW, because recently I've been having problems with finding a machine on which it doesn't work. ;-) If you could tell me (in private) what the problems are, I'd try to help.

Quoted 2 lines

I... actually do not like that patch. It adds code... at little or no benefit.

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

Hmm, can't we just add another word to struct page?

Plus we really need PageNosave from boot on...

Quoted 1 line · Pavel Machek

Hmm, can't we just add another word to struct page?

Quoted 1 line

Plus we really need PageNosave from boot on...

Well it would be great to get the story straight. First I was told that the bitmaps can be allocated later. Now we dont. The current patch should do what you want.

Quoted 3 lines · Christoph Lameter

Quoted 1 line · Pavel Machek

Hmm, can't we just add another word to struct page?

?

Quoted 1 line

Quoted 1 line

Plus we really need PageNosave from boot on...

Quoted 4 lines

Well it would be great to get the story straight. First I was told that the bitmaps can be allocated later. Now we dont. The current patch should do what you want.

PageNosave is set by boot code in some cases, as Rafael told you.

It may be possible to redo boot processing at suspend time, but it would be ugly can of worms, I'm not sure if BIOS data are still available, and neccessary functions are probably __init.

Hi!

Quoted 4 lines

Quoted 2 lines

I... actually do not like that patch. It adds code... at little or no benefit.

We are looking into saving page flags since we are running out. The two page flags used by software suspend are rarely needed and should be taken out of the flags. If you can do it a different way then please do.

Hmm, can't we just add another word to struct page?

Quoted 3 lines

I think we can just move the flags completely into the kernel/power directory? This centralizes all your handling of pageflags into snapshot.c so that you need no external definitions anymore.

Yes, I think we can do it this way, but can we generally assume that the offset for eg. test_bit() won't be taken modulo 32 (or 64)?

And ...