[swsusp] Rework image freeing

[swsusp] Rework image freeing

[Pavel Machek]

Do not store pagedirs in swap twice. This needed rewrite of image
freeing, but it enabled me nice cleanups in the end.

Signed-off-by: Pavel Machek <pavel@suse.cz>

---
commit 67434821951d6f10d55e29465a24e7f5015038f1
tree ee0f4a209e8b680ffdfa6e7837a3a248b524f421
parent 7bdc8fc378f053bd4eb4210beb1d494485318512
author <pavel@amd.(none)> Tue, 20 Sep 2005 15:34:55 +0200
committer <pavel@amd.(none)> Tue, 20 Sep 2005 15:34:55 +0200

 kernel/power/swsusp.c |  103 ++++++++++++++++++++++---------------------------
 1 files changed, 46 insertions(+), 57 deletions(-)

diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
--- a/kernel/power/swsusp.c
+++ b/kernel/power/swsusp.c
@@ -729,16 +729,6 @@ static void copy_data_pages(void)
 	BUG_ON(pbe);
 }
 
-
-/**
- *	calc_nr - Determine the number of pages needed for a pbe list.
- */
-
-static int calc_nr(int nr_copy)
-{
-	return nr_copy + (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1);
-}
-
 /**
  *	free_pagedir - free pages allocated with alloc_pagedir()
  */
@@ -749,6 +739,8 @@ static inline void free_pagedir(struct p
 
 	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;
 	}
@@ -794,6 +786,16 @@ static void create_pbe_list(struct pbe *
 	pr_debug("create_pbe_list(): initialized %d PBEs\n", num);
 }
 
