[PATCH] mm/page_alloc.c: fix a typo

[PATCH] mm/page_alloc.c: fix a typo

[Li Zefan]

---
 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bdd5c43..d0ba10d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page, unsigned long order)
 	for (i = 1; i < nr_pages; i++) {
 		struct page *p = page + i;
 
-		if (unlikely(!PageTail(p) |
+		if (unlikely(!PageTail(p) ||
 				(p->first_page != page)))
 			bad_page(page);
 		__ClearPageTail(p);
-- 1.5.4.rc3 

Re: [PATCH] mm/page_alloc.c: fix a typo

[Li Zefan]

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bdd5c43..d0ba10d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page, unsigned long order)
 	for (i = 1; i < nr_pages; i++) {
 		struct page *p = page + i;
 
-		if (unlikely(!PageTail(p) |
+		if (unlikely(!PageTail(p) ||
 				(p->first_page != page)))
 			bad_page(page);
 		__ClearPageTail(p);
-- 1.5.4.rc3 

Re: [PATCH] mm/page_alloc.c: fix a typo

[Li Zefan]

Li Zefan wrote:
> ---

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---

Sorry for the noise, but the signed-off was eaten. :(
Maybe I should leave a blank line before the signed-off.

---

 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index bdd5c43..d0ba10d 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page, unsigned long order)
 	for (i = 1; i < nr_pages; i++) {
 		struct page *p = page + i;
 
-		if (unlikely(!PageTail(p) |
+		if (unlikely(!PageTail(p) ||
 				(p->first_page != page)))
 			bad_page(page);
 		__ClearPageTail(p);
-- 1.5.4.rc3 

Re: [PATCH] mm/page_alloc.c: fix a typo

[Andrew Morton]

On Tue, 06 May 2008 17:50:31 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:

> Li Zefan wrote:
> > ---
> 
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
> 
> Sorry for the noise, but the signed-off was eaten. :(
> Maybe I should leave a blank line before the signed-off.
> 
> ---
> 
>  mm/page_alloc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index bdd5c43..d0ba10d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page, unsigned long order)
>  	for (i = 1; i < nr_pages; i++) {
>  		struct page *p = page + i;
>  
> -		if (unlikely(!PageTail(p) |
> +		if (unlikely(!PageTail(p) ||
>  				(p->first_page != page)))
>  			bad_page(page);
>  		__ClearPageTail(p);

I have a vague memory that the "|" was deliberate.  Most of the time,
"!PageTail" will be false so most of the time we won't take the first
branch so it's probably worth omitting it and always doing the pointer
comparison.

It's a somewhat dopey trick and shouldn't have been done without a comment.

otoh maybe it was a typo.

Re: [PATCH] mm/page_alloc.c: fix a typo

[Li Zefan]

Andrew Morton wrote:
> On Tue, 06 May 2008 17:50:31 +0800 Li Zefan <lizf@cn.fujitsu.com> wrote:
> 
>> Li Zefan wrote:
>>> ---
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>
>> Sorry for the noise, but the signed-off was eaten. :(
>> Maybe I should leave a blank line before the signed-off.
>>
>> ---
>>
>>  mm/page_alloc.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index bdd5c43..d0ba10d 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page, unsigned long order)
>>  	for (i = 1; i < nr_pages; i++) {
>>  		struct page *p = page + i;
>>  
>> -		if (unlikely(!PageTail(p) |
>> +		if (unlikely(!PageTail(p) ||
>>  				(p->first_page != page)))
>>  			bad_page(page);
>>  		__ClearPageTail(p);
> 
> I have a vague memory that the "|" was deliberate.  Most of the time,
> "!PageTail" will be false so most of the time we won't take the first

!PageTail will be true if nothing bad happened, corrected me if I'm wrong:

static void prep_compound_page(struct page *page, unsigned long order)
{
	...
	for (i = 1; i < nr_pages; i++) {
		struct page *p = page + i;

		__SetPageTail(p);
		p->first_page = page;
	}
}

> branch so it's probably worth omitting it and always doing the pointer
> comparison.
> 
> It's a somewhat dopey trick and shouldn't have been done without a comment.
> 
> otoh maybe it was a typo.
> 
> 

Re: [PATCH] mm/page_alloc.c: fix a typo

[Li Zefan]

Li Zefan wrote:
>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>> index bdd5c43..d0ba10d 100644
>>> --- a/mm/page_alloc.c
>>> +++ b/mm/page_alloc.c
>>> @@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page, unsigned long order)
>>>  	for (i = 1; i < nr_pages; i++) {
>>>  		struct page *p = page + i;
>>>  
>>> -		if (unlikely(!PageTail(p) |
>>> +		if (unlikely(!PageTail(p) ||
>>>  				(p->first_page != page)))
>>>  			bad_page(page);
>>>  		__ClearPageTail(p);
>> I have a vague memory that the "|" was deliberate.  Most of the time,
>> "!PageTail" will be false so most of the time we won't take the first
> 
> !PageTail will be true if nothing bad happened, corrected me if I'm wrong:
> 

Silly, I was wrong...

Christoph Lameter wrote:
> I think the | there was some developers attempt to avoid gcc generating 
> too many branches. I am fine either way.
> 

Yes, I found out it's 224abf92b2f439a9030f21d2926ec8047d1ffcdb :

[PATCH] mm: bad_page optimisation
Nick Piggin [Fri, 6 Jan 2006 08:11:11 +0000 (00:11 -0800)]

Cut down size slightly by not passing bad_page the function name (it should be
able to be determined by dump_stack()).  And cut down the number of printks in
bad_page.

Also, cut down some branching in the destroy_compound_page path.

Re: [PATCH] mm/page_alloc.c: fix a typo

[Christoph Lameter]

On Tue, 6 May 2008, Li Zefan wrote:

> Sorry for the noise, but the signed-off was eaten. :(
> Maybe I should leave a blank line before the signed-off.

I think the | there was some developers attempt to avoid gcc generating 
too many branches. I am fine either way.

Re: [PATCH] mm/page_alloc.c: fix a typo

[KOSAKI Motohiro]

> > Sorry for the noise, but the signed-off was eaten. :(
> > Maybe I should leave a blank line before the signed-off.
> 
> I think the | there was some developers attempt to avoid gcc generating 
> too many branches. I am fine either way.

Agreed.
but it is not normal conding style.
comment adding is better, IMHO.

at first, I think typo too.

Re: [PATCH] mm/page_alloc.c: fix a typo

[Oliver Pinter]

CC: stable

On 5/6/08, Li Zefan <lizf@cn.fujitsu.com> wrote:
>
> ---
>  mm/page_alloc.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index bdd5c43..d0ba10d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -303,7 +303,7 @@ static void destroy_compound_page(struct page *page,
> unsigned long order)
>  	for (i = 1; i < nr_pages; i++) {
>  		struct page *p = page + i;
>
> -		if (unlikely(!PageTail(p) |
> +		if (unlikely(!PageTail(p) ||
>  				(p->first_page != page)))
>  			bad_page(page);
>  		__ClearPageTail(p);
> -- 1.5.4.rc3
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>


-- 
Thanks,
Oliver

Re: [PATCH] mm/page_alloc.c: fix a typo

[Christoph Lameter]

On Tue, 6 May 2008, Oliver Pinter wrote:

> CC: stable

Not a bug, nor a typo so why stable?

Re: [PATCH] mm/page_alloc.c: fix a typo

[Oliver Pinter]

sorry,
when it is not bug


On 5/6/08, Christoph Lameter <clameter@sgi.com> wrote:
> On Tue, 6 May 2008, Oliver Pinter wrote:
>
> > CC: stable
>
> Not a bug, nor a typo so why stable?
>
>


-- 
Thanks,
Oliver