From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Rafael J. Wysocki" Subject: Re: [RFC][PATCH 2/3] swsusp: Do not use page flags Date: Tue, 13 Mar 2007 11:17:42 +0100 Message-ID: <200703131117.43818.rjw@sisk.pl> References: <200703131016.12935.rjw@sisk.pl> <45F66D9B.7000301@yahoo.com.au> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <45F66D9B.7000301@yahoo.com.au> Content-Disposition: inline List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-pm-bounces@lists.osdl.org Errors-To: linux-pm-bounces@lists.osdl.org To: Nick Piggin Cc: linux-mm@kvack.org, Peter Zijlstra , Pavel Machek , pm list , Johannes Berg , Christoph Lameter List-Id: linux-pm@vger.kernel.org On Tuesday, 13 March 2007 10:23, Nick Piggin wrote: > Rafael J. Wysocki wrote: > > On Tuesday, 13 March 2007 05:47, Nick Piggin wrote: > > = > >>Rafael J. Wysocki wrote: > >> > >> > >>> } > >>> = > >>> /** > >>>+ * This structure represents a range of page frames the contents of w= hich > >>>+ * 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 conte= nts > >>>+ * 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 >=3D end_pfn) > >>>+ return; > >>>+ > >>>+ if (!list_empty(&nosave_regions)) { > >>>+ /* Try to extend the previous region (they should be sorted) */ > >>>+ region =3D list_entry(nosave_regions.prev, > >>>+ struct nosave_region, list); > >>>+ if (region->end_pfn =3D=3D start_pfn) { > >>>+ region->end_pfn =3D end_pfn; > >>>+ goto Report; > >>>+ } > >>>+ } > >>>+ /* This allocation cannot fail */ > >>>+ region =3D alloc_bootmem_low(sizeof(struct nosave_region)); > >>>+ region->start_pfn =3D start_pfn; > >>>+ region->end_pfn =3D end_pfn; > >>>+ list_add_tail(®ion->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 th= an > >>use my version which was nicely in its own file, had appropriate lockin= g, > >>etc.? > > = > > = > > Well, the locking is not necessary and we only need a list for that. A= lso, > = > 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 coul= d 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. > > mark_nosave_pages() refers to a function that's invisible outside snaps= hot.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 tha= t. I just wanted to avoid making too many changes at a time in this particular patch series. Greetings, Rafael