+static long alloc_image_page(void)
+{
+	long res = 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.
  *
@@ -807,7 +809,7 @@ static void create_pbe_list(struct pbe *
  *	On each page we set up a list of struct_pbe elements.
  */
 
-static struct pbe * alloc_pagedir(unsigned nr_pages)
+static struct pbe *alloc_pagedir(unsigned nr_pages)
 {
 	unsigned num;
 	struct pbe *pblist, *pbe;
@@ -816,11 +818,11 @@ static struct pbe * alloc_pagedir(unsign
 		return NULL;
 
 	pr_debug("alloc_pagedir(): nr_pages = %d\n", nr_pages);
-	pblist = (struct pbe *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
+	pblist = (struct pbe *) alloc_image_page();
 	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 *)get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
+		pbe->next = (struct pbe *) alloc_image_page();
 	}
 	if (!pbe) { /* get_zeroed_page() failed */
 		free_pagedir(pblist);
@@ -829,48 +831,27 @@ static struct pbe * alloc_pagedir(unsign
 	return pblist;
 }
 
-/**
- *	free_image_pages - Free pages allocated for snapshot
+/* Free pages we allocated for suspend. Suspend pages are alocated
+ * before atomic copy, so we need to free them after resume.
  */
-
-static void free_image_pages(void)
+void swsusp_free(void)
 {
-	struct pbe * p;
+	struct zone *zone;
+	unsigned long zone_pfn;
 
-	for_each_pbe (p, pagedir_save) {
-		if (p->address) {
-			ClearPageNosave(virt_to_page(p->address));
-			free_page(p->address);
-			p->address = 0;
+	for_each_zone(zone) {
+		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_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));
+			}
 		}
 	}
 }
 
-/**
- *	alloc_image_pages - Allocate pages for the snapshot.
- */
-
-static int alloc_image_pages(void)
-{
-	struct pbe * p;
-
-	for_each_pbe (p, pagedir_save) {
-		p->address = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
-		if (!p->address)
-			return -ENOMEM;
-		SetPageNosave(virt_to_page(p->address));
-	}
-	return 0;
-}
-
-void swsusp_free(void)
-{
-	BUG_ON(PageNosave(virt_to_page(pagedir_save)));
-	BUG_ON(PageNosaveFree(virt_to_page(pagedir_save)));
-	free_image_pages();
-	free_pagedir(pagedir_save);
-}
-
 
 /**
  *	enough_free_mem - Make sure we enough free memory to snapshot.
@@ -914,19 +895,23 @@ static int enough_swap(void)
 
 static int swsusp_alloc(void)
 {
-	int error;
+	struct pbe *p;
 
 	pagedir_nosave = NULL;
-	nr_copy_pages = calc_nr(nr_copy_pages);
 
 	pr_debug("suspend: (pages needed: %d + %d free: %d)\n",
 		 nr_copy_pages, PAGES_FOR_IO, nr_free_pages());
 
-	if (!enough_free_mem())
+	if (!enough_free_mem()) {
+		printk(KERN_ERR "swsusp: Not enough free memory\n");
 		return -ENOMEM;
+	}
 
-	if (!enough_swap())
+	if (!enough_swap()) {
+		printk(KERN_ERR "swsusp: Not enough free swap\n");
 		return -ENOSPC;
+	}
+		
 
 	if (!(pagedir_save = alloc_pagedir(nr_copy_pages))) {
 		printk(KERN_ERR "suspend: Allocating pagedir failed.\n");
@@ -934,10 +919,14 @@ static int swsusp_alloc(void)
 	}
 	create_pbe_list(pagedir_save, nr_copy_pages);
 	pagedir_nosave = pagedir_save;
-	if ((error = alloc_image_pages())) {
-		printk(KERN_ERR "suspend: Allocating image pages failed.\n");
-		swsusp_free();
-		return error;
+
+	for_each_pbe (p, pagedir_save) {
+		p->address = alloc_image_page();
+		if (!p->address) {
+			printk(KERN_ERR "suspend: Allocating image pages failed.\n");
+			swsusp_free();
+			return -ENOMEM;
+		}
 	}
 
 	nr_copy_pages_check = nr_copy_pages;
@@ -1037,7 +1026,7 @@ 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);
+	BUG_ON (!error && (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

Re: [swsusp] Rework image freeing

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> +	for_each_zone(zone) {
>  +		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_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));
>  +			}

There doesn't seem to be much point in converting the pageframe address to
a virtual address, then back to a pageframe address.  Why not just do
put_page() here?

Re: [swsusp] Rework image freeing

[Dave Hansen]

On Wed, 2005-09-21 at 22:51 +0200, Pavel Machek wrote:
> +void swsusp_free(void)
...
> +       for_each_zone(zone) {
> +               for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_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));
> +                       }
>                 }
>         }

This won't work with discontiguous page ranges.  Due to sparsemem you
can run into pages in the middle of a zone where pfn_to_page() doesn't
work.  I'd suggest adding a pfn_valid() check in there.  

-- Dave

Re: [swsusp] Rework image freeing

[Dave Hansen]

On Wed, 2005-09-21 at 22:51 +0200, Pavel Machek wrote:
> +static long alloc_image_page(void)
> +{
> +       long res = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
> +       if (res) {
> +               SetPageNosave(virt_to_page(res));
> +               SetPageNosaveFree(virt_to_page(res));
> +       }
> +       return res;
> +}

Please avoid using longs here.  "res" really is a virtual address, and
it would be polite to keep it in a pointer of some kind.  Returning
void* would also avoid the two casts in alloc_pagedir().  The same
probably goes for pbe->address and pbe->orig_address.

BTW, I think get_zeroed_page() returns a long to keep people from
confusing it with the allocator routines that return actual 'struct page
*', and not the page's virtual address.  So, you really should be
casting them to real pointers as soon as it comes back from
get_zeroed_page() and cousins.

-- Dave

Re: [swsusp] Rework image freeing

[Pavel Machek]

Hi!

> >
> > +	for_each_zone(zone) {
> >  +		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_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));
> >  +			}
> 
> There doesn't seem to be much point in converting the pageframe address to
> a virtual address, then back to a pageframe address.  Why not just do
> put_page() here?

I do not know what put_page does, and free_page() has some friendly
BUG_ONs. But if I should just do put_page(page); instead of
free_page((long) page_address(page)), that is okay with me.

[Sound of harddrive seeking and CPU fan going up as test kernel
compiles].

								Pavel

-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [swsusp] Rework image freeing

[Pavel Machek]

Hi!

> > +static long alloc_image_page(void)
> > +{
> > +       long res = get_zeroed_page(GFP_ATOMIC | __GFP_COLD);
> > +       if (res) {
> > +               SetPageNosave(virt_to_page(res));
> > +               SetPageNosaveFree(virt_to_page(res));
> > +       }
> > +       return res;
> > +}
> 
> Please avoid using longs here.  "res" really is a virtual address, and
> it would be polite to keep it in a pointer of some kind.  Returning
> void* would also avoid the two casts in alloc_pagedir().  The same
> probably goes for pbe->address and pbe->orig_address.
> 
> BTW, I think get_zeroed_page() returns a long to keep people from
> confusing it with the allocator routines that return actual 'struct page
> *', and not the page's virtual address.  So, you really should be
> casting them to real pointers as soon as it comes back from
> get_zeroed_page() and cousins.

Okay, but that's another patch, as I'll have to do s/long/void */ on
whole file. address/orig_address is used at lot of places.

								Pavel

-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [swsusp] Rework image freeing

[Andrew Morton]

Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
> 
> > >
> > > +	for_each_zone(zone) {
> > >  +		for (zone_pfn = 0; zone_pfn < zone->spanned_pages; ++zone_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));
> > >  +			}
> > 
> > There doesn't seem to be much point in converting the pageframe address to
> > a virtual address, then back to a pageframe address.  Why not just do
> > put_page() here?
> 
> I do not know what put_page does, and free_page() has some friendly
> BUG_ONs. But if I should just do put_page(page); instead of
> free_page((long) page_address(page)), that is okay with me.

I don't know what alloc_pages() and free_page[s]() are supposed to do either
really.  They date from when Linus was in diapers.  They talk in terms of
kernel virtual addresses and will explode loudly if used for highmem pages.

All modern code will use page*'s or pfn's.

> [Sound of harddrive seeking and CPU fan going up as test kernel
> compiles].
> 

Ah, I wondered what it was.

Re: [swsusp] Rework image freeing

[Rafael J. Wysocki]

Hi,

On Wednesday, 21 of September 2005 22:51, Pavel Machek wrote:
> Do not store pagedirs in swap twice. This needed rewrite of image
> freeing, but it enabled me nice cleanups in the end.
> 
> Signed-off-by: Pavel Machek <pavel@suse.cz>
> 
> ---
> commit 67434821951d6f10d55e29465a24e7f5015038f1
> tree ee0f4a209e8b680ffdfa6e7837a3a248b524f421
> parent 7bdc8fc378f053bd4eb4210beb1d494485318512
> author <pavel@amd.(none)> Tue, 20 Sep 2005 15:34:55 +0200
> committer <pavel@amd.(none)> Tue, 20 Sep 2005 15:34:55 +0200
> 
>  kernel/power/swsusp.c |  103 ++++++++++++++++++++++---------------------------
>  1 files changed, 46 insertions(+), 57 deletions(-)
> 
> diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
> --- a/kernel/power/swsusp.c
> +++ b/kernel/power/swsusp.c
> @@ -729,16 +729,6 @@ static void copy_data_pages(void)
>  	BUG_ON(pbe);
>  }
>  
> -
> -/**
> - *	calc_nr - Determine the number of pages needed for a pbe list.
> - */
> -
> -static int calc_nr(int nr_copy)
> -{
> -	return nr_copy + (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1);
> -}

