Skip to contentSkip to conversation

mm/page_alloc.c: fix a typo

PATCH In review Li Zefan 5 participants 11 messages May 6, 2008 – May 8, 2008

Subsystems (2)
Memory management Page allocator
lore
Share & tools
lore

Li Zefan

author
#0 lore raw

PATCH 1/1 mm/page_alloc.c: fix a typo

---
 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/page_alloc.c +1 -1
index bdd5c43..d0ba10d 100644
@@ -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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#1 lore raw
---
 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/page_alloc.c +1 -1
index bdd5c43..d0ba10d 100644
@@ -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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#2 lore raw
Quoted 1 line · Li Zefan

---

Signed-off-by Li Zefan <[email protected]>

---

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(-)
mm/page_alloc.c +1 -1
index bdd5c43..d0ba10d 100644
@@ -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, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

maintainer
#3 lore raw
Quoted 19 lines · On Tue, 06 May 2008 17:50:31 +0800 Li Zefan <[email protected]> wrote:
Quoted 1 line · Li Zefan

---

Signed-off-by Li Zefan <[email protected]>

---

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(-)
mm/page_alloc.c +0 -0
index bdd5c43..d0ba10d 100644

mm/page_alloc.c static void destroy_compound_page(struct page *page, unsigned long order) · line 309

Quoted 8 lines
mm/page_alloc.c +1 -1
@@ -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.

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

author
#4 lore raw
Quoted 23 lines · Andrew Morton
Quoted 21 lines · On Tue, 06 May 2008 17:50:31 +0800 Li Zefan <[email protected]> wrote:
Quoted 1 line · Li Zefan

---

Signed-off-by Li Zefan <[email protected]>

---

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(-)
mm/page_alloc.c +0 -0
index bdd5c43..d0ba10d 100644
@@ -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;
Quoted 8 lines

mm/page_alloc.c static void destroy_compound_page(struct page *page, unsigned long order) · line 309

Quoted 5 lines
mm/page_alloc.c +1 -1
-		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;
	}
}
Quoted 8 lines

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.

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

author
#5 lore raw
Quoted 16 lines · Li Zefan
Quoted 8 lines
Quoted 8 lines
mm/page_alloc.c +0 -0
index bdd5c43..d0ba10d 100644
@@ -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;
Quoted 7 lines

mm/page_alloc.c static void destroy_compound_page(struct page *page, unsigned long order) · line 309

Quoted 5 lines
mm/page_alloc.c +1 -1
-		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

Quoted 2 lines

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

Silly, I was wrong...

Quoted 3 lines · Christoph Lameter

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.

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

#6 lore raw
In reply to Li Zefan
Quoted 2 lines · Li Zefan

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.

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

#7 lore raw
Quoted 5 lines
Quoted 2 lines

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.

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

#8 lore raw
In reply to Li Zefan
Quoted 26 lines · Li Zefan
---
 mm/page_alloc.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
mm/page_alloc.c +1 -1
index bdd5c43..d0ba10d 100644
@@ -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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Signature · 8 lines
Thanks,
Oliver

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to [email protected].  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
#10 lore raw

sorry,
when it is not bug

Quoted 7 lines · Christoph Lameter
Quoted 1 line · Oliver Pinter
CC stable

Not a bug, nor a typo so why stable?

Signature · 8 lines
Thanks,
Oliver

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