Re: [PATCH 2/8] readahead: state based method routines: explicitly embed class_new/class_old inside flags

Re: [PATCH 2/8] readahead: state based method routines: explicitly embed class_new/class_old inside flags

[Bodo Eggert]

Fengguang Wu <wfg@mail.ustc.edu.cn> wrote:

> The new/old ra class were implicitly stored in low bits of
> file_ra_state.flags. Now make the data structure obvious, and remove the
> coding tricks.

> +++ linux-2.6.20-rc4-mm1/include/linux/fs.h

> -     unsigned long flags;    /* RA_FLAG_xxx | ra_class_old | ra_class_new */
> +     union {
> +             unsigned long   flags;  /* RA_FLAG_xxx | class_old | class_new */
> +             struct {
> +                     u8      class_new;
> +                     u8      class_old;

This doesn't work on big endiam machines, does it?

IMO you should use a bitfield instead.
-- 
Your e-mail has been returned due to insufficient voltage. 

Friß, Spammer: aegzcH4Js@luevh-7.7eggert.dyndns.org
 ptulV7@C.7eggert.dyndns.org vkIfyB@SEcltba.7eggert.dyndns.org

Re: [PATCH 2/8] readahead: state based method routines: explicitly embed class_new/class_old inside flags

[Randy Dunlap]

On Sat, 27 Jan 2007 13:01:08 +0100 Bodo Eggert wrote:

> Fengguang Wu <wfg@mail.ustc.edu.cn> wrote:
> 
> > The new/old ra class were implicitly stored in low bits of
> > file_ra_state.flags. Now make the data structure obvious, and remove the
> > coding tricks.
> 
> > +++ linux-2.6.20-rc4-mm1/include/linux/fs.h
> 
> > -     unsigned long flags;    /* RA_FLAG_xxx | ra_class_old | ra_class_new */
> > +     union {
> > +             unsigned long   flags;  /* RA_FLAG_xxx | class_old | class_new */
> > +             struct {
> > +                     u8      class_new;
> > +                     u8      class_old;
> 
> This doesn't work on big endiam machines, does it?
> 
> IMO you should use a bitfield instead.

Or "use bit masks instead" since

"The order of allocation of bit-fields within a unit (high-order to
low-order or low-order to high-order) is implementation-defined."

---
~Randy

[PATCH 2/8] readahead: state based method routines: explicitly embed class_new/class_old inside flags

[Fengguang Wu]

[-- Attachment #1: readahead-state-based-method-routines-explicitly-embed-class_new-class_old-inside-flags.patch --]
[-- Type: text/plain, Size: 2155 bytes --]

The new/old ra class were implicitly stored in low bits of file_ra_state.flags.
Now make the data structure obvious, and remove the coding tricks.

Signed-off-by: Fengguang Wu <wfg@mail.ustc.edu.cn>
---
 include/linux/fs.h |    9 ++++++++-
 mm/readahead.c     |   18 ++++--------------
 2 files changed, 12 insertions(+), 15 deletions(-)

--- linux-2.6.20-rc4-mm1.orig/include/linux/fs.h
+++ linux-2.6.20-rc4-mm1/include/linux/fs.h
@@ -738,7 +738,14 @@ struct file_ra_state {
 	unsigned long mmap_hit;		/* Cache hit stat for mmap accesses */
 	unsigned long mmap_miss;	/* Cache miss stat for mmap accesses */
 
-	unsigned long flags;	/* RA_FLAG_xxx | ra_class_old | ra_class_new */
+	union {
+		unsigned long	flags;	/* RA_FLAG_xxx | class_old | class_new */
+		struct {
+			u8	class_new;
+			u8	class_old;
+		};
+	};
+
 	unsigned long prev_page;	/* Cache last read() position */
 	unsigned long ra_pages;		/* Maximum readahead window */
 };
--- linux-2.6.20-rc4-mm1.orig/mm/readahead.c
+++ linux-2.6.20-rc4-mm1/mm/readahead.c
@@ -48,8 +48,6 @@ int readahead_hit_rate = 1;
 /*
  * Detailed classification of read-ahead behaviors.
  */
-#define RA_CLASS_SHIFT 4
-#define RA_CLASS_MASK  ((1 << RA_CLASS_SHIFT) - 1)
 enum ra_class {
 	RA_CLASS_ALL,
 	RA_CLASS_INITIAL,
@@ -795,12 +793,12 @@ out:
 
 static enum ra_class ra_class_new(struct file_ra_state *ra)
 {
-	return ra->flags & RA_CLASS_MASK;
+	return ra->class_new;
 }
 
 static inline enum ra_class ra_class_old(struct file_ra_state *ra)
 {
-	return (ra->flags >> RA_CLASS_SHIFT) & RA_CLASS_MASK;
+	return ra->class_old;
 }
 
 static unsigned long ra_readahead_size(struct file_ra_state *ra)
@@ -832,16 +830,8 @@ static int ra_has_index(struct file_ra_s
  */
 static void ra_set_class(struct file_ra_state *ra, enum ra_class ra_class)
 {
-	unsigned long flags_mask;
-	unsigned long flags;
-	unsigned long old_ra_class;
-
-	flags_mask = ~(RA_CLASS_MASK | (RA_CLASS_MASK << RA_CLASS_SHIFT));
-	flags = ra->flags & flags_mask;
-
-	old_ra_class = ra_class_new(ra) << RA_CLASS_SHIFT;
-
-	ra->flags = flags | old_ra_class | ra_class;
+	ra->class_old = ra->class_new;
+	ra->class_new = ra_class;
 }
 
 /*

--