Re: [PATCH] PM / Hiberante : optimize swsusp_free()
From: tyeon
Date: Tue Apr 14 2015 - 21:41:23 EST
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxx>
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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/