[PATCH 0/3] explicitly document overloaded page flags

Andy Whitcroft

author

2008-05-15 17:19 UTC #0 lore raw

cover explicitly document overloaded page flags

With the recent page flag reorganisation we have a single enum which defines the valid page flags and their values, nice and clear. However there are a number of bits which are overloaded by different subsystems. Firstly there is PG_owner_priv_1 which is used by filesystems and by XEN. Secondly both SLOB and SLUB use a couple of extra page bits to manage internal state for pages they own; both overlay other bits. All of these "aliases" are scattered about the source making it very hard for a reader to know if the bits are safe to rely on in all contexts; confusion here is bad.

As we now have a single place where the bits are clearly assigned it makes sense to clarify the reuse of bits by making the aliases explicit and visible with the original bit assignments. This patch creates explicit aliases within the enum itself for the overloaded bits and uses those aliases throughout.

Andrew please consider for -mm.

-apw

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

Andy Whitcroft

author

2008-05-15 17:19 UTC #1 lore raw

PATCH 1/3 page-flags: record page flag overlays explicitly

Some page flags are used for more than one purpose, for example PG_owner_priv_1. Currently there are individual accessors for each user, each built using the common flag name far away from the bit definitions. This makes it hard to see all possible uses of these bits.

Now that we have a single enum to generate the bit orders it makes sense to express overlays in the same place. So create per use aliases for this bit in the main page-flags enum and use those in the accessors.

Signed-off-by Andy Whitcroft <[email protected]>

---
 include/linux/page-flags.h |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

include/linux/page-flags.h +9 -3

index 590cff3..2cc1fb1 100644
@@ -96,7 +96,13 @@ enum pageflags {
 #ifdef CONFIG_IA64_UNCACHED_ALLOCATOR
 	PG_uncached,		/* Page has been mapped as uncached */
 #endif
-	__NR_PAGEFLAGS
+	__NR_PAGEFLAGS,
+
+	/* Filesystems */
+	PG_checked = PG_owner_priv_1,
+
+	/* XEN */
+	PG_pinned = PG_owner_priv_1,
 };
 
 #ifndef __GENERATING_BOUNDS_H

@@ -155,8 +161,8 @@ PAGEFLAG(Dirty, dirty) TESTSCFLAG(Dirty, dirty) __CLEARPAGEFLAG(Dirty, dirty)
 PAGEFLAG(LRU, lru) __CLEARPAGEFLAG(LRU, lru)
 PAGEFLAG(Active, active) __CLEARPAGEFLAG(Active, active)
 __PAGEFLAG(Slab, slab)
-PAGEFLAG(Checked, owner_priv_1)		/* Used by some filesystems */
-PAGEFLAG(Pinned, owner_priv_1) TESTSCFLAG(Pinned, owner_priv_1) /* Xen */
+PAGEFLAG(Checked, checked)		/* Used by some filesystems */
+PAGEFLAG(Pinned, pinned) TESTSCFLAG(Pinned, pinned) /* Xen */
 PAGEFLAG(Reserved, reserved) __CLEARPAGEFLAG(Reserved, reserved)
 PAGEFLAG(Private, private) __CLEARPAGEFLAG(Private, private)
 	__SETPAGEFLAG(Private, private)

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

Christoph Lameter

2008-05-15 17:28 UTC #2 lore raw

Re: [PATCH 1/3] page-flags: record page flag overlays explicitly

Quoted 3 lines · Andy Whitcroft

Now that we have a single enum to generate the bit orders it makes sense to express overlays in the same place. So create per use aliases for this bit in the main page-flags enum and use those in the accessors.

Well I thought it would be better to have the overlays defined when the PAGEFLAGS_xx macro is used. If that is done then every PG_xxx has a unique id. The aliasing is then only through PageXXXX() using a PG_yyy

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

Andy Whitcroft

author

2008-05-15 17:19 UTC #3 lore raw

PATCH 2/3 slub: record page flag overlays explicitly

In reply to Andy Whitcroft

SLUB reuses two page bits for internal purposes, it overlays PG_active and PG_error. This is hidden away in slub.c. Document these overlays explicitly in the main page-flags enum along with all the others.

Signed-off-by Andy Whitcroft <[email protected]>

---
 include/linux/page-flags.h |    4 ++++
 mm/slub.c                  |    4 ++--
 2 files changed, 6 insertions(+), 2 deletions(-)

