[RESEND][PATCH] __isolate_lru_page:skip unneeded "not"

[RESEND][PATCH] __isolate_lru_page:skip unneeded "not"

[Bob Liu]

PageActive(page) will return int 0 or 1, mode is also int 0 or 1,
they are comparible so "not" is unneeded to be sure to boolean
values.
I also collected the ISOLATE_BOTH check together.

Signed-off-by: Bob Liu <lliubbo@gmail.com>
---
 mm/vmscan.c |   15 +++++----------
 1 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index e0e5f15..ce9ee85 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -862,16 +862,11 @@ int __isolate_lru_page(struct page *page, int mode, int file)
 	if (!PageLRU(page))
 		return ret;
 
-	/*
-	 * When checking the active state, we need to be sure we are
-	 * dealing with comparible boolean values.  Take the logical not
-	 * of each.
-	 */
-	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
-		return ret;
-
-	if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
-		return ret;
+	if (mode != ISOLATE_BOTH) {
+		if ((PageActive(page) != mode) ||
+			(page_is_file_cache(page) != file))
+				return ret;
+	}
 
 	/*
 	 * When this function is being called for lumpy reclaim, we
-- 
1.5.6.3

--
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: [RESEND][PATCH] __isolate_lru_page:skip unneeded "not"

[Andrew Morton]

On Thu,  1 Apr 2010 21:37:35 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> PageActive(page) will return int 0 or 1, mode is also int 0 or 1,
> they are comparible so "not" is unneeded to be sure to boolean
> values.
> I also collected the ISOLATE_BOTH check together.
> 
> Signed-off-by: Bob Liu <lliubbo@gmail.com>
> ---
>  mm/vmscan.c |   15 +++++----------
>  1 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index e0e5f15..ce9ee85 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -862,16 +862,11 @@ int __isolate_lru_page(struct page *page, int mode, int file)
>  	if (!PageLRU(page))
>  		return ret;
>  
> -	/*
> -	 * When checking the active state, we need to be sure we are
> -	 * dealing with comparible boolean values.  Take the logical not
> -	 * of each.
> -	 */

You deleted a spelling mistake too!

> -	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
> -		return ret;
> -
> -	if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
> -		return ret;
> +	if (mode != ISOLATE_BOTH) {
> +		if ((PageActive(page) != mode) ||
> +			(page_is_file_cache(page) != file))
> +				return ret;
> +	}

The compiler should be able to avoid testing for ISOLATE_BOTH twice,
and I think the previous code layout was superior:

	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
		return ret;

	if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
		return ret;

Because it gives us nice places to put a comment explaining what the
code is doing, whereas making it a more complex single expression:

	if (mode != ISOLATE_BOTH) {
		if ((PageActive(page) != mode) ||
			(page_is_file_cache(page) != file))
				return ret;
	}

makes clearly commenting each test more difficult.

Yeah, there's no comment there at present.  But that's because we suck
- I'm sure someone is working on it ;)

--
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: [RESEND][PATCH] __isolate_lru_page:skip unneeded "not"

[Bob Liu]

On 4/3/10, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Thu,  1 Apr 2010 21:37:35 +0800
> Bob Liu <lliubbo@gmail.com> wrote:
>
>> PageActive(page) will return int 0 or 1, mode is also int 0 or 1,
>> they are comparible so "not" is unneeded to be sure to boolean
>> values.
>> I also collected the ISOLATE_BOTH check together.
>>
>> Signed-off-by: Bob Liu <lliubbo@gmail.com>
>> ---
>>  mm/vmscan.c |   15 +++++----------
>>  1 files changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>> index e0e5f15..ce9ee85 100644
>> --- a/mm/vmscan.c
>> +++ b/mm/vmscan.c
>> @@ -862,16 +862,11 @@ int __isolate_lru_page(struct page *page, int mode,
>> int file)
>>  	if (!PageLRU(page))
>>  		return ret;
>>
>> -	/*
>> -	 * When checking the active state, we need to be sure we are
>> -	 * dealing with comparible boolean values.  Take the logical not
>> -	 * of each.
>> -	 */
>
> You deleted a spelling mistake too!
>
>> -	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
>> -		return ret;
>> -
>> -	if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
>> -		return ret;
>> +	if (mode != ISOLATE_BOTH) {
>> +		if ((PageActive(page) != mode) ||
>> +			(page_is_file_cache(page) != file))
>> +				return ret;
>> +	}
>
> The compiler should be able to avoid testing for ISOLATE_BOTH twice,

Thanks for your kindly reply.
then is the two "not" able to avoid by the compiler ?
if yes, this patch is meanless and should be ignore.

> and I think the previous code layout was superior:
>
> 	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
> 		return ret;
>
> 	if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
> 		return ret;
>
> Because it gives us nice places to put a comment explaining what the
> code is doing, whereas making it a more complex single expression:
>
> 	if (mode != ISOLATE_BOTH) {
> 		if ((PageActive(page) != mode) ||
> 			(page_is_file_cache(page) != file))
> 				return ret;
> 	}
>
> makes clearly commenting each test more difficult.
>
> Yeah, there's no comment there at present.  But that's because we suck
> - I'm sure someone is working on it ;)
>
>
-- 
Regards,
--Bob

--
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: [RESEND][PATCH] __isolate_lru_page:skip unneeded "not"

[Andrew Morton]

On Sat, 3 Apr 2010 06:25:08 +0800
Bob Liu <lliubbo@gmail.com> wrote:

> >> -	/*
> >> -	 * When checking the active state, we need to be sure we are
> >> -	 * dealing with comparible boolean values.  Take the logical not
> >> -	 * of each.
> >> -	 */
> >
> > You deleted a spelling mistake too!
> >
> >> -	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
> >> -		return ret;
> >> -
> >> -	if (mode != ISOLATE_BOTH && page_is_file_cache(page) != file)
> >> -		return ret;
> >> +	if (mode != ISOLATE_BOTH) {
> >> +		if ((PageActive(page) != mode) ||
> >> +			(page_is_file_cache(page) != file))
> >> +				return ret;
> >> +	}
> >
> > The compiler should be able to avoid testing for ISOLATE_BOTH twice,
> 
> Thanks for your kindly reply.
> then is the two "not" able to avoid by the compiler ?
> if yes, this patch is meanless and should be ignore.

I very much doubt if the compiler knows that these two variables can
only ever have values 0 or 1, so I expect that removing the !'s will
reduce text size.

That being said, it wouldn't be a good and maintainable change - 
one point in using enumerations such as ISOLATE_* is to hide their real
values.  Adding code which implicitly "knows" that a particular
enumerated identifier has a particular underlying value is rather
grubby and fragile.

But the code's already doing that.

It's also a bit fragile to assume that a true/false-returning C
function (PageActive) will always return 0 or 1.  It's a common C idiom
for such functions to return 0 or non-zero (not necessarily 1).


So a clean and maintainable implementation of

	if (mode != ISOLATE_BOTH && (!PageActive(page) != !mode))
		return ret;

would be

	if (mode != ISOLATE_BOTH &&
			((PageActive(page) && mode == ISOLATE_ACTIVE) ||
			 (!PageActive(page) && mode == ISOLATE_INACTIVE)))
		return ret;

which is just dying for an optimisation trick such as the one which is
already there ;)


--
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>