page->flags cleanup

page->flags cleanup

Post by Andrew Morto » Thu, 25 Apr 2002 18:00:17



Moves the definitions of the page->flags bits and all the PageFoo
macros into linux/page-flags.h.  That file is currently included from
mm.h, but the stage is set to remove that and include page-flags.h
direct in all .c files which require that.  (120 of them).

The patch also makes all the page flag macros and functions consistent:

For PG_foo, the following functions are defined:

        SetPageFoo
        ClearPageFoo
        TestSetPageFoo
        TestClearPageFoo
        PageFoo

and that's it.

- Page_Uptodate is renamed to PageUptodate

- LockPage is removed.  All users updated to use SetPageLocked

- UnlockPage is removed.  All callers updated to use unlock_page().
  it's a real function - there's no need to hide that fact.

- PageTestandClearReferenced renamed to TestClearPageReferenced

- PageSetSlab renamed to SetPageSlab

- __SetPageReserved is removed.  It's an infinitesimally small
   microoptimisation, and is inconsistent.

- TryLockPage is renamed to TestSetPageLocked

- PageSwapCache() is renamed to page_swap_cache(), so it doesn't
  pretend to be a page->flags bit test.

=====================================

--- 2.5.9/include/linux/page-flags.h~cleanup-010-page_flags     Wed Apr 24 01:06:11 2002
+++ 2.5.9-akpm/include/linux/page-flags.h       Wed Apr 24 01:44:56 2002
@@ -6,6 +6,72 @@
 #define PAGE_FLAGS_H

 /*
+ * Various page->flags bits:
+ *
+ * PG_reserved is set for special pages, which can never be swapped
+ * out. Some of them might not even exist (eg empty_bad_page)...
+ *
+ * The PG_private bitflag is set if page->private contains a valid value.
+ *
+ * During disk I/O, PG_locked_dontuse is used. This bit is set before I/O
+ * and reset when I/O completes. page_waitqueue(page) is a wait queue of all
+ * tasks waiting for the I/O on this page to complete.
+ *
+ * PG_uptodate tells whether the page's contents is valid.
+ * When a read completes, the page becomes uptodate, unless a disk I/O
+ * error happened.
+ *
+ * For choosing which pages to swap out, inode pages carry a
+ * PG_referenced bit, which is set any time the system accesses
+ * that page through the (mapping,index) hash table. This referenced
+ * bit, together with the referenced bit in the page tables, is used
+ * to manipulate page->age and move the page across the active,
+ * inactive_dirty and inactive_clean lists.
+ *
+ * Note that the referenced bit, the page->lru list_head and the
+ * active, inactive_dirty and inactive_clean lists are protected by
+ * the pagemap_lru_lock, and *NOT* by the usual PG_locked_dontuse bit!
+ *
+ * PG_skip is used on sparc/sparc64 architectures to "skip" certain
+ * parts of the address space.
+ *
+ * PG_error is set to indicate that an I/O error occurred on this page.
+ *
+ * PG_arch_1 is an architecture specific page state bit.  The generic
+ * code guarantees that this bit is cleared for a page when it first
+ * is entered into the page cache.
+ *
+ * PG_highmem pages are not permanently mapped into the kernel virtual
+ * address space, they need to be kmapped separately for doing IO on
+ * the pages. The struct page (these bits with information) are always
+ * mapped into kernel address space...
+ */
+
+/*
+ * Don't use the *_dontuse flags.  Use the macros.  Otherwise
+ * you'll break locked- and dirty-page accounting.
+ */
+#define PG_locked_dontuse       0      /* Page is locked. Don't touch. */
+#define PG_error                1
+#define PG_referenced           2
+#define PG_uptodate             3
+
+#define PG_dirty_dontuse        4
+#define PG_unused               5      /* err.  This is unused. */
+#define PG_lru                  6
+#define PG_active               7
+
+#define PG_slab                         8      /* slab debug (Suparna wants this) */
+#define PG_skip                        10      /* kill me now: obsolete */
+#define PG_highmem             11
+#define PG_checked             12      /* kill me in 2.5.<early>. */
+
+#define PG_arch_1              13
+#define PG_reserved            14
+#define PG_launder             15      /* written out by VM pressure.. */
+#define PG_private             16      /* Has something at ->private */
+
+/*
  * Per-CPU page acounting.  Inclusion of this file requires
  * that <linux/percpu.h> be included beforehand.
  */
