Skip to contentSkip to conversation

mm: Minor clean-up of page flags in mm/page_alloc.c v4

PATCH In review Russ Anderson 2 participants 3 messages May 13, 2008 – May 14, 2008

lore
Share & tools
lore

Russ Anderson

author
#0 lore raw

PATCH 1/3 mm: Minor clean-up of page flags in mm/page_alloc.c v4

Minor source code cleanup of page flags in mm/page_alloc.c. There is no functional change.

Signed-off-by Russ Anderson <[email protected]>
---
 include/linux/page-flags.h |    9 +++++++++
 mm/page_alloc.c            |   34 +++-------------------------------
 2 files changed, 12 insertions(+), 31 deletions(-)

Index: linus/mm/page_alloc.c
=================================--- linus.orig/mm/page_alloc.c 2008-05-12 14:23:07.711484337 -0500
+++ linus/mm/page_alloc.c 2008-05-13 10:26:46.513459253 -0500
@@ -237,16 +237,7 @@ static void bad_page(struct page *page)
printk(KERN_EMERG "Trying to fix it up, but a reboot is needed\n"
KERN_EMERG "Backtrace:\n");
dump_stack();
- page->flags &= ~(1 << PG_lru |
- 1 << PG_private |
- 1 << PG_locked |
- 1 << PG_active |
- 1 << PG_dirty |
- 1 << PG_reclaim |
- 1 << PG_slab |
- 1 << PG_swapcache |
- 1 << PG_writeback |
- 1 << PG_buddy );
+ page->flags &= ~(PAGE_FLAGS_RECLAIM);
set_page_count(page, 0);
reset_page_mapcount(page);
page->mapping = NULL;
@@ -463,16 +454,7 @@ static inline int free_pages_check(struc
(page->mapping != NULL) | (page_get_page_cgroup(page) != NULL) | (page_count(page) != 0) |
- (page->flags & (
- 1 << PG_lru |
- 1 << PG_private |
- 1 << PG_locked |
- 1 << PG_active |
- 1 << PG_slab |
- 1 << PG_swapcache |
- 1 << PG_writeback |
- 1 << PG_reserved |
- 1 << PG_buddy ))))
+ (page->flags & (PAGE_FLAGS_RESERVE))))
bad_page(page);
if (PageDirty(page))
__ClearPageDirty(page);
@@ -616,17 +598,7 @@ static int prep_new_page(struct page *pa
(page->mapping != NULL) | (page_get_page_cgroup(page) != NULL) | (page_count(page) != 0) |
- (page->flags & (
- 1 << PG_lru |
- 1 << PG_private |
- 1 << PG_locked |
- 1 << PG_active |
- 1 << PG_dirty |
- 1 << PG_slab |
- 1 << PG_swapcache |
- 1 << PG_writeback |
- 1 << PG_reserved |
- 1 << PG_buddy ))))
+ (page->flags & (PAGE_FLAGS_DIRTY))))
bad_page(page);

/*
Index: linus/include/linux/page-flags.h
=================================--- linus.orig/include/linux/page-flags.h 2008-05-12 14:23:07.711484337 -0500
+++ linus/include/linux/page-flags.h 2008-05-13 10:26:46.513459253 -0500
@@ -306,5 +306,14 @@ static inline void __ClearPageTail(struc
}

#endif /* !PAGEFLAGS_EXTENDED */
+
+#define PAGE_FLAGS (1 << PG_lru | 1 << PG_private | 1 << PG_locked | \

+			 1 << PG_buddy | 1 << PG_writeback | \
+			 1 << PG_slab  | 1 << PG_swapcache | 1 << PG_active)

+
+#define PAGE_FLAGS_RECLAIM (PAGE_FLAGS | 1 << PG_reclaim | 1 << PG_dirty)
+#define PAGE_FLAGS_RESERVE (PAGE_FLAGS | 1 << PG_reserved)
+#define PAGE_FLAGS_DIRTY (PAGE_FLAGS | 1 << PG_reserved | 1 << PG_dirty)
+
#endif /* !__GENERATING_BOUNDS_H */
#endif /* PAGE_FLAGS_H */

Signature · 2 lines
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]
#1 lore raw
Quoted 2 lines · Russ Anderson

Minor source code cleanup of page flags in mm/page_alloc.c.

Could we (a) make the naming reflect the *use* rather than the flags involved and (b) perhaps add a comment about that use at the point of definition?

Quoted 5 lines
(quoted diff) +0 -1
@@ -237,16 +237,7 @@ static void bad_page(struct page *page)
 	printk(KERN_EMERG "Trying to fix it up, but a reboot is needed\n"
 		KERN_EMERG "Backtrace:\n");
 	dump_stack();
-	page->flags &= ~(1 << PG_lru	|

Review static void bad_page(struct page *page) · line 240

Quoted 10 lines
(quoted diff) +1 -9
-			1 << PG_private |
-			1 << PG_locked	|
-			1 << PG_active	|
-			1 << PG_dirty	|
-			1 << PG_reclaim |
-			1 << PG_slab    |
-			1 << PG_swapcache |
-			1 << PG_writeback |
-			1 << PG_buddy );
+	page->flags &= ~(PAGE_FLAGS_RECLAIM);

