2007-01-27 08:24:28

by Wu Fengguang

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

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 <[email protected]>
---
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;
}

/*

--

2007-01-27 12:07:48

by Bodo Eggert

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

Fengguang Wu <[email protected]> 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: [email protected]
[email protected] [email protected]

2007-01-28 19:15:54

by Randy Dunlap

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

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

> Fengguang Wu <[email protected]> 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