lkml.org 
[lkml]   [2015]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] PM / Hiberante : optimize swsusp_free()
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.


\
 
 \ /
  Last update: 2015-04-15 04:21    [W:0.055 / U:1.868 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site