include/linux/page-flags.h +4 -0

index 2cc1fb1..2e88df6 100644
@@ -103,6 +103,10 @@ enum pageflags {
 
 	/* XEN */
 	PG_pinned = PG_owner_priv_1,
+
+	/* SLUB */
+	PG_slub_frozen = PG_active,
+	PG_slub_debug = PG_error,
 };
 
 #ifndef __GENERATING_BOUNDS_H

mm/slub.c +2 -2

index a505a82..fd7c61a 100644
@@ -102,10 +102,10 @@
  * 			the fast path and disables lockless freelists.
  */
 
-#define FROZEN (1 << PG_active)
+#define FROZEN (1 << PG_slub_frozen)
 
 #ifdef CONFIG_SLUB_DEBUG
-#define SLABDEBUG (1 << PG_error)
+#define SLABDEBUG (1 << PG_slub_debug)
 #else
 #define SLABDEBUG 0
 #endif

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

Christoph Lameter

2008-05-15 17:29 UTC #4 lore raw

Re: [PATCH 2/3] slub: record page flag overlays explicitly

Quoted 3 lines · Andy Whitcroft

SLUB reuses two page bits for internal purposes, it overlays PG_active and PG_error. This is hidden away in slub.c. Document these overlays explicitly in the main page-flags enum along with all the others.

Hmmm.. Add the definitions also to page-flags.h?

SLABFLAG(Frozen, PG_active)
SLABFLAG(Debug, PG_error)
?

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

KOSAKI Motohiro

2008-05-16 00:21 UTC #5 lore raw

Re: [PATCH 2/3] slub: record page flag overlays explicitly

In reply to Andy Whitcroft

Quoted 5 lines

SLUB reuses two page bits for internal purposes, it overlays PG_active and PG_error. This is hidden away in slub.c. Document these overlays explicitly in the main page-flags enum along with all the others.
Signed-off-by Andy Whitcroft <[email protected]>

Agreed.
I like your approach :)

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

Andy Whitcroft

author

2008-05-15 17:20 UTC #6 lore raw

PATCH 3/3 slob: record page flag overlays explicitly

In reply to Andy Whitcroft

SLOB reuses two page bits for internal purposes, it overlays PG_active and PG_private. This is hidden away in slob.c. Document these overlays explicitly in the main page-flags enum along with all the others.

Signed-off-by Andy Whitcroft <[email protected]>

---
 include/linux/page-flags.h |    4 ++++
 mm/slob.c                  |   12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

include/linux/page-flags.h +4 -0