I can't see why you are going to drop this function.  Isn't it necessary any more?

Greetings,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [swsusp] Rework image freeing

[Rafael J. Wysocki]

Hi,

On Thursday, 22 of September 2005 00:53, Rafael J. Wysocki wrote:
> Hi,
> 
> On Wednesday, 21 of September 2005 22:51, Pavel Machek wrote:
> > Do not store pagedirs in swap twice. This needed rewrite of image
> > freeing, but it enabled me nice cleanups in the end.
> > 
> > Signed-off-by: Pavel Machek <pavel@suse.cz>
> > 
> > ---
> > commit 67434821951d6f10d55e29465a24e7f5015038f1
> > tree ee0f4a209e8b680ffdfa6e7837a3a248b524f421
> > parent 7bdc8fc378f053bd4eb4210beb1d494485318512
> > author <pavel@amd.(none)> Tue, 20 Sep 2005 15:34:55 +0200
> > committer <pavel@amd.(none)> Tue, 20 Sep 2005 15:34:55 +0200
> > 
> >  kernel/power/swsusp.c |  103 ++++++++++++++++++++++---------------------------
> >  1 files changed, 46 insertions(+), 57 deletions(-)
> > 
> > diff --git a/kernel/power/swsusp.c b/kernel/power/swsusp.c
> > --- a/kernel/power/swsusp.c
> > +++ b/kernel/power/swsusp.c
> > @@ -729,16 +729,6 @@ static void copy_data_pages(void)
> >  	BUG_ON(pbe);
> >  }
> >  
> > -
> > -/**
> > - *	calc_nr - Determine the number of pages needed for a pbe list.
> > - */
> > -
> > -static int calc_nr(int nr_copy)
> > -{
> > -	return nr_copy + (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1);
> > -}
> 
> I can't see why you are going to drop this function.  Isn't it necessary any more?

OK, swsusp fails anyway when it cannot allocate a page, so in fact it is not.
However, the information printed in swsusp_alloc():

>  /**
>   *	enough_free_mem - Make sure we enough free memory to snapshot.
> @@ -914,19 +895,23 @@ static int enough_swap(void)
>  
>  static int swsusp_alloc(void)
>  {
> -	int error;
> +	struct pbe *p;
>  
>  	pagedir_nosave = NULL;
> -	nr_copy_pages = calc_nr(nr_copy_pages);
>  
>  	pr_debug("suspend: (pages needed: %d + %d free: %d)\n",
>  		 nr_copy_pages, PAGES_FOR_IO, nr_free_pages());

now seems to be inaccurate, because we likely need more pages than
 nr_copy_pages + PAGES_FOR_IO.

Greetings,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [swsusp] Rework image freeing

[Pavel Machek]

Hi!

> > -
> > -/**
> > - *	calc_nr - Determine the number of pages needed for a pbe list.
> > - */
> > -
> > -static int calc_nr(int nr_copy)
> > -{
> > -	return nr_copy + (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1);
> > -}
> 
> I can't see why you are going to drop this function.  Isn't it necessary any more?

I've actually decreased on-disk memory requirements by... guess what:
(nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1) factor. I do not store two
copies of page directories any more.
								Pavel
-- 
if you have sharp zaurus hardware you don't need... you know my address

Re: [swsusp] Rework image freeing

[Rafael J. Wysocki]

Hi,

On Thursday, 22 of September 2005 11:46, Pavel Machek wrote:
> Hi!
> 
> > > -
> > > -/**
> > > - *	calc_nr - Determine the number of pages needed for a pbe list.
> > > - */
> > > -
> > > -static int calc_nr(int nr_copy)
> > > -{
> > > -	return nr_copy + (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1);
> > > -}
> > 
> > I can't see why you are going to drop this function.  Isn't it necessary any more?
> 
> I've actually decreased on-disk memory requirements by... guess what:
> (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1) factor. I do not store two
> copies of page directories any more.

On-disk - yes, but we still need to allocate RAM to create the "pagedir".
It takes ca (nr_copy_pages/(PBS_PER_PAGE-1) + !!(nr_copy_pages % (PBS_PER_PAGE-1))),
so we should include this number in the check for free RAM and in the message
in swsusp_alloc(), I think.

Besides, the checks for free RAM and swap could be moved to
suspend_prepare_image(), along with the accompanying printk()s,
so that we call swsusp_alloc() only after we have verified there should
be enough resources.

Greetings,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"

Re: [swsusp] Rework image freeing

[Rafael J. Wysocki]

On Friday, 23 of September 2005 22:52, Rafael J. Wysocki wrote:
> Hi,
> 
> On Thursday, 22 of September 2005 11:46, Pavel Machek wrote:
> > Hi!
> > 
> > > > -
> > > > -/**
> > > > - *	calc_nr - Determine the number of pages needed for a pbe list.
> > > > - */
> > > > -
> > > > -static int calc_nr(int nr_copy)
> > > > -{
> > > > -	return nr_copy + (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1);
> > > > -}
> > > 
> > > I can't see why you are going to drop this function.  Isn't it necessary any more?
> > 
> > I've actually decreased on-disk memory requirements by... guess what:
> > (nr_copy+PBES_PER_PAGE-2)/(PBES_PER_PAGE-1) factor. I do not store two
> > copies of page directories any more.
> 
> On-disk - yes, but we still need to allocate RAM to create the "pagedir".
> It takes ca (nr_copy_pages/(PBS_PER_PAGE-1) + !!(nr_copy_pages % (PBS_PER_PAGE-1))),

Sorry, this is not the right number, of course.  Should be
(nr_copy_pages/PBS_PER_PAGE + !!(nr_copy_pages % PBS_PER_PAGE))

Greetings,
Rafael


-- 
- Would you tell me, please, which way I ought to go from here?
- That depends a good deal on where you want to get to.
		-- Lewis Carroll "Alice's Adventures in Wonderland"