Re: [RFC][PATCH 2/3] swsusp: Do not use page flags

On Tuesday, 13 March 2007 10:23, Nick Piggin wrote:

Rafael J. Wysocki wrote:

quoted

On Tuesday, 13 March 2007 05:47, Nick Piggin wrote:

quoted

Rafael J. Wysocki wrote:

quoted

}

/**
+ *	This structure represents a range of page frames the contents of which
+ *	should not be saved during the suspend.
+ */
+
+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:
+	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,

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.

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

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.

Still, for now the bits are not optimally used, but I'm going to change that.
I just wanted to avoid making too many changes at a time in this particular
patch series.

Greetings,
Rafael