index 2e88df6..71aec98 100644
@@ -104,6 +104,10 @@ enum pageflags {
 	/* XEN */
 	PG_pinned = PG_owner_priv_1,
 
+	/* SLOB */
+	PG_slob_page = PG_active,
+	PG_slob_free = PG_private,
+
 	/* SLUB */
 	PG_slub_frozen = PG_active,
 	PG_slub_debug = PG_error,

mm/slob.c +6 -6

index 6038cba..9bc3147 100644
@@ -130,17 +130,17 @@ static LIST_HEAD(free_slob_large);
  */
 static inline int slob_page(struct slob_page *sp)
 {
-	return test_bit(PG_active, &sp->flags);
+	return test_bit(PG_slob_page, &sp->flags);
 }
 
 static inline void set_slob_page(struct slob_page *sp)
 {
-	__set_bit(PG_active, &sp->flags);
+	__set_bit(PG_slob_page, &sp->flags);
 }
 
 static inline void clear_slob_page(struct slob_page *sp)
 {
-	__clear_bit(PG_active, &sp->flags);
+	__clear_bit(PG_slob_free, &sp->flags);
 }
 
 /*

@@ -148,19 +148,19 @@ static inline void clear_slob_page(struct slob_page *sp)
  */
 static inline int slob_page_free(struct slob_page *sp)
 {
-	return test_bit(PG_private, &sp->flags);
+	return test_bit(PG_slob_free, &sp->flags);
 }
 
 static void set_slob_page_free(struct slob_page *sp, struct list_head *list)
 {
 	list_add(&sp->list, list);
-	__set_bit(PG_private, &sp->flags);
+	__set_bit(PG_slob_free, &sp->flags);
 }
 
 static inline void clear_slob_page_free(struct slob_page *sp)
 {
 	list_del(&sp->list);
-	__clear_bit(PG_private, &sp->flags);
+	__clear_bit(PG_slob_free, &sp->flags);
 }
 
 #define SLOB_UNIT sizeof(slob_t)

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

Andy Whitcroft

author

2008-05-15 17:37 UTC #7 lore raw

Re: [PATCH 3/3] slob: record page flag overlays explicitly

mm/slob.c static LIST_HEAD(free_slob_large); · line 144

Quoted 5 lines · Andy Whitcroft

static inline void clear_slob_page(struct slob_page *sp) {
mm/slob.c +1 -1
-	__clear_bit(PG_active, &sp->flags);
+	__clear_bit(PG_slob_free, &sp->flags);
 }

Bah, this hunk is wrong. Seems this is not the latest version. Will replace this patch momentarily.

-apw

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

Andy Whitcroft

author

2008-05-15 17:42 UTC #8 lore raw

PATCH 1/1 slob: record page flag overlays explicitly v2

In reply to Andy Whitcroft

SLOB reuses two page bits for internal purposes, it overlays PG_active and PG_private. This is hidden away in slob.c. Document these overlays explicitly in the main page-flags enum along with all the others.

Signed-off-by Andy Whitcroft <[email protected]>

---
 include/linux/page-flags.h |    4 ++++
 mm/slob.c                  |   12 ++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

include/linux/page-flags.h +4 -0

index 2e88df6..71aec98 100644
@@ -104,6 +104,10 @@ enum pageflags {
 	/* XEN */
 	PG_pinned = PG_owner_priv_1,
 
+	/* SLOB */
+	PG_slob_page = PG_active,
+	PG_slob_free = PG_private,
+
 	/* SLUB */
 	PG_slub_frozen = PG_active,
 	PG_slub_debug = PG_error,

mm/slob.c +6 -6

index 6038cba..0aaf54f 100644
@@ -130,17 +130,17 @@ static LIST_HEAD(free_slob_large);
  */
 static inline int slob_page(struct slob_page *sp)
 {
-	return test_bit(PG_active, &sp->flags);
+	return test_bit(PG_slob_page, &sp->flags);
 }
 
 static inline void set_slob_page(struct slob_page *sp)
 {
-	__set_bit(PG_active, &sp->flags);
+	__set_bit(PG_slob_page, &sp->flags);
 }
 
 static inline void clear_slob_page(struct slob_page *sp)
 {
-	__clear_bit(PG_active, &sp->flags);
+	__clear_bit(PG_slob_page, &sp->flags);
 }
 
 /*

@@ -148,19 +148,19 @@ static inline void clear_slob_page(struct slob_page *sp)
  */
 static inline int slob_page_free(struct slob_page *sp)
 {
-	return test_bit(PG_private, &sp->flags);
+	return test_bit(PG_slob_free, &sp->flags);
 }
 
 static void set_slob_page_free(struct slob_page *sp, struct list_head *list)
 {
 	list_add(&sp->list, list);
-	__set_bit(PG_private, &sp->flags);
+	__set_bit(PG_slob_free, &sp->flags);
 }
 
 static inline void clear_slob_page_free(struct slob_page *sp)
 {
 	list_del(&sp->list);
-	__clear_bit(PG_private, &sp->flags);
+	__clear_bit(PG_slob_free, &sp->flags);
 }
 
 #define SLOB_UNIT sizeof(slob_t)

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

Christoph Lameter

2008-05-15 17:26 UTC #9 lore raw

In reply to Andy Whitcroft

Quoted 10 lines · Andy Whitcroft

With the recent page flag reorganisation we have a single enum which defines the valid page flags and their values, nice and clear. However there are a number of bits which are overloaded by different subsystems. Firstly there is PG_owner_priv_1 which is used by filesystems and by XEN. Secondly both SLOB and SLUB use a couple of extra page bits to manage internal state for pages they own; both overlay other bits. All of these "aliases" are scattered about the source making it very hard for a reader to know if the bits are safe to rely on in all contexts; confusion here is bad.

Quoted 5 lines

As we now have a single place where the bits are clearly assigned it makes sense to clarify the reuse of bits by making the aliases explicit and visible with the original bit assignments. This patch creates explicit aliases within the enum itself for the overloaded bits and uses those aliases throughout.

Ahh. Great! I considered doing that work too but never got around to it.

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