[PATCH] mm: remove incorrect swap_count() from try_to_unuse()

[PATCH] mm: remove incorrect swap_count() from try_to_unuse()

[Hugh Dickins]

From: Bo Liu <bo-liu@hotmail.com>

In try_to_unuse(), swcount is a local copy of *swap_map, including the
SWAP_HAS_CACHE bit; but a wrong comparison against swap_count(*swap_map),
which masks off the SWAP_HAS_CACHE bit, succeeded where it should fail.

That had the effect of resetting the mm from which to start searching
for the next swap page, to an irrelevant mm instead of to an mm in which
this swap page had been found: which may increase search time by ~20%.
But we're used to swapoff being slow, so never noticed the slowdown.

Remove that one spurious use of swap_count(): Bo Liu thought it merely
redundant, Hugh rewrote the description since it was measurably wrong.

Signed-off-by: Bo Liu <bo-liu@hotmail.com>
Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Cc: stable@kernel.org
---

 mm/swapfile.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- 2.6.32-rc5/mm/swapfile.c	2009-10-05 04:20:31.000000000 +0100
+++ linux/mm/swapfile.c	2009-10-28 19:31:43.000000000 +0000
@@ -1151,8 +1151,7 @@ static int try_to_unuse(unsigned int typ
 				} else
 					retval = unuse_mm(mm, entry, page);
 
-				if (set_start_mm &&
-				    swap_count(*swap_map) < swcount) {
+				if (set_start_mm && *swap_map < swcount) {
 					mmput(new_start_mm);
 					atomic_inc(&mm->mm_users);
 					new_start_mm = mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] try_to_unuse : remove redundant swap_count()

[Bo Liu]

 
While comparing with swcount,it's no need to
call swap_count(). Just as int set_start_mm = 
(*swap_map>= swcount) is ok.
 
Signed-off-by: Bo Liu <bo-liu@hotmail.com>
---
 
diff --git a/mm/swapfile.c b/mm/swapfile.c
index 63ce10f..2456fc6 100644
--- a/mm/swapfile.c
+++ b/mm/swapfile.c
@@ -1152,7 +1152,7 @@ static int try_to_unuse(unsigned int type)
      retval = unuse_mm(mm, entry, page);
     if (set_start_mm &&
-        swap_count(*swap_map) < swcount) {
+         ((*swap_map) < swcount)) {
      mmput(new_start_mm);
      atomic_inc(&mm->mm_users);
      new_start_mm = mm;
 
-- 
1.6.0.6 		 	   		  
_________________________________________________________________
Windows Live Hotmail: Your friends can get your Facebook updates, right from Hotmail®.
http://www.microsoft.com/middleeast/windows/windowslive/see-it-in-action/social-network-basics.aspx?ocid=PID23461::T:WLMTAGL:ON:WL:en-xm:SI_SB_4:092009
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] try_to_unuse : remove redundant swap_count()

[KAMEZAWA Hiroyuki]

Bo Liu  wrote:
>
>
> While comparing with swcount,it's no need to
> call swap_count(). Just as int set_start_mm =
> (*swap_map>= swcount) is ok.
>
Hmm ?
*swap_map = (SWAP_HAS_CACHE) | count. What this change means ?

Anyway, swap_count() macro is removed by Hugh's patch (queued in -mm)

Regards,
-Kame

> Signed-off-by: Bo Liu <bo-liu@hotmail.com>
> ---
>
> diff --git a/mm/swapfile.c b/mm/swapfile.c
> index 63ce10f..2456fc6 100644
> --- a/mm/swapfile.c
> +++ b/mm/swapfile.c
> @@ -1152,7 +1152,7 @@ static int try_to_unuse(unsigned int type)
>       retval = unuse_mm(mm, entry, page);
>      if (set_start_mm &&
> -        swap_count(*swap_map) < swcount) {
> +         ((*swap_map) < swcount)) {
>       mmput(new_start_mm);
>       atomic_inc(&mm->mm_users);
>       new_start_mm = mm;
>
> --
> 1.6.0.6
> _________________________________________________________________
> Windows Live Hotmail: Your friends can get your Facebook updates, right
from
>  Hotmailョ.
> http://www.microsoft.com/middleeast/windows/windowslive/see-it-in-action/social-network-basics.aspx?ocid=PID23461::T:WLMTAGL:ON:WL:en-xm:SI_SB_4:092009
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] try_to_unuse : remove redundant swap_count()

[Bob Liu]

>>
>> While comparing with swcount,it's no need to
>> call swap_count(). Just as int set_start_mm =
>> (*swap_map>= swcount) is ok.
>>
> Hmm ?
> *swap_map = (SWAP_HAS_CACHE) | count. What this change means ?
>

Sorry for the wrong format, I changed to gmail.
Because swcount is assigned value *swap_map not swap_count(*swap_map).
So I think here should compare with *swap_map not swap_count(*swap_map).

And refer to variable set_start_mm, it is inited also by comparing
*swap_map and swcount not swap_count(*swap_map) and swcount.
So I submited this patch.

> Anyway, swap_count() macro is removed by Hugh's patch (queued in -mm)
>
I am sorry for not notice that. So just forget about this patch.
Thanks!
-Bo

> Regards,
> -Kame
>
>> Signed-off-by: Bo Liu <bo-liu@hotmail.com>
>> ---
>>
>> diff --git a/mm/swapfile.c b/mm/swapfile.c
>> index 63ce10f..2456fc6 100644
>> --- a/mm/swapfile.c
>> +++ b/mm/swapfile.c
>> @@ -1152,7 +1152,7 @@ static int try_to_unuse(unsigned int type)
>>       retval = unuse_mm(mm, entry, page);
>>      if (set_start_mm &&
>> -        swap_count(*swap_map) < swcount) {
>> +         ((*swap_map) < swcount)) {
>>       mmput(new_start_mm);
>>       atomic_inc(&mm->mm_users);
>>       new_start_mm = mm;
>>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] try_to_unuse : remove redundant swap_count()

[Hugh Dickins]

On Wed, 21 Oct 2009, Bob Liu wrote:
> >>
> >> While comparing with swcount,it's no need to
> >> call swap_count(). Just as int set_start_mm =
> >> (*swap_map>= swcount) is ok.
> >>
> > Hmm ?
> > *swap_map = (SWAP_HAS_CACHE) | count. What this change means ?
> >
> 
> Sorry for the wrong format, I changed to gmail.
> Because swcount is assigned value *swap_map not swap_count(*swap_map).
> So I think here should compare with *swap_map not swap_count(*swap_map).
> 
> And refer to variable set_start_mm, it is inited also by comparing
> *swap_map and swcount not swap_count(*swap_map) and swcount.
> So I submited this patch.

Thanks a lot for the fuller description: I mistakenly dismissed
your patch the first time, misunderstanding what you had found.

As I remarked in private mail (being smtp-challenged last week),
what you found was worse than a redundant use of swap_count(): it
was a wrong use of swap_count(), and caused an (easily overlooked)
regression in swapoff's (never wonderful) performance.

> 
> > Anyway, swap_count() macro is removed by Hugh's patch (queued in -mm)

Actually no: I removed some of the other wrappers, which were obscuring
things for me; but swap_count() still seemed useful, so I left it.

> >
> I am sorry for not notice that. So just forget about this patch.

No, let's not forget it at all, it was a good find, thank you.
Updated version of the patch comes in following mail.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] mm: remove incorrect swap_count() from try_to_unuse()