Because here I'm now otherwise looking at the code, and I wonder
- why the extra odd parenthesis?
- what does PAGE_FLAG_RECLAIM mean?

and it would be much nicer (I think) if the mask was instead called something that reflected what it was all about, ie something along the lines of PAGE_FLAG_CLEAR_WHEN_BAD instead.

Review static inline int free_pages_check(struc · line 458

Quoted 6 lines
(quoted diff) +1 -0
@@ -463,16 +454,7 @@ static inline int free_pages_check(struc
 		(page->mapping != NULL)  |
 		(page_get_page_cgroup(page) != NULL) |
 		(page_count(page) != 0)  |
+		(page->flags & (PAGE_FLAGS_RESERVE))))
 		bad_page(page);

Same exact thing. I wonder
- why are those parenthesis there
- what does "PAGE_FLAGS_RESERVE" mean?

Woudln't it be much more readable if it was called "PAGE_FLAGS_CHECK_AT_FREE" or something? That says "those flags will be checked when the page is free'd", and now the use _and_ the definition hopefully makes some sense?

Review static int prep_new_page(struct page *pa · line 602

Quoted 6 lines
(quoted diff) +1 -0
@@ -616,17 +598,7 @@ static int prep_new_page(struct page *pa
 		(page->mapping != NULL)  |
 		(page_get_page_cgroup(page) != NULL) |
 		(page_count(page) != 0)  |
+		(page->flags & (PAGE_FLAGS_DIRTY))))
 		bad_page(page);

And again - parenthesis, and perhaps "PAGE_FLAGS_CHECK_AT_PREP"?

Review static int prep_new_page(struct page *pa

Quoted 9 lines
(quoted diff) +9 -0
+++ linus/include/linux/page-flags.h	2008-05-13 10:26:46.513459253 -0500
+
+#define PAGE_FLAGS	(1 << PG_lru   | 1 << PG_private   | 1 << PG_locked | \
+			 1 << PG_buddy | 1 << PG_writeback | \
+			 1 << PG_slab  | 1 << PG_swapcache | 1 << PG_active)
+
+#define PAGE_FLAGS_RECLAIM	(PAGE_FLAGS | 1 << PG_reclaim | 1 << PG_dirty)
+#define PAGE_FLAGS_RESERVE	(PAGE_FLAGS | 1 << PG_reserved)
+#define PAGE_FLAGS_DIRTY	(PAGE_FLAGS | 1 << PG_reserved | 1 << PG_dirty)

This part would also be more explainable - rather than being a random collection of flags, wouldn't it be more natural to think of them as "flags I check at free", or "flags that I clear when they are corrupt" or "flags that I check before I pass on a new page allocation".

Right now it is _totally_ unclear why we should call something PAGE_FLAGS_DIRTY when it has a lot of other bits set than just PG_dirty. Hmm?

Linus

author
#2 lore raw
Quoted 7 lines · Linus Torvalds
Quoted 2 lines · Russ Anderson

Minor source code cleanup of page flags in mm/page_alloc.c.

Could we (a) make the naming reflect the *use* rather than the flags involved and (b) perhaps add a comment about that use at the point of definition?

Yes, will do. The reason for the cleanup is the following patch defines PG_memerror, but only on configs with extended page flags. So checking them in page_alloc.c would require adding #ifdef, which would be rather ugly. Moving the definitions into page-flags.h allows the #ifdef to be in the header file rather than the C code.

From page.discard.v4:
+#ifdef CONFIG_PAGEFLAGS_EXTENDED
+PAGEFLAG(MemError, memerror)
+#define PAGE_FLAGS (PAGE_FLAGS_BASE | 1UL << PG_memerror)
+#else
+PAGEFLAG_FALSE(MemError)
+#define PAGE_FLAGS (PAGE_FLAGS_BASE)
+#endif

Quoted 20 lines
Quoted 5 lines
(quoted diff) +0 -1
@@ -237,16 +237,7 @@ static void bad_page(struct page *page)
 	printk(KERN_EMERG "Trying to fix it up, but a reboot is needed\n"
 		KERN_EMERG "Backtrace:\n");
 	dump_stack();
-	page->flags &= ~(1 << PG_lru	|

Review static void bad_page(struct page *page) · line 240

Quoted 10 lines
(quoted diff) +1 -9
-			1 << PG_private |
-			1 << PG_locked	|
-			1 << PG_active	|
-			1 << PG_dirty	|
-			1 << PG_reclaim |
-			1 << PG_slab    |
-			1 << PG_swapcache |
-			1 << PG_writeback |
-			1 << PG_buddy );
+	page->flags &= ~(PAGE_FLAGS_RECLAIM);

Because here I'm now otherwise looking at the code, and I wonder
- why the extra odd parenthesis?
- what does PAGE_FLAG_RECLAIM mean?

Quoted 3 lines

and it would be much nicer (I think) if the mask was instead called something that reflected what it was all about, ie something along the lines of PAGE_FLAG_CLEAR_WHEN_BAD instead.

I'll make those changes.

Signature · 2 lines
Russ Anderson, OS RAS/Partitioning Project Lead  
SGI - Silicon Graphics Inc          [email protected]