* [PATCH] PM / Hiberante : optimize swsusp_free() @ 2015-03-19 8:28 Tom(JeHyeon) Yeon 2015-03-19 8:54 ` Pavel Machek 2015-03-19 11:37 ` Rafael J. Wysocki 0 siblings, 2 replies; 9+ messages in thread From: Tom(JeHyeon) Yeon @ 2015-03-19 8:28 UTC (permalink / raw) To: pavel, len.brown, jroedel, linux-pm; +Cc: linux-kernel, Tom(JeHyeon) Yeon From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> I tested the performance of swsusp_free operation. The free time took about 58768us before commit fdd64ed, and the free time took about 40535us after the commit fdd64ed. But, I optimized the function before I saw commit fdd64ed. So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) The free time took about 35164us. I think that the finding routine for the same pfn is redundant Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> --- kernel/power/snapshot.c | 43 ++++++++++--------------------------------- 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index c24d5a2..a1ad801 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) clear_bit(bit, addr); } -static void memory_bm_clear_current(struct memory_bitmap *bm) -{ - int bit; - - bit = max(bm->cur.node_bit - 1, 0); - clear_bit(bit, bm->cur.node->data); -} - static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) { void *addr; @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; void swsusp_free(void) { - unsigned long fb_pfn, fr_pfn; + unsigned long pfn; if (!forbidden_pages_map || !free_pages_map) goto out; memory_bm_position_reset(forbidden_pages_map); - memory_bm_position_reset(free_pages_map); - -loop: - fr_pfn = memory_bm_next_pfn(free_pages_map); - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); - - /* - * Find the next bit set in both bitmaps. This is guaranteed to - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. - */ - do { - if (fb_pfn < fr_pfn) - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); - if (fr_pfn < fb_pfn) - fr_pfn = memory_bm_next_pfn(free_pages_map); - } while (fb_pfn != fr_pfn); - - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { - struct page *page = pfn_to_page(fr_pfn); - - memory_bm_clear_current(forbidden_pages_map); - memory_bm_clear_current(free_pages_map); - __free_page(page); - goto loop; + for ( ; ; ) { + pfn = memory_bm_next_pfn(forbidden_pages_map); + if (BM_END_OF_MAP == pfn) + break; + if (memory_bm_test_bit(free_pages_map, pfn)) { + memory_bm_clear_bit(forbidden_pages_map, pfn); + memory_bm_clear_bit(free_pages_map, pfn); + __free_page(pfn_to_page(pfn)); + } } out: -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PM / Hiberante : optimize swsusp_free() 2015-03-19 8:28 [PATCH] PM / Hiberante : optimize swsusp_free() Tom(JeHyeon) Yeon @ 2015-03-19 8:54 ` Pavel Machek 2015-03-19 11:37 ` Rafael J. Wysocki 1 sibling, 0 replies; 9+ messages in thread From: Pavel Machek @ 2015-03-19 8:54 UTC (permalink / raw) To: Tom(JeHyeon) Yeon Cc: len.brown, jroedel, linux-pm, linux-kernel, Tom(JeHyeon) Yeon On Thu 2015-03-19 17:28:58, Tom(JeHyeon) Yeon wrote: > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > > I tested the performance of swsusp_free operation. > The free time took about 58768us before commit fdd64ed, and > the free time took about 40535us after the commit fdd64ed. > > But, I optimized the function before I saw commit fdd64ed. > So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) > The free time took about 35164us. > I think that the finding routine for the same pfn is redundant > > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> Well, saving 5msec on operation that takes 30 seconds is not that much, but new code looks cleaner. Acked-by: Pavel Machek <pavel@ucw.cz> > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM / Hiberante : optimize swsusp_free() 2015-03-19 8:28 [PATCH] PM / Hiberante : optimize swsusp_free() Tom(JeHyeon) Yeon 2015-03-19 8:54 ` Pavel Machek @ 2015-03-19 11:37 ` Rafael J. Wysocki 2015-03-24 0:50 ` Yeon, JeHyeon (Tom) 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 1 sibling, 2 replies; 9+ messages in thread From: Rafael J. Wysocki @ 2015-03-19 11:37 UTC (permalink / raw) To: Tom(JeHyeon) Yeon Cc: pavel, len.brown, jroedel, linux-pm, linux-kernel, Tom(JeHyeon) Yeon On Thursday, March 19, 2015 05:28:58 PM Tom Yeon wrote: > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > > I tested the performance of swsusp_free operation. > The free time took about 58768us before commit fdd64ed, and > the free time took about 40535us after the commit fdd64ed. > > But, I optimized the function before I saw commit fdd64ed. > So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) > The free time took about 35164us. > I think that the finding routine for the same pfn is redundant This changelog in total pants, sorry. Please write what the patch is doing and why instead of describing your testing experience. The numbers are useful too, but only to show what the gain is, and you need to explain what is changing and why. > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> > --- > kernel/power/snapshot.c | 43 ++++++++++--------------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index c24d5a2..a1ad801 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { So why exactly isn't it necessary to look at memory_bm_next_pfn(forbidden_pages_map)? > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] PM / Hiberante : optimize swsusp_free() 2015-03-19 11:37 ` Rafael J. Wysocki @ 2015-03-24 0:50 ` Yeon, JeHyeon (Tom) 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 1 sibling, 0 replies; 9+ messages in thread From: Yeon, JeHyeon (Tom) @ 2015-03-24 0:50 UTC (permalink / raw) To: Rafael J. Wysocki, Tom(JeHyeon) Yeon Cc: pavel, BROWN, A LEONARD, jroedel, linux-pm, linux-kernel [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3682 bytes --] I'm sorry not to answer it. I'm too busy nowadays on my project. So, I'll add some details later. Thank you. -----Original Message----- From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] Sent: Thursday, March 19, 2015 8:37 PM To: Tom(JeHyeon) Yeon Cc: pavel@ucw.cz; BROWN, A LEONARD; jroedel@suse.de; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Yeon, JeHyeon (Tom) Subject: Re: [PATCH] PM / Hiberante : optimize swsusp_free() On Thursday, March 19, 2015 05:28:58 PM Tom Yeon wrote: > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > > I tested the performance of swsusp_free operation. > The free time took about 58768us before commit fdd64ed, and > the free time took about 40535us after the commit fdd64ed. > > But, I optimized the function before I saw commit fdd64ed. > So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) > The free time took about 35164us. > I think that the finding routine for the same pfn is redundant This changelog in total pants, sorry. Please write what the patch is doing and why instead of describing your testing experience. The numbers are useful too, but only to show what the gain is, and you need to explain what is changing and why. > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> > --- > kernel/power/snapshot.c | 43 ++++++++++--------------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index c24d5a2..a1ad801 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { So why exactly isn't it necessary to look at memory_bm_next_pfn(forbidden_pages_map)? > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥ ^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH] PM / Hiberante : optimize swsusp_free() @ 2015-03-24 0:50 ` Yeon, JeHyeon (Tom) 0 siblings, 0 replies; 9+ messages in thread From: Yeon, JeHyeon (Tom) @ 2015-03-24 0:50 UTC (permalink / raw) To: Rafael J. Wysocki, Tom(JeHyeon) Yeon Cc: pavel, BROWN, A LEONARD, jroedel, linux-pm, linux-kernel I'm sorry not to answer it. I'm too busy nowadays on my project. So, I'll add some details later. Thank you. -----Original Message----- From: Rafael J. Wysocki [mailto:rjw@rjwysocki.net] Sent: Thursday, March 19, 2015 8:37 PM To: Tom(JeHyeon) Yeon Cc: pavel@ucw.cz; BROWN, A LEONARD; jroedel@suse.de; linux-pm@vger.kernel.org; linux-kernel@vger.kernel.org; Yeon, JeHyeon (Tom) Subject: Re: [PATCH] PM / Hiberante : optimize swsusp_free() On Thursday, March 19, 2015 05:28:58 PM Tom Yeon wrote: > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > > I tested the performance of swsusp_free operation. > The free time took about 58768us before commit fdd64ed, and > the free time took about 40535us after the commit fdd64ed. > > But, I optimized the function before I saw commit fdd64ed. > So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) > The free time took about 35164us. > I think that the finding routine for the same pfn is redundant This changelog in total pants, sorry. Please write what the patch is doing and why instead of describing your testing experience. The numbers are useful too, but only to show what the gain is, and you need to explain what is changing and why. > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> > --- > kernel/power/snapshot.c | 43 ++++++++++--------------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index c24d5a2..a1ad801 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { So why exactly isn't it necessary to look at memory_bm_next_pfn(forbidden_pages_map)? > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM / Hiberante : optimize swsusp_free() 2015-03-19 11:37 ` Rafael J. Wysocki @ 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 1 sibling, 0 replies; 9+ messages in thread From: Yeon, JeHyeon (Tom) @ 2015-03-25 1:49 UTC (permalink / raw) To: Rafael J. Wysocki, pavel, jroedel Cc: BROWN, A LEONARD, jroedel, linux-pm, linux-kernel, Tom(JeHyeon) Yeon >From 6cb5fffc41911a29212be52d4ce7e481f5077ccf Mon Sep 17 00:00:00 2001 From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> Date: Thu, 19 Mar 2015 17:10:45 +0900 Subject: [PATCH] PM / Hiberante : optimize swsusp_free() Our team developed the snapshot booting. Fisrt of all, make a snapshot image, compress it and finally save it in the storage(like mmc). When the system is booting next time, bootloader read it from mmc, decompress it and jump to the kernel. In this circumstance, mili seconds is very important. So, I prepared this patch, but not applied because I missed the time to apply it. And, I came across to find commit fdd64ed. It's very similar to the patch I prepared. I think do { ... } while (fb_pfn != fr_pfn) operation is very similar to my patch. but, it takes a little more time to iterate. So suggest to iterate one of two maps and check whether the other map has the same pfn, finally free the page. Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> --- kernel/power/snapshot.c | 43 ++++++++++--------------------------------- 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index c24d5a2..a1ad801 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) clear_bit(bit, addr); } -static void memory_bm_clear_current(struct memory_bitmap *bm) -{ - int bit; - - bit = max(bm->cur.node_bit - 1, 0); - clear_bit(bit, bm->cur.node->data); -} - static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) { void *addr; @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; void swsusp_free(void) { - unsigned long fb_pfn, fr_pfn; + unsigned long pfn; if (!forbidden_pages_map || !free_pages_map) goto out; memory_bm_position_reset(forbidden_pages_map); - memory_bm_position_reset(free_pages_map); - -loop: - fr_pfn = memory_bm_next_pfn(free_pages_map); - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); - - /* - * Find the next bit set in both bitmaps. This is guaranteed to - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. - */ - do { - if (fb_pfn < fr_pfn) - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); - if (fr_pfn < fb_pfn) - fr_pfn = memory_bm_next_pfn(free_pages_map); - } while (fb_pfn != fr_pfn); - - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { - struct page *page = pfn_to_page(fr_pfn); - - memory_bm_clear_current(forbidden_pages_map); - memory_bm_clear_current(free_pages_map); - __free_page(page); - goto loop; + for ( ; ; ) { + pfn = memory_bm_next_pfn(forbidden_pages_map); + if (BM_END_OF_MAP == pfn) + break; + if (memory_bm_test_bit(free_pages_map, pfn)) { + memory_bm_clear_bit(forbidden_pages_map, pfn); + memory_bm_clear_bit(free_pages_map, pfn); + __free_page(pfn_to_page(pfn)); + } } out: -- 1.7.9.5 describe it in details. As pavel said, 5ms is not important in the normal booting system. but mili seconds is important in the hibernation or snapshot system. Just suggestion. Thank you. ------------------ On Thursday, March 19, 2015 05:28:58 PM Tom Yeon wrote: > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > > I tested the performance of swsusp_free operation. > The free time took about 58768us before commit fdd64ed, and > the free time took about 40535us after the commit fdd64ed. > > But, I optimized the function before I saw commit fdd64ed. > So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) > The free time took about 35164us. > I think that the finding routine for the same pfn is redundant This changelog in total pants, sorry. Please write what the patch is doing and why instead of describing your testing experience. The numbers are useful too, but only to show what the gain is, and you need to explain what is changing and why. > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> > --- > kernel/power/snapshot.c | 43 ++++++++++--------------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index c24d5a2..a1ad801 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { So why exactly isn't it necessary to look at memory_bm_next_pfn(forbidden_pages_map)? > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PM / Hiberante : optimize swsusp_free() @ 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 0 siblings, 0 replies; 9+ messages in thread From: Yeon, JeHyeon (Tom) @ 2015-03-25 1:49 UTC (permalink / raw) To: Rafael J. Wysocki, pavel Cc: BROWN, A LEONARD, jroedel, linux-pm, linux-kernel, Tom(JeHyeon) Yeon >From 6cb5fffc41911a29212be52d4ce7e481f5077ccf Mon Sep 17 00:00:00 2001 From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> Date: Thu, 19 Mar 2015 17:10:45 +0900 Subject: [PATCH] PM / Hiberante : optimize swsusp_free() Our team developed the snapshot booting. Fisrt of all, make a snapshot image, compress it and finally save it in the storage(like mmc). When the system is booting next time, bootloader read it from mmc, decompress it and jump to the kernel. In this circumstance, mili seconds is very important. So, I prepared this patch, but not applied because I missed the time to apply it. And, I came across to find commit fdd64ed. It's very similar to the patch I prepared. I think do { ... } while (fb_pfn != fr_pfn) operation is very similar to my patch. but, it takes a little more time to iterate. So suggest to iterate one of two maps and check whether the other map has the same pfn, finally free the page. Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> --- kernel/power/snapshot.c | 43 ++++++++++--------------------------------- 1 file changed, 10 insertions(+), 33 deletions(-) diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c index c24d5a2..a1ad801 100644 --- a/kernel/power/snapshot.c +++ b/kernel/power/snapshot.c @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) clear_bit(bit, addr); } -static void memory_bm_clear_current(struct memory_bitmap *bm) -{ - int bit; - - bit = max(bm->cur.node_bit - 1, 0); - clear_bit(bit, bm->cur.node->data); -} - static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) { void *addr; @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; void swsusp_free(void) { - unsigned long fb_pfn, fr_pfn; + unsigned long pfn; if (!forbidden_pages_map || !free_pages_map) goto out; memory_bm_position_reset(forbidden_pages_map); - memory_bm_position_reset(free_pages_map); - -loop: - fr_pfn = memory_bm_next_pfn(free_pages_map); - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); - - /* - * Find the next bit set in both bitmaps. This is guaranteed to - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. - */ - do { - if (fb_pfn < fr_pfn) - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); - if (fr_pfn < fb_pfn) - fr_pfn = memory_bm_next_pfn(free_pages_map); - } while (fb_pfn != fr_pfn); - - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { - struct page *page = pfn_to_page(fr_pfn); - - memory_bm_clear_current(forbidden_pages_map); - memory_bm_clear_current(free_pages_map); - __free_page(page); - goto loop; + for ( ; ; ) { + pfn = memory_bm_next_pfn(forbidden_pages_map); + if (BM_END_OF_MAP == pfn) + break; + if (memory_bm_test_bit(free_pages_map, pfn)) { + memory_bm_clear_bit(forbidden_pages_map, pfn); + memory_bm_clear_bit(free_pages_map, pfn); + __free_page(pfn_to_page(pfn)); + } } out: -- 1.7.9.5 describe it in details. As pavel said, 5ms is not important in the normal booting system. but mili seconds is important in the hibernation or snapshot system. Just suggestion. Thank you. ------------------ On Thursday, March 19, 2015 05:28:58 PM Tom Yeon wrote: > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > > I tested the performance of swsusp_free operation. > The free time took about 58768us before commit fdd64ed, and > the free time took about 40535us after the commit fdd64ed. > > But, I optimized the function before I saw commit fdd64ed. > So, I applied the patch in my system.(ARM Coretex A9, Dual Core 1GHz) > The free time took about 35164us. > I think that the finding routine for the same pfn is redundant This changelog in total pants, sorry. Please write what the patch is doing and why instead of describing your testing experience. The numbers are useful too, but only to show what the gain is, and you need to explain what is changing and why. > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> > --- > kernel/power/snapshot.c | 43 ++++++++++--------------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index c24d5a2..a1ad801 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { So why exactly isn't it necessary to look at memory_bm_next_pfn(forbidden_pages_map)? > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: > -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] PM / Hiberante : optimize swsusp_free() 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) (?) @ 2015-04-11 0:20 ` Rafael J. Wysocki 2015-04-15 1:40 ` tyeon -1 siblings, 1 reply; 9+ messages in thread From: Rafael J. Wysocki @ 2015-04-11 0:20 UTC (permalink / raw) To: Yeon, JeHyeon (Tom) Cc: pavel, jroedel, BROWN, A LEONARD, linux-pm, linux-kernel, Tom(JeHyeon) Yeon On Wednesday, March 25, 2015 01:49:36 AM Yeon, JeHyeon wrote: > From 6cb5fffc41911a29212be52d4ce7e481f5077ccf Mon Sep 17 00:00:00 2001 > From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> > Date: Thu, 19 Mar 2015 17:10:45 +0900 > Subject: [PATCH] PM / Hiberante : optimize swsusp_free() > > Our team developed the snapshot booting. > Fisrt of all, make a snapshot image, compress it and finally save it > in the storage(like mmc). > When the system is booting next time, bootloader read it from mmc, > decompress it and jump to the kernel. > In this circumstance, mili seconds is very important. > So, I prepared this patch, but not applied because I missed the time > to apply it. > > And, I came across to find commit fdd64ed. > It's very similar to the patch I prepared. So the part of the changelog above this line is not really relevant. But the below is OK. > I think do { ... } while (fb_pfn != fr_pfn) operation is very similar > to my patch. but, it takes a little more time to iterate. > So suggest to iterate one of two maps and check whether the other map > has the same pfn, finally free the page. > > Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> As for the patch itself -> > --- > kernel/power/snapshot.c | 43 ++++++++++--------------------------------- > 1 file changed, 10 insertions(+), 33 deletions(-) > > diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c > index c24d5a2..a1ad801 100644 > --- a/kernel/power/snapshot.c > +++ b/kernel/power/snapshot.c > @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) > clear_bit(bit, addr); > } > > -static void memory_bm_clear_current(struct memory_bitmap *bm) > -{ > - int bit; > - > - bit = max(bm->cur.node_bit - 1, 0); > - clear_bit(bit, bm->cur.node->data); > -} > - > static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) > { > void *addr; > @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; > > void swsusp_free(void) > { > - unsigned long fb_pfn, fr_pfn; > + unsigned long pfn; > > if (!forbidden_pages_map || !free_pages_map) > goto out; > > memory_bm_position_reset(forbidden_pages_map); > - memory_bm_position_reset(free_pages_map); > - > -loop: > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - > - /* > - * Find the next bit set in both bitmaps. This is guaranteed to > - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. > - */ > - do { > - if (fb_pfn < fr_pfn) > - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); > - if (fr_pfn < fb_pfn) > - fr_pfn = memory_bm_next_pfn(free_pages_map); > - } while (fb_pfn != fr_pfn); > - > - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { > - struct page *page = pfn_to_page(fr_pfn); > - > - memory_bm_clear_current(forbidden_pages_map); > - memory_bm_clear_current(free_pages_map); > - __free_page(page); > - goto loop; > + for ( ; ; ) { > + pfn = memory_bm_next_pfn(forbidden_pages_map); > + if (BM_END_OF_MAP == pfn) -> First, the usual way of writing such things is if (pfn == BM_END_OF_MAP) (ie. the variable on the left-hand side of the operator). Second, don't you need to do the pfn_valid() check here too? > + break; > + if (memory_bm_test_bit(free_pages_map, pfn)) { > + memory_bm_clear_bit(forbidden_pages_map, pfn); > + memory_bm_clear_bit(free_pages_map, pfn); > + __free_page(pfn_to_page(pfn)); > + } > } > > out: -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PM / Hiberante : optimize swsusp_free() 2015-04-11 0:20 ` Rafael J. Wysocki @ 2015-04-15 1:40 ` tyeon 0 siblings, 0 replies; 9+ messages in thread From: tyeon @ 2015-04-15 1:40 UTC (permalink / raw) To: Rafael J. Wysocki Cc: pavel, jroedel, BROWN, A LEONARD, linux-pm, linux-kernel, Tom(JeHyeon) Yeon On Saturday, April 11, 2015 09:20 AM Rafael J. Wysocki worte: > On Wednesday, March 25, 2015 01:49:36 AM Yeon, JeHyeon wrote: >> From 6cb5fffc41911a29212be52d4ce7e481f5077ccf Mon Sep 17 00:00:00 2001 >> From: "Tom(JeHyeon) Yeon" <tom.yeon@windriver.com> >> Date: Thu, 19 Mar 2015 17:10:45 +0900 >> Subject: [PATCH] PM / Hiberante : optimize swsusp_free() >> >> Our team developed the snapshot booting. >> Fisrt of all, make a snapshot image, compress it and finally save it >> in the storage(like mmc). >> When the system is booting next time, bootloader read it from mmc, >> decompress it and jump to the kernel. >> In this circumstance, mili seconds is very important. >> So, I prepared this patch, but not applied because I missed the time >> to apply it. >> >> And, I came across to find commit fdd64ed. >> It's very similar to the patch I prepared. > > So the part of the changelog above this line is not really relevant. > > But the below is OK. Ok, I'll get rid of the upper changelog. > >> I think do { ... } while (fb_pfn != fr_pfn) operation is very similar >> to my patch. but, it takes a little more time to iterate. >> So suggest to iterate one of two maps and check whether the other map >> has the same pfn, finally free the page. >> >> Signed-off-by: Tom(JeHyeon) Yeon <tom.yeon@windriver.com> > > As for the patch itself -> > >> --- >> kernel/power/snapshot.c | 43 ++++++++++--------------------------------- >> 1 file changed, 10 insertions(+), 33 deletions(-) >> >> diff --git a/kernel/power/snapshot.c b/kernel/power/snapshot.c >> index c24d5a2..a1ad801 100644 >> --- a/kernel/power/snapshot.c >> +++ b/kernel/power/snapshot.c >> @@ -726,14 +726,6 @@ static void memory_bm_clear_bit(struct memory_bitmap *bm, unsigned long pfn) >> clear_bit(bit, addr); >> } >> >> -static void memory_bm_clear_current(struct memory_bitmap *bm) >> -{ >> - int bit; >> - >> - bit = max(bm->cur.node_bit - 1, 0); >> - clear_bit(bit, bm->cur.node->data); >> -} >> - >> static int memory_bm_test_bit(struct memory_bitmap *bm, unsigned long pfn) >> { >> void *addr; >> @@ -1342,36 +1334,21 @@ static struct memory_bitmap copy_bm; >> >> void swsusp_free(void) >> { >> - unsigned long fb_pfn, fr_pfn; >> + unsigned long pfn; >> >> if (!forbidden_pages_map || !free_pages_map) >> goto out; >> >> memory_bm_position_reset(forbidden_pages_map); >> - memory_bm_position_reset(free_pages_map); >> - >> -loop: >> - fr_pfn = memory_bm_next_pfn(free_pages_map); >> - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); >> - >> - /* >> - * Find the next bit set in both bitmaps. This is guaranteed to >> - * terminate when fb_pfn == fr_pfn == BM_END_OF_MAP. >> - */ >> - do { >> - if (fb_pfn < fr_pfn) >> - fb_pfn = memory_bm_next_pfn(forbidden_pages_map); >> - if (fr_pfn < fb_pfn) >> - fr_pfn = memory_bm_next_pfn(free_pages_map); >> - } while (fb_pfn != fr_pfn); >> - >> - if (fr_pfn != BM_END_OF_MAP && pfn_valid(fr_pfn)) { >> - struct page *page = pfn_to_page(fr_pfn); >> - >> - memory_bm_clear_current(forbidden_pages_map); >> - memory_bm_clear_current(free_pages_map); >> - __free_page(page); >> - goto loop; >> + for ( ; ; ) { >> + pfn = memory_bm_next_pfn(forbidden_pages_map); >> + if (BM_END_OF_MAP == pfn) > > -> First, the usual way of writing such things is > > if (pfn == BM_END_OF_MAP) > > (ie. the variable on the left-hand side of the operator). Is there any rules for this this in kernel? Sometime, human makes a mistake like "if (pfn = BM_END_OF_MAP)" that's why I wrote like that even though the compiler may notice about it. > > Second, don't you need to do the pfn_valid() check here too? hmm. I can add pfn_valid() check. but I don't think that pfn_valid() should be checked in this stage. I think forbidden_pages_map & free_pages_map should be always valid. Is there any possibility those are not valid in this stage? > >> + break; >> + if (memory_bm_test_bit(free_pages_map, pfn)) { >> + memory_bm_clear_bit(forbidden_pages_map, pfn); >> + memory_bm_clear_bit(free_pages_map, pfn); >> + __free_page(pfn_to_page(pfn)); >> + } >> } >> >> out: > > thanks. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-04-15 1:41 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-19 8:28 [PATCH] PM / Hiberante : optimize swsusp_free() Tom(JeHyeon) Yeon 2015-03-19 8:54 ` Pavel Machek 2015-03-19 11:37 ` Rafael J. Wysocki 2015-03-24 0:50 ` Yeon, JeHyeon (Tom) 2015-03-24 0:50 ` Yeon, JeHyeon (Tom) 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 2015-03-25 1:49 ` Yeon, JeHyeon (Tom) 2015-04-11 0:20 ` Rafael J. Wysocki 2015-04-15 1:40 ` tyeon
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.