@@ -35,7 +101,6 @@ extern void get_page_state(struct page_s
 /*
  * Manipulation of page state flags
  */
-#define UnlockPage(page)       unlock_page(page)
 #define PageLocked(page)       test_bit(PG_locked_dontuse, &(page)->flags)
 #define SetPageLocked(page)                                            \
        do {                                                            \
@@ -43,8 +108,7 @@ extern void get_page_state(struct page_s
                                &(page)->flags))                 \
                        inc_page_state(nr_locked);                      \
        } while (0)
-#define LockPage(page)         SetPageLocked(page)     /* grr.  kill me */
-#define TryLockPage(page)                                              \
+#define TestSetPageLocked(page)                                                \
        ({                                                              \
                int ret;                                                \
                ret = test_and_set_bit(PG_locked_dontuse,               \
@@ -76,9 +140,9 @@ extern void get_page_state(struct page_s
 #define PageReferenced(page)   test_bit(PG_referenced, &(page)->flags)
 #define SetPageReferenced(page)        set_bit(PG_referenced, &(page)->flags)
 #define ClearPageReferenced(page)      clear_bit(PG_referenced, &(page)->flags)
-#define PageTestandClearReferenced(page)       test_and_clear_bit(PG_referenced, &(page)->flags)
+#define TestClearPageReferenced(page) test_and_clear_bit(PG_referenced, &(page)->flags)

-#define Page_Uptodate(page)    test_bit(PG_uptodate, &(page)->flags)
+#define PageUptodate(page)     test_bit(PG_uptodate, &(page)->flags)
 #define SetPageUptodate(page)  set_bit(PG_uptodate, &(page)->flags)
 #define ClearPageUptodate(page)        clear_bit(PG_uptodate, &(page)->flags)

@@ -123,8 +187,8 @@ extern void get_page_state(struct page_s
 #define ClearPageActive(page)  clear_bit(PG_active, &(page)->flags)

 #define PageSlab(page)         test_bit(PG_slab, &(page)->flags)
-#define PageSetSlab(page)      set_bit(PG_slab, &(page)->flags)
-#define PageClearSlab(page)    clear_bit(PG_slab, &(page)->flags)
+#define SetPageSlab(page)      set_bit(PG_slab, &(page)->flags)
+#define ClearPageSlab(page)    clear_bit(PG_slab, &(page)->flags)

 #ifdef CONFIG_HIGHMEM
 #define PageHighMem(page)      test_bit(PG_highmem, &(page)->flags)
@@ -138,7 +202,6 @@ extern void get_page_state(struct page_s
 #define PageReserved(page)     test_bit(PG_reserved, &(page)->flags)
 #define SetPageReserved(page)  set_bit(PG_reserved, &(page)->flags)
 #define ClearPageReserved(page)        clear_bit(PG_reserved, &(page)->flags)
-#define __SetPageReserved(page)        __set_bit(PG_reserved, &(page)->flags)

 #define PageLaunder(page)      test_bit(PG_launder, &(page)->flags)
 #define SetPageLaunder(page)   set_bit(PG_launder, &(page)->flags)
--- 2.5.9/include/linux/mm.h~cleanup-010-page_flags     Wed Apr 24 01:06:11 2002
+++ 2.5.9-akpm/include/linux/mm.h       Wed Apr 24 01:45:02 2002
@@ -191,11 +191,6 @@ typedef struct page {
 #define set_page_count(p,v)    atomic_set(&(p)->count, v)

 /*
- * Various page->flags bits:
- *
- * PG_reserved is set for special pages, which can never be swapped
- * out. Some of them might not even exist (eg empty_bad_page)...
- *
  * Multiple processes may "see" the same page. E.g. for untouched
  * mappings of /dev/null, all processes see the same page full of
  * zeroes, and text pages of executables and shared libraries have
@@ -224,8 +219,6 @@ typedef struct page {
  * page's address_space.  Usually, this is the address of a circular
  * list of the page's disk buffers.
  *
- * The PG_private bitflag is set if page->private contains a valid
- * value.
  * For pages belonging to inodes, the page->count is the number of
  * attaches, plus 1 if `private' contains something, plus one for
  * the page cache itself.
@@ -244,62 +237,7 @@ typedef struct page {
  *   to be written to disk,
  * - private pages which have been modified may need to be swapped out
  *   to swap space and (later) to be read back into memory.
- * During disk I/O, PG_locked_dontuse is used. This bit is set before I/O
- * and reset when I/O completes. page_waitqueue(page) is a wait queue of all
- * tasks waiting for the I/O on this page to complete.
- * PG_uptodate tells whether the page's contents is valid.
- * When a read completes, the page becomes uptodate, unless a disk I/O
- * error happened.
- *
- * For choosing which pages to swap out, inode pages carry a
- * PG_referenced bit, which is set any time the system accesses
- * that page through the (mapping,index) hash table. This referenced
- * bit, together with the referenced bit in the page tables, is used
- * to manipulate page->age and move the page across the active,
- * inactive_dirty and inactive_clean lists.
- *
- * Note that the referenced bit, the page->lru list_head and the
- * active, inactive_dirty and inactive_clean lists are protected by
- * the pagemap_lru_lock, and *NOT* by the usual PG_locked_dontuse bit!
- *
- * PG_skip is used on sparc/sparc64 architectures to "skip" certain
- * parts of the address space.
- *
- * PG_error is set to indicate that an I/O error occurred on this page.
- *
- * PG_arch_1 is an architecture specific page state bit.  The generic
- * code guarantees that this bit is cleared for a page when it first
- * is entered into the page cache.
- *
- * PG_highmem pages are not permanently mapped into the kernel virtual
- * address space, they need to be kmapped separately for doing IO on
- * the pages. The struct page (these bits with information) are always
- * mapped into kernel address space...
- */
-
-/*
- * Don't use the *_dontuse flags.  Use the macros.  Otherwise
- * you'll break locked- and dirty-page accounting.
  */
-#define PG_locked_dontuse       0      /* Page is locked. Don't touch. */
-#define PG_error                1
-#define PG_referenced           2
-#define PG_uptodate             3
-
-#define PG_dirty_dontuse        4
-#define PG_unused               5      /* err.  This is unused. */
-#define PG_lru                  6
-#define PG_active               7
-
-#define PG_slab                         8      /* slab debug (Suparna wants this) */
-#define PG_skip                        10      /* kill me now: obsolete */
-#define PG_highmem             11
-#define PG_checked             12      /* kill me in 2.5.<early>. */
-
-#define PG_arch_1              13
-#define PG_reserved            14
-#define PG_launder             15      /* written out by VM pressure.. */
-#define PG_private             16      /* Has something at ->private */

 /*
  * FIXME: take this include out, include page-flags.h in
@@ -445,12 +383,7 ...

read more »

 
 
 

page->flags cleanup

Post by Hugh Dickin » Fri, 26 Apr 2002 04:30:13



> The patch also makes all the page flag macros and functions consistent:

> - Page_Uptodate is renamed to PageUptodate
> - LockPage is removed.  All users updated to use SetPageLocked

Good!

Quote:> - UnlockPage is removed.  All callers updated to use unlock_page().
>   it's a real function - there's no need to hide that fact.

Hmm, well, I'd prefer not to change that very widely used name;
but I've no serious objection if you wish to.

Quote:> - PageTestandClearReferenced renamed to TestClearPageReferenced
> - PageSetSlab renamed to SetPageSlab
> - __SetPageReserved is removed.  It's an infinitesimally small
>    microoptimisation, and is inconsistent.
> - TryLockPage is renamed to TestSetPageLocked

Good!  I especially hate trying to guess the return value of "Try"s.

Quote:> - PageSwapCache() is renamed to page_swap_cache(), so it doesn't
>   pretend to be a page->flags bit test.

Again, I'd prefer to leave PageSwapCache as is: it used to have a
page->flags bit, it might be given a page->flags bit again in future
(multiple swapper_spaces?).  I don't think "Page" in the macro name
should have to imply implementation using a page->flags bit.  But
again, I've no serious objection if you wish to make this change.

However, I again want to do that irritating thing I do, Andrew,
propose some related cleanups I'd noticed before and was waiting
for a suitable moment to make.  If these catch your fancy, please
include, else ignore.

1. The two "if (PageSwapCache(page)) BUG();"s  in mm/page_alloc.c
   are redundant and should just be deleted rather than converted:
   we have just checked that page->mapping is unset, so (excepting
   a volatile mod of a kind which would need an infinite number of
   identical tests to protect against) of course it isn't &swapper_space
   (but the compiler doesn't optimize away).  The two PageSwapCache BUG
   tests in mm/page_io.c similarly redundant and should also be deleted.

2. I can't see from your mail (patch against an earlier version?) what
   happened to the comment immediately above #define PageError(page)
   in 2.5.9/include/linux/mm.h - the comment beginning "The first mb".
   That comment originally belonged to UnlockPage(), and should have
   been moved to unlock_page() when that went into mm/filemap.c.
   I sometimes wonder whether those two "mb"s are actually still
   required (quite a lot has changed since they went in), but that's
   a different kind of question, and certainly not one I can answer.

3. You're removing PG_skip and shifting highers down (in patch you
   mailed separately): good, but please remove PG_unused too and shift
   highers down (I cautiously renamed PG_swap_cache to PG_unused when
   changing PageSwapCache macro, the time for that caution has past).

4. You've updated arch/m68k/atari/stram.c.  I'd prefer all traces of
   CONFIG_STRAM_SWAP be removed from m68k's Config.help, config.in
   and stram.c.  If that code even compiles (I never tried), it is
   using at least one function (shm_unuse) which didn't even exist in
   2.4.0, and has not been maintained to match all the swap changes
   which have gone on since.  What it appears to be doing (using
   slow RAM for swap) is sensible enough, but shouldn't that be done
   using the standard mm swap code (with extensions if necessary)
   on some atari/stram block device giving access to the memory?
   But obviously nobody has configured CONFIG_STRAM_SWAP y since
   2.4.0, so providing such functionality doesn't seem to be a high
   priority; and I'd prefer all that out-of-date stram.c swap code
   not to show up when we grep the source tree for such things.  Jes?

Hugh

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

page->flags cleanup

Post by Rik van Rie » Fri, 26 Apr 2002 05:20:07



> Moves the definitions of the page->flags bits and all the PageFoo
> macros into linux/page-flags.h.  That file is currently included from
> mm.h, but the stage is set to remove that and include page-flags.h
> direct in all .c files which require that.  (120 of them).

I like this patch a lot.  It's definately the right time to
clean up some of the years old cruft.

regards,

Rik
--
Bravely reimplemented by the knights who say "NIH".

http://www.surriel.com/             http://distro.conectiva.com/

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

page->flags cleanup

Post by Andrew Morto » Fri, 26 Apr 2002 05:30:09



> ...
> > - UnlockPage is removed.  All callers updated to use unlock_page().
> >   it's a real function - there's no need to hide that fact.

> Hmm, well, I'd prefer not to change that very widely used name;
> but I've no serious objection if you wish to.

UnlockPage() looks like a simple clear_bit(), but it very much isn't.
I think it's better this way.

Quote:> ...

> > - PageSwapCache() is renamed to page_swap_cache(), so it doesn't
> >   pretend to be a page->flags bit test.

> Again, I'd prefer to leave PageSwapCache as is: it used to have a
> page->flags bit, it might be given a page->flags bit again in future
> (multiple swapper_spaces?).  I don't think "Page" in the macro name
> should have to imply implementation using a page->flags bit.  But
> again, I've no serious objection if you wish to make this change.

OK, I re-renamed the predicate back to PageSwapCache, added some
commentary about this.

Quote:> ...

> 1. The two "if (PageSwapCache(page)) BUG();"s  in mm/page_alloc.c
>    are redundant and should just be deleted rather than converted:
>    we have just checked that page->mapping is unset, so (excepting
>    a volatile mod of a kind which would need an infinite number of
>    identical tests to protect against) of course it isn't &swapper_space
>    (but the compiler doesn't optimize away).  The two PageSwapCache BUG
>    tests in mm/page_io.c similarly redundant and should also be deleted.

OK, I've done this in a separate patch.

Quote:> 2. I can't see from your mail (patch against an earlier version?) what
>    happened to the comment immediately above #define PageError(page)
>    in 2.5.9/include/linux/mm.h - the comment beginning "The first mb".
>    That comment originally belonged to UnlockPage(), and should have
>    been moved to unlock_page() when that went into mm/filemap.c.
>    I sometimes wonder whether those two "mb"s are actually still
>    required (quite a lot has changed since they went in), but that's
>    a different kind of question, and certainly not one I can answer.

I moved the comment to unlock_page().

Quote:> 3. You're removing PG_skip and shifting highers down (in patch you
>    mailed separately): good, but please remove PG_unused too and shift
>    highers down (I cautiously renamed PG_swap_cache to PG_unused when
>    changing PageSwapCache macro, the time for that caution has past).

I wondered what that was doing there ;)  Done.

Updated patch series (compiles, untested, should be OK) is at
http://www.zip.com.au/~akpm/linux/patches/2.5/2.5.10/

Thanks.

Sometime I'll bite the bullet and actually change all .c
files to include page-flags.h direct.  I did that yesterday
but wasn't happy with it.  There are some unfortunate dependencies
in pagemap.h and highmem.h which need cleaning up first.

-
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

page->flags cleanup

Post by Christoph Hellwi » Fri, 26 Apr 2002 22:00:16



> Moves the definitions of the page->flags bits and all the PageFoo
> macros into linux/page-flags.h.  That file is currently included from
> mm.h, but the stage is set to remove that and include page-flags.h
> direct in all .c files which require that.  (120 of them).

What about moving not only the flags but also struct page itself to
<linux/page.h>?  I think they belong together and don't have much shared
with the other bits of mm.h, so this makes some sense.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

1. ATA: cleanup of channel->autodma flags usage

Bartlomiej> incremental to generic ATA PCI auto-dma patches...
[...]
Bartlomiej>  - remove ATA_F_NOAUTODMA flag from aec62xx.c, hpt34x.c,
Bartlomiej>    sis5513.c and via82cxxx.c drivers, it's use was bogus
Bartlomiej>    in these drivers

Bartlomiej>    only two usages of ATA_F_NOAUTODMA are left (in ide-pci.c),
Bartlomiej>    probably they can alse be removed due to fact that drivers
Bartlomiej>    should disable autodma not ide-pci (i.e. hpt34x)
[...]

That should say: ATA_F_NOADMA

I have removed ATA_F_NODMA in ide-pci.c for my VIA8233
(PCI_DEVICE_ID_VIA_82C586_1). So far it has not failed (using 2.5.20).

--- linux-2.5.20/drivers/ide/ide-pci.c~ Mon Jun  3 14:49:59 2002

        {
                vendor: PCI_VENDOR_ID_VIA,
                device: PCI_DEVICE_ID_VIA_82C586_1,
-               bootable: ON_BOARD,
-               flags: ATA_F_NOADMA
+               bootable: ON_BOARD
        },
        {
                vendor: PCI_VENDOR_ID_TTI,

- Kees
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

2. Changing disks

3. remove mixture of non-atomic operations with page->flags which requires atomic operations to access

4. Router for users

5. extra PG_* bits for page->flags

6. installing with a pci graphics matrox card

7. always update page->flags atomically

8. korn shell script

9. [CLEANUP] task->state cleanups part 9

10. [CLEANUP] task->state cleanups part 7

11. [CLEANUP] task->state cleanups part 6

12. [CLEANUP] task->state cleanups part 3

13. [CLEANUP] task->state cleanups part 4