[Hugh Dickins]

From: Bo Liu <bo-liu@hotmail.com>

In try_to_unuse(), swcount is a local copy of *swap_map, including the
SWAP_HAS_CACHE bit; but a wrong comparison against swap_count(*swap_map),
which masks off the SWAP_HAS_CACHE bit, succeeded where it should fail.

That had the effect of resetting the mm from which to start searching
for the next swap page, to an irrelevant mm instead of to an mm in which
this swap page had been found: which may increase search time by ~20%.
But we're used to swapoff being slow, so never noticed the slowdown.

Remove that one spurious use of swap_count(): Bo Liu thought it merely
redundant, Hugh rewrote the description since it was measurably wrong.

Signed-off-by: Bo Liu <bo-liu@hotmail.com>
Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: stable@kernel.org
---

 mm/swapfile.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

--- 2.6.32-rc5/mm/swapfile.c	2009-10-05 04:20:31.000000000 +0100
+++ linux/mm/swapfile.c	2009-10-28 19:31:43.000000000 +0000
@@ -1151,8 +1151,7 @@ static int try_to_unuse(unsigned int typ
 				} else
 					retval = unuse_mm(mm, entry, page);
 
-				if (set_start_mm &&
-				    swap_count(*swap_map) < swcount) {
+				if (set_start_mm && *swap_map < swcount) {
 					mmput(new_start_mm);
 					atomic_inc(&mm->mm_users);
 					new_start_mm = mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm: remove incorrect swap_count() from try_to_unuse()

[KAMEZAWA Hiroyuki]

On Wed, 28 Oct 2009 20:34:38 +0000 (GMT)
Hugh Dickins <hugh.dickins@tiscali.co.uk> wrote:

> From: Bo Liu <bo-liu@hotmail.com>
> 
> In try_to_unuse(), swcount is a local copy of *swap_map, including the
> SWAP_HAS_CACHE bit; but a wrong comparison against swap_count(*swap_map),
> which masks off the SWAP_HAS_CACHE bit, succeeded where it should fail.
> 
Ah, okay...

> That had the effect of resetting the mm from which to start searching
> for the next swap page, to an irrelevant mm instead of to an mm in which
> this swap page had been found: which may increase search time by ~20%.
> But we're used to swapoff being slow, so never noticed the slowdown.
> 
> Remove that one spurious use of swap_count(): Bo Liu thought it merely
> redundant, Hugh rewrote the description since it was measurably wrong.
> 
> Signed-off-by: Bo Liu <bo-liu@hotmail.com>
> Signed-off-by: Hugh Dickins <hugh.dickins@tiscali.co.uk>
> Cc: stable@kernel.org

Sorry for my misunderstanding.

Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

> ---
> 
>  mm/swapfile.c |    3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> --- 2.6.32-rc5/mm/swapfile.c	2009-10-05 04:20:31.000000000 +0100
> +++ linux/mm/swapfile.c	2009-10-28 19:31:43.000000000 +0000
> @@ -1151,8 +1151,7 @@ static int try_to_unuse(unsigned int typ
>  				} else
>  					retval = unuse_mm(mm, entry, page);
>  
> -				if (set_start_mm &&
> -				    swap_count(*swap_map) < swcount) {
> +				if (set_start_mm && *swap_map < swcount) {
>  					mmput(new_start_mm);
>  					atomic_inc(&mm->mm_users);
>  					new_start_mm = mm;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>