swsusp cleanups
Subsystems (4)
cover swsusp cleanups
Hi,
The following patches contain some small clean up some functions in kernel/power/snapshot.c and mm/page_alloc.c.
Greetings,
Rafael
PATCH 1/3 swsusp: Fix mark_free_pages
Clean up mm/page_alloc.c#mark_free_pages() and make it avoid clearing PageNosaveFree for PageNosave pages. This allows us to get rid of an ugly hack in kernel/power/snapshot.c#copy_data_pages().
Additionally, the page-copying loop in copy_data_pages() is moved to an inline function.
--- kernel/power/snapshot.c | 27 +++++++++++++-------------- mm/page_alloc.c | 22 ++++++++++++++-------- 2 files changed, 27 insertions(+), 22 deletions(-)
mm/page_alloc.c +14 -8
@@ -703,7 +703,8 @@ static void __drain_pages(unsigned int cvoid mark_free_pages (struct zone *zone) { -unsigned long zone_pfn, flags; +unsigned long pfn, max_zone_pfn; +unsigned long flags;int order;struct list_head *curr;
@@ -711,18 +712,23 @@ void mark_free_pages(struct zone *zone)return ;spin_lock_irqsave (&zone->lock, flags); -for (zone_pfn =0 ; zone_pfn < zone->spanned_pages; ++zone_pfn) -ClearPageNosaveFree (pfn_to_page (zone_pfn + zone->zone_start_pfn)); + max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages; +for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; 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 start_pfn, i; +unsigned long i; - start_pfn = page_to_pfn(list_entry(curr, struct page, lru)); + pfn = page_to_pfn(list_entry(curr, struct page, lru)); +for (i =0 ; i < (1UL << order); i++) +SetPageNosaveFree (pfn_to_page (pfn + i)); + } -for (i=0 ; i < (1 <<order); i++) -SetPageNosaveFree (pfn_to_page (start_pfn+i)); - }spin_unlock_irqrestore (&zone->lock, flags); }
kernel/power/snapshot.c +13 -14
@@ -208,37 +208,36 @@ unsigned int count_data_pages(void)return n; } +static inline void copy_data_page (long *dst,long *src) +{ +int n; + +/* copy_page and memcpy are not usable for copying task structs. */ +for (n =PAGE_SIZE /sizeof (long ); n; n--) + *dst++ = *src++; +} +static void copy_data_pages (struct pbe *pblist) {struct zone *zone;unsigned long pfn, max_zone_pfn; -struct pbe *pbe, *p; +struct pbe *pbe; pbe = pblist;for_each_zone (zone) {if (is_highmem (zone))continue ;mark_free_pages (zone); -/* This is necessary for swsusp_free() */ - for_each_pb_page (p, pblist) -SetPageNosaveFree (virt_to_page (p)); - for_each_pbe (p, pblist) -SetPageNosaveFree (virt_to_page (p->address)); max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages;for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {struct page *page =saveable_page (pfn);if (page) { -long *src, *dst; -int n; +void *ptr =page_address (page);;BUG_ON (!pbe); - pbe->orig_address = (unsigned long )page_address (page); -/* copy_page and memcpy are not usable for copying task structs. */ - dst = (long *)pbe->address; - src = (long *)pbe->orig_address; -for (n =PAGE_SIZE /sizeof (long ); n; n--) - *dst++ = *src++; +copy_data_page ((void *)pbe->address, ptr); + pbe->orig_address = (unsigned long )ptr; pbe = pbe->next; } }
Re: [PATCH 1/3] swsusp: Fix mark_free_pages
mm/page_alloc.c void mark_free_pages(struct zone *zone) · line 722
Quoted 7 lines · Rafael J. Wysocki
mm/page_alloc.c +7
-0+ max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages; +for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) { +struct page *page =pfn_to_page (pfn); + +if (!PageNosave (page)) +ClearPageNosaveFree (page); + }
This is certainly not the only place in swsusp where there is a bug like this, but it is not correct to assume that each page inside of a zone is valid. With sparsemem, you need to do pfn_valid() on each pfn before calling pfn_to_page().
Something like:
max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages;
for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {
struct page *page;
if (!pfn_valid(pfn))
continue;
page = pfn_to_page(pfn);
if (!PageNosave(page))
ClearPageNosaveFree(page);
} -- Dave
Re: [PATCH 1/3] swsusp: Fix mark_free_pages
Quoted 11 lines · Dave Hansen
Review
Quoted 7 lines · Rafael J. Wysocki
(quoted diff) +7
-0+ max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages; + for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) { + struct page *page = pfn_to_page(pfn); + + if (!PageNosave(page)) + ClearPageNosaveFree(page); + }This is certainly not the only place in swsusp where there is a bug like this,
Well, I don't think so. They have been hunted for and dealt whith, except for this one. ;-)
Quoted 2 lines
but it is not correct to assume that each page inside of a zone is valid.
Obviously, I just forgot about it.
---
Clean up mm/page_alloc.c#mark_free_pages() and make it avoid clearing PageNosaveFree for PageNosave pages. This allows us to get rid of an ugly hack in kernel/power/snapshot.c#copy_data_pages().
Additionally, the page-copying loop in copy_data_pages() is moved to an inline function.
--- kernel/power/snapshot.c | 27 +++++++++++++-------------- mm/page_alloc.c | 24 ++++++++++++++++-------- 2 files changed, 29 insertions(+), 22 deletions(-)
mm/page_alloc.c +16 -8
@@ -703,7 +703,8 @@ static void __drain_pages(unsigned int cvoid mark_free_pages (struct zone *zone) { -unsigned long zone_pfn, flags; +unsigned long pfn, max_zone_pfn; +unsigned long flags;int order;struct list_head *curr;
@@ -711,18 +712,25 @@ void mark_free_pages(struct zone *zone)return ;spin_lock_irqsave (&zone->lock, flags); -for (zone_pfn =0 ; zone_pfn < zone->spanned_pages; ++zone_pfn) -ClearPageNosaveFree (pfn_to_page (zone_pfn + zone->zone_start_pfn)); + + 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 start_pfn, i; +unsigned long i; - start_pfn = page_to_pfn(list_entry(curr, struct page, lru)); + pfn = page_to_pfn(list_entry(curr, struct page, lru)); +for (i =0 ; i < (1UL << order); i++) +SetPageNosaveFree (pfn_to_page (pfn + i)); + } -for (i=0 ; i < (1 <<order); i++) -SetPageNosaveFree (pfn_to_page (start_pfn+i)); - }spin_unlock_irqrestore (&zone->lock, flags); }
kernel/power/snapshot.c +13 -14
@@ -208,37 +208,36 @@ unsigned int count_data_pages(void)return n; } +static inline void copy_data_page (long *dst,long *src) +{ +int n; + +/* copy_page and memcpy are not usable for copying task structs. */ +for (n =PAGE_SIZE /sizeof (long ); n; n--) + *dst++ = *src++; +} +static void copy_data_pages (struct pbe *pblist) {struct zone *zone;unsigned long pfn, max_zone_pfn; -struct pbe *pbe, *p; +struct pbe *pbe; pbe = pblist;for_each_zone (zone) {if (is_highmem (zone))continue ;mark_free_pages (zone); -/* This is necessary for swsusp_free() */ - for_each_pb_page (p, pblist) -SetPageNosaveFree (virt_to_page (p)); - for_each_pbe (p, pblist) -SetPageNosaveFree (virt_to_page (p->address)); max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages;for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {struct page *page =saveable_page (pfn);if (page) { -long *src, *dst; -int n; +void *ptr =page_address (page);;BUG_ON (!pbe); - pbe->orig_address = (unsigned long )page_address (page); -/* copy_page and memcpy are not usable for copying task structs. */ - dst = (long *)pbe->address; - src = (long *)pbe->orig_address; -for (n =PAGE_SIZE /sizeof (long ); n; n--) - *dst++ = *src++; +copy_data_page ((void *)pbe->address, ptr); + pbe->orig_address = (unsigned long )ptr; pbe = pbe->next; } }
Re: [PATCH 1/3] swsusp: Fix mark_free_pages
Hi!
Looks good to me (ACK).
kernel/power/snapshot.c unsigned int count_data_pages(void) · line 236
Quoted 4 lines
kernel/power/snapshot.c +1
-2if (page) { -long *src, *dst; -int n; +void *ptr =page_address (page);;
You probably want to remove one of ";"s.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Re: [PATCH 1/3] swsusp: Fix mark_free_pages
Quoted 10 lines · Pavel Machek
Hi!
Looks good to me (ACK).
Review
Quoted 4 lines
(quoted diff) +1
-2if (page) { - long *src, *dst; - int n; + void *ptr = page_address(page);;You probably want to remove one of ";"s.
Ouch.
I hope this is the final one.
---
Clean up mm/page_alloc.c#mark_free_pages() and make it avoid clearing PageNosaveFree for PageNosave pages. This allows us to get rid of an ugly hack in kernel/power/snapshot.c#copy_data_pages().
Additionally, the page-copying loop in copy_data_pages() is moved to an inline function.
--- kernel/power/snapshot.c | 27 +++++++++++++-------------- mm/page_alloc.c | 24 ++++++++++++++++-------- 2 files changed, 29 insertions(+), 22 deletions(-)
mm/page_alloc.c +16 -8
@@ -703,7 +703,8 @@ static void __drain_pages(unsigned int cvoid mark_free_pages (struct zone *zone) { -unsigned long zone_pfn, flags; +unsigned long pfn, max_zone_pfn; +unsigned long flags;int order;struct list_head *curr;
@@ -711,18 +712,25 @@ void mark_free_pages(struct zone *zone)return ;spin_lock_irqsave (&zone->lock, flags); -for (zone_pfn =0 ; zone_pfn < zone->spanned_pages; ++zone_pfn) -ClearPageNosaveFree (pfn_to_page (zone_pfn + zone->zone_start_pfn)); + + 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 start_pfn, i; +unsigned long i; - start_pfn = page_to_pfn(list_entry(curr, struct page, lru)); + pfn = page_to_pfn(list_entry(curr, struct page, lru)); +for (i =0 ; i < (1UL << order); i++) +SetPageNosaveFree (pfn_to_page (pfn + i)); + } -for (i=0 ; i < (1 <<order); i++) -SetPageNosaveFree (pfn_to_page (start_pfn+i)); - }spin_unlock_irqrestore (&zone->lock, flags); }
kernel/power/snapshot.c +13 -14
@@ -208,37 +208,36 @@ unsigned int count_data_pages(void)return n; } +static inline void copy_data_page (long *dst,long *src) +{ +int n; + +/* copy_page and memcpy are not usable for copying task structs. */ +for (n =PAGE_SIZE /sizeof (long ); n; n--) + *dst++ = *src++; +} +static void copy_data_pages (struct pbe *pblist) {struct zone *zone;unsigned long pfn, max_zone_pfn; -struct pbe *pbe, *p; +struct pbe *pbe; pbe = pblist;for_each_zone (zone) {if (is_highmem (zone))continue ;mark_free_pages (zone); -/* This is necessary for swsusp_free() */ - for_each_pb_page (p, pblist) -SetPageNosaveFree (virt_to_page (p)); - for_each_pbe (p, pblist) -SetPageNosaveFree (virt_to_page (p->address)); max_zone_pfn = zone->zone_start_pfn + zone->spanned_pages;for (pfn = zone->zone_start_pfn; pfn < max_zone_pfn; pfn++) {struct page *page =saveable_page (pfn);if (page) { -long *src, *dst; -int n; +void *ptr =page_address (page);BUG_ON (!pbe); - pbe->orig_address = (unsigned long )page_address (page); -/* copy_page and memcpy are not usable for copying task structs. */ - dst = (long *)pbe->address; - src = (long *)pbe->orig_address; -for (n =PAGE_SIZE /sizeof (long ); n; n--) - *dst++ = *src++; +copy_data_page ((void *)pbe->address, ptr); + pbe->orig_address = (unsigned long )ptr; pbe = pbe->next; } }
PATCH 2/3 swsusp: Reorder memory-allocating functions
In reply to Rafael J. WysockiMove some functions in kernel/power/snapshot.c to a better place (in the same file) and introduce free_image_page() (will be necessary in the future).
--- kernel/power/snapshot.c | 93 +++++++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 40 deletions(-)
kernel/power/snapshot.c +53 -40
@@ -156,6 +156,58 @@ static inline int save_highmem(void) {restatic inline int restore_highmem (void ) {return 0 ;}#endif +/** +* @safe_needed - on resume, for storing the PBE list and the image, +* we can only use memory pages that do not conflict with the pages +* used before suspend. +* +* The unsafe pages are marked with the PG_nosave_free flag +* and we count them using unsafe_pages +*/ + +static unsigned int unsafe_pages; + +static void *alloc_image_page (gfp_t gfp_mask,int safe_needed) +{ +void *res; + + res = (void *)get_zeroed_page (gfp_mask); +if (safe_needed) +while (res &&PageNosaveFree (virt_to_page (res))) { +/* The page is unsafe, mark it for swsusp_free() */ +SetPageNosave (virt_to_page (res)); + unsafe_pages++; + res = (void *)get_zeroed_page (gfp_mask); + } +if (res) { +SetPageNosave (virt_to_page (res)); +SetPageNosaveFree (virt_to_page (res)); + } +return res; +} + +unsigned long get_safe_page (gfp_t gfp_mask) +{ +return (unsigned long )alloc_image_page (gfp_mask,1 ); +} + +/** +* free_image_page - free page represented by @addr, allocated with +* alloc_image_page (page flags set by it must be cleared) +*/ + +static inline void free_image_page (void *addr,int clear_nosave_free) +{ +ClearPageNosave (virt_to_page (addr)); +if (clear_nosave_free) +ClearPageNosaveFree (virt_to_page (addr)); +free_page ((unsigned long )addr); +} + +/** +* pfn_is_nosave - check if given pfn is in the 'nosave' section +*/ +static inline int pfn_is_nosave (unsigned long pfn) {unsigned long nosave_begin_pfn =__pa (&__nosave_begin) >>PAGE_SHIFT ;
@@ -245,7 +297,6 @@ static void copy_data_pages(struct pbe *BUG_ON (pbe); } -/** * free_pagedir - free pages allocated with alloc_pagedir() */
@@ -256,10 +307,7 @@ static void free_pagedir(struct pbe *pblwhile (pblist) { pbe = (pblist +PB_PAGE_SKIP )->next; -ClearPageNosave (virt_to_page (pblist)); -if (clear_nosave_free) -ClearPageNosaveFree (virt_to_page (pblist)); -free_page ((unsigned long )pblist); +free_image_page (pblist, clear_nosave_free); pblist = pbe; } }
@@ -303,41 +351,6 @@ static inline void create_pbe_list(struc } } -static unsigned int unsafe_pages; - -/** -* @safe_needed - on resume, for storing the PBE list and the image, -* we can only use memory pages that do not conflict with the pages -* used before suspend. -* -* The unsafe pages are marked with the PG_nosave_free flag -* and we count them using unsafe_pages -*/ - -static void *alloc_image_page (gfp_t gfp_mask,int safe_needed) -{ -void *res; - - res = (void *)get_zeroed_page (gfp_mask); -if (safe_needed) -while (res &&PageNosaveFree (virt_to_page (res))) { -/* The page is unsafe, mark it for swsusp_free() */ -SetPageNosave (virt_to_page (res)); - unsafe_pages++; - res = (void *)get_zeroed_page (gfp_mask); - } -if (res) { -SetPageNosave (virt_to_page (res)); -SetPageNosaveFree (virt_to_page (res)); - } -return res; -} - -unsigned long get_safe_page (gfp_t gfp_mask) -{ -return (unsigned long )alloc_image_page (gfp_mask,1 ); -} -/** * alloc_pagedir - Allocate the page directory. *
Re: [PATCH 2/3] swsusp: Reorder memory-allocating functions
Quoted 3 lines · Rafael J. Wysocki
Move some functions in
kernel/power/snapshot.cto a better place (in the same file) and introducefree_image_page()(will be necessary in the future).
ACK.
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
PATCH 3/3 swsusp: Fix alloc_pagedir
In reply to Rafael J. WysockiGet rid of the FIXME in kernel/power/snapshot.c#alloc_pagedir() and simplify the functions called by it.
--- kernel/power/snapshot.c | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-)
kernel/power/snapshot.c +17 -15
@@ -316,12 +316,12 @@ static void free_pagedir(struct pbe *pbl* fill_pb_page - Create a list of PBEs on a given memory page */ -static inline void fill_pb_page (struct pbe *pbpage) +static inline void fill_pb_page (struct pbe *pbpage,unsigned int n) {struct pbe *p; p = pbpage; - pbpage +=PB_PAGE_SKIP ; + pbpage += n -1 ;do p->next = p +1 ;while (++p < pbpage);
@@ -330,24 +330,26 @@ static inline void fill_pb_page(struct p/** * create_pbe_list - Create a list of PBEs on top of a given chain * of memory pages allocated with alloc_pagedir() +* +* This function assumes that pages allocated by alloc_image_page() will +* always be zeroed. */ static inline void create_pbe_list (struct pbe *pblist,unsigned int nr_pages) { -struct pbe *pbpage, *p; +struct pbe *pbpage;unsigned int num =PBES_PER_PAGE ; for_each_pb_page (pbpage, pblist) {if (num >= nr_pages)break ; -fill_pb_page (pbpage); +fill_pb_page (pbpage,PBES_PER_PAGE ); 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 ; + num -=PBES_PER_PAGE ; +fill_pb_page (pbpage, nr_pages - num); } }
@@ -374,17 +376,17 @@ static struct pbe *alloc_pagedir(unsignereturn NULL ; pblist =alloc_image_page (gfp_mask, safe_needed); -/* FIXME: rewrite this ugly loop */ -for (pbe = pblist, num =PBES_PER_PAGE ; pbe && num < nr_pages; - pbe = pbe->next, num +=PBES_PER_PAGE ) { + pbe = pblist; +for (num =PBES_PER_PAGE ; num < nr_pages; num +=PBES_PER_PAGE ) { +if (!pbe) { +free_pagedir (pblist,1 ); +return NULL ; + } pbe +=PB_PAGE_SKIP ; pbe->next =alloc_image_page (gfp_mask, safe_needed); + pbe = pbe->next; } -if (!pbe) {/* get_zeroed_page() failed */ -free_pagedir (pblist,1 ); - pblist =NULL ; - }else -create_pbe_list (pblist, nr_pages); +create_pbe_list (pblist, nr_pages);return pblist; }
Re: [PATCH 3/3] swsusp: Fix alloc_pagedir
Hi!
Quoted 2 lines
Get rid of the FIXME in kernel/power/snapshot.c#alloc_pagedir() and simplify the functions called by it.
ACK.
Pavel
Signature · 2 lines
(english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html