Re: page-flags.h

Re: page-flags.h

[Christoph Hellwig]

On Wed, May 01, 2002 at 07:27:37PM +0200, Dave Jones wrote:
> A new header file appeared in 2.5.12, include/linux/page-flags.h
> Currently, there are many places that need this, but instead of
> including it, they are including <linux/mm.h> which in turn sucks
> in zillions of other files.
> 
> According to a comment in mm.h, there are 119 places that need
> fixing here.
> 
> Method:
> o  Remove the #include <linux/page-flags.h> from mm.h
> o  Compile, and see what breaks.
> o  Add #include <linux/page-flags.h> to file compilation died on.
> o  Try removing the <linux/mm.h> include also, but this may not
>    be possible in all circumstances.

This step is wasted work - it will NEVER compile.  Rationale:
the page flags operate on page->flags and without having the definition
of struct page from mm.h this won't do.

The better idea is IMHO to replace page-flags.h by page.h that also
contains the definition of struct page.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Dave Jones]

On Wed, May 01, 2002 at 06:34:14PM +0100, Christoph Hellwig wrote:
 > This step is wasted work - it will NEVER compile.  Rationale:
 > the page flags operate on page->flags and without having the definition
 > of struct page from mm.h this won't do.
 > 
 > The better idea is IMHO to replace page-flags.h by page.h that also
 > contains the definition of struct page.

That's a good point, and something I completley overlooked.
I wonder if Andrew Morton (who I'm guessing wrote that comment
in mm.h) has some ingenious plan here..

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Andrew Morton]

Dave Jones wrote:
> 
> On Wed, May 01, 2002 at 06:34:14PM +0100, Christoph Hellwig wrote:
>  > This step is wasted work - it will NEVER compile.  Rationale:
>  > the page flags operate on page->flags and without having the definition
>  > of struct page from mm.h this won't do.
>  >
>  > The better idea is IMHO to replace page-flags.h by page.h that also
>  > contains the definition of struct page.
> 
> That's a good point, and something I completley overlooked.
> I wonder if Andrew Morton (who I'm guessing wrote that comment
> in mm.h) has some ingenious plan here..

who, me?

I'd envisaged those 119 files doing:

#include <linux/mm.h>
#include <linux/page-flags.h>

so then anything which includes mm.h but doesn't do any PageFoo()
operations doesn't have to process those macros.

I actually did those 119 edits, but dumped it - there are some
awkward forward, backward and sideward refs in pagemap.h and
highmem.h which need to be fixed up first.  umm..  Move
wait_on_page_locked() into page-flags.h and uninline bio_kmap_irq().

Also, moving bh_kmap(), bh_kunmap() and bh_offset() down into 
their only user, raid5.c will help solve a few ordering nasties.

The other low-hanging fruit here is pulling buffer_head.h
out of fs.h.  But as with page-flags.h, the first step
should be to sort out the .h files which refer to buffers,
then to do .c.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Christoph Hellwig]

On Thu, May 02, 2002 at 07:52:40PM -0700, Andrew Morton wrote:
> > That's a good point, and something I completley overlooked.
> > I wonder if Andrew Morton (who I'm guessing wrote that comment
> > in mm.h) has some ingenious plan here..
> 
> who, me?
> 
> I'd envisaged those 119 files doing:
> 
> #include <linux/mm.h>
> #include <linux/page-flags.h>
> 
> so then anything which includes mm.h but doesn't do any PageFoo()
> operations doesn't have to process those macros.

Okay, that makes some sense.  I still think it's preferrable to
have <linux/page.h>  - many filesystems only need struct page, the
flags and few supporting functions, so do drivers using kiobufs.

Having these no need the rest of the MM internals is a good thing (TM).

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Erik van Konijnenburg]

Hi Andrew,

Do you really have to edit 119 files if you just want to avoid
processing the PageFoo macros?  Include page-flags.h in pagemap.h,
and you only have to add include lines to 13 files to get the kernel
compiled, while still getting rid of 1050 times reading of page-flags.h.

Motivation:
    --	linux/mm.h is included in 1163 files
    --	linux/pagemap.h is included in only 109 files.
    --	In pagemap.h, wait_on_page_locked() and PageLocked()
	are mixed rather awkwardly.  Moving wait_on_page_locked()
	to page-flags.h as you suggested doesn't really help:
	you'd have to move the ___wait_on_page_locked declaration
	as well, resulting in other ugliness, so we might as well
	include page-flags.h.
    --	most of the added includes are for page-flags.h, so it makes
	sense not to merge page-flags.h and pagemap.h.
    --	yes, the number 1050 is wrong, since pagemap.h is also
	included indirectly.  I'm not sufficiently familiar with
	the kernel to make a better estimate, so this whole
	thing may be a red herring.

Please don't regard the attached diffs as a patch, more as a suggestion
of what a next intermediate step could look like.  In particular,
putting page_state in page-state.h is bogus, something left over
from earlier experiments.  No idea if it actually works, but at least it
compiles with my limited set of modules.

Of course this does not affect Christoph's remarks regarding 
the need for page.h one way or the other.

Regards,
Erik

> On Thu, May 02, 2002 at 07:52:40PM -0700, Andrew Morton wrote:
> > > That's a good point, and something I completley overlooked.
> > > I wonder if Andrew Morton (who I'm guessing wrote that comment
> > > in mm.h) has some ingenious plan here..
> > 
> > who, me?
> > 
> > I'd envisaged those 119 files doing:
> > 
> > #include <linux/mm.h>
> > #include <linux/page-flags.h>
> > 
> > so then anything which includes mm.h but doesn't do any PageFoo()
> > operations doesn't have to process those macros.
> 
> Okay, that makes some sense.  I still think it's preferrable to
> have <linux/page.h>  - many filesystems only need struct page, the
> flags and few supporting functions, so do drivers using kiobufs.
> 
> Having these no need the rest of the MM internals is a good thing (TM).

--- linux-2.5.13/arch/i386/mm/ioremap.c.org	Fri May  3 23:19:01 2002
+++ linux-2.5.13/arch/i386/mm/ioremap.c	Fri May  3 23:19:18 2002
@@ -9,6 +9,7 @@
  */
 
 #include <linux/vmalloc.h>
+#include <linux/page-flags.h>
 #include <asm/io.h>
 #include <asm/pgalloc.h>
 #include <asm/fixmap.h>
--- linux-2.5.13/include/linux/mm.h.org	Fri May  3 21:33:04 2002
+++ linux-2.5.13/include/linux/mm.h	Fri May  3 22:01:49 2002
@@ -243,7 +243,7 @@
  * FIXME: take this include out, include page-flags.h in
  * files which need it (119 of them)
  */
-#include <linux/page-flags.h>
+/* #include <linux/page-flags.h> */
 
 /*
  * The zone field is never updated after free_area_init_core()
--- linux-2.5.13/include/linux/page-flags.h.org	Fri May  3 21:58:20 2002
+++ linux-2.5.13/include/linux/page-flags.h	Fri May  3 22:44:42 2002
@@ -5,6 +5,8 @@
 #ifndef PAGE_FLAGS_H
 #define PAGE_FLAGS_H
 
+#include <linux/page-state.h>
+
 /*
  * Various page->flags bits:
  *
@@ -67,28 +69,6 @@
 #define PG_writeback		14	/* Page is under writeback */
 
 /*
- * Global page accounting.  One instance per CPU.
- */
-extern struct page_state {
-	unsigned long nr_dirty;
-	unsigned long nr_writeback;
-	unsigned long nr_pagecache;
-} ____cacheline_aligned_in_smp page_states[NR_CPUS];
-
-extern void get_page_state(struct page_state *ret);
-
-#define mod_page_state(member, delta)					\
-	do {								\
-		preempt_disable();					\
-		page_states[smp_processor_id()].member += (delta);	\
-		preempt_enable();					\
-	} while (0)
-
-#define inc_page_state(member)	mod_page_state(member, 1UL)
-#define dec_page_state(member)	mod_page_state(member, 0UL - 1)
-
-
-/*
  * Manipulation of page state flags
  */
 #define PageLocked(page)		\
@@ -219,3 +199,6 @@
 #define PageSwapCache(page) ((page)->mapping == &swapper_space)
 
 #endif	/* PAGE_FLAGS_H */
+
+
+
--- /dev/null	Thu Jan  1 01:00:00 1970
+++ linux-2.5.13/include/linux/page-state.h	Fri May  3 21:57:13 2002
@@ -0,0 +1,25 @@
+/*
+ * Global page accounting.  One instance per CPU.
+ */
+#ifndef PAGE_STATE_H
+#define PAGE_STATE_H
+
+extern struct page_state {
+	unsigned long nr_dirty;
+	unsigned long nr_writeback;
+	unsigned long nr_pagecache;
+} ____cacheline_aligned_in_smp page_states[NR_CPUS];
+
+extern void get_page_state(struct page_state *ret);
+
+#define mod_page_state(member, delta)					\
+	do {								\
+		preempt_disable();					\
+		page_states[smp_processor_id()].member += (delta);	\
+		preempt_enable();					\
+	} while (0)
+
+#define inc_page_state(member)	mod_page_state(member, 1UL)
+#define dec_page_state(member)	mod_page_state(member, 0UL - 1)
+
+#endif	/* PAGE_STATE_H */
--- linux-2.5.13/include/linux/pagemap.h.org	Fri May  3 21:58:57 2002
+++ linux-2.5.13/include/linux/pagemap.h	Fri May  3 22:47:28 2002
@@ -8,6 +8,7 @@
  */
 
 #include <linux/mm.h>
+#include <linux/page-flags.h>
 #include <linux/fs.h>
 #include <linux/list.h>
 
@@ -91,3 +92,4 @@
 extern struct page *read_cache_page(struct address_space *, unsigned long,
 				filler_t *, void *);
 #endif
+
--- linux-2.5.13/drivers/char/drm/radeon_drv.c.org	Fri May  3 23:26:13 2002
+++ linux-2.5.13/drivers/char/drm/radeon_drv.c	Fri May  3 23:26:15 2002
@@ -30,6 +30,7 @@
 #include <linux/config.h>
 #include "radeon.h"
 #include "drmP.h"
+#include <linux/page-flags.h>
 #include "radeon_drv.h"
 #include "ati_pcigart.h"
 
--- linux-2.5.13/drivers/char/drm/drm_memory.h.org	Fri May  3 23:27:48 2002
+++ linux-2.5.13/drivers/char/drm/drm_memory.h	Fri May  3 23:27:52 2002
@@ -32,6 +32,7 @@
 #define __NO_VERSION__
 #include <linux/config.h>
 #include "drmP.h"
+#include <linux/page-flags.h>
 #include <linux/wrapper.h>
 
 typedef struct drm_mem_stats {
--- linux-2.5.13/drivers/usb/class/audio.c.org	Fri May  3 23:31:14 2002
+++ linux-2.5.13/drivers/usb/class/audio.c	Fri May  3 23:31:55 2002
@@ -184,6 +184,7 @@
 #include <linux/soundcard.h>
 #include <linux/list.h>
 #include <linux/vmalloc.h>
+#include <linux/page-flags.h>
 #include <linux/wrapper.h>
 #include <linux/init.h>
 #include <linux/poll.h>
--- linux-2.5.13/sound/pci/rme9652/rme9652_mem.c.org	Fri May  3 23:47:00 2002
+++ linux-2.5.13/sound/pci/rme9652/rme9652_mem.c	Fri May  3 23:47:28 2002
@@ -40,6 +40,7 @@
 #include <linux/pci.h>
 #include <linux/init.h>
 #include <linux/mm.h>
+#include <linux/page-flags.h>
 #include <sound/initval.h>
 
 #define RME9652_CARDS			8
--- linux-2.5.13/sound/core/memory.c.org	Fri May  3 23:10:50 2002
+++ linux-2.5.13/sound/core/memory.c	Fri May  3 23:15:19 2002
@@ -21,6 +21,7 @@
 
 #define __NO_VERSION__
 #include <sound/driver.h>
+#include <linux/page-flags.h>
 #include <asm/io.h>
 #include <asm/uaccess.h>
 #include <linux/init.h>
--- linux-2.5.13/mm/memory.c.org	Fri May  3 22:22:52 2002
+++ linux-2.5.13/mm/memory.c	Fri May  3 22:23:40 2002
@@ -37,6 +37,7 @@
  */
 
 #include <linux/mm.h>
+#include <linux/page-flags.h>
 #include <linux/mman.h>
 #include <linux/swap.h>
 #include <linux/smp_lock.h>
--- linux-2.5.13/mm/slab.c.org	Fri May  3 22:52:59 2002
+++ linux-2.5.13/mm/slab.c	Fri May  3 22:54:12 2002
@@ -72,6 +72,7 @@
 #include	<linux/config.h>
 #include	<linux/slab.h>
 #include	<linux/mm.h>
+#include	<linux/page-flags.h>
 #include	<linux/cache.h>
 #include	<linux/interrupt.h>
 #include	<linux/init.h>
--- linux-2.5.13/mm/vmalloc.c.org	Fri May  3 22:52:46 2002
+++ linux-2.5.13/mm/vmalloc.c	Fri May  3 22:52:51 2002
@@ -9,6 +9,7 @@
 #include <linux/config.h>
 #include <linux/slab.h>
 #include <linux/vmalloc.h>
+#include <linux/page-flags.h>
 #include <linux/spinlock.h>
 #include <linux/highmem.h>
 #include <linux/smp_lock.h>
--- linux-2.5.13/mm/bootmem.c.org	Fri May  3 22:55:56 2002
+++ linux-2.5.13/mm/bootmem.c	Fri May  3 22:56:21 2002
@@ -10,6 +10,7 @@
  */
 
 #include <linux/mm.h>
+#include <linux/page-flags.h>
 #include <linux/kernel_stat.h>
 #include <linux/swap.h>
 #include <linux/swapctl.h>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Andrew Morton]

Erik van Konijnenburg wrote:
> 
> Hi Andrew,
> 
> Do you really have to edit 119 files if you just want to avoid
> processing the PageFoo macros?  Include page-flags.h in pagemap.h,
> and you only have to add include lines to 13 files to get the kernel
> compiled, while still getting rid of 1050 times reading of page-flags.h.
> 
> Motivation:
>     --  linux/mm.h is included in 1163 files
>     --  linux/pagemap.h is included in only 109 files.
>     --  In pagemap.h, wait_on_page_locked() and PageLocked()
>         are mixed rather awkwardly.  Moving wait_on_page_locked()
>         to page-flags.h as you suggested doesn't really help:
>         you'd have to move the ___wait_on_page_locked declaration
>         as well, resulting in other ugliness, so we might as well
>         include page-flags.h.
>     --  most of the added includes are for page-flags.h, so it makes
>         sense not to merge page-flags.h and pagemap.h.
>     --  yes, the number 1050 is wrong, since pagemap.h is also
>         included indirectly.  I'm not sufficiently familiar with
>         the kernel to make a better estimate, so this whole
>         thing may be a red herring.
> 

I guess so.  Certainly, not having to alter that many files
is a bonus.

Part of my uncertainty here is that we just don't seem to
have a "plan".  Is the objective to completely flatten
the include heirarchy, no nested includes, and make all
.c files include all headers to which they (and their included
headers) refer?

That's pretty aggressive, but I think it's the only sane
objective.

BTW,


akpm-1:/usr/src/25> grep pagemap.h include/linux/*.h
include/linux/blkdev.h:#include <linux/pagemap.h>
include/linux/locks.h:#include <linux/pagemap.h>
include/linux/nfs_fs.h:#include <linux/pagemap.h>
include/linux/smb_fs.h:#include <linux/pagemap.h>

These need to be pulled out first.  locks.h and blkdev.h
really don't need the include at all.  page.h
would suffice.

And locks.h can just be deleted from the kernel.  Move
its two definitions into fs.h.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Dave Jones]

On Fri, May 03, 2002 at 04:06:05PM -0700, Andrew Morton wrote:
 > Part of my uncertainty here is that we just don't seem to
 > have a "plan".  Is the objective to completely flatten
 > the include heirarchy, no nested includes, and make all
 > .c files include all headers to which they (and their included
 > headers) refer?
 > 
 > That's pretty aggressive, but I think it's the only sane
 > objective.

<linux/fs.h>, <linux/mm.h>, <linux/sched.h>, <linux/pagemap.h>
are usually the main culprits. Each of these suckers pulls in
dozens and dozens of includes.

So every time someone wants for eg, something trivial like
jiffies, they end up pulling in crap like <asm/mmx.h>
It's hurrendous how much stuff we have in some of the above
mentioned files which probably deserves their own files.

Updated graphs of these dependancies just went up to
ftp://ftp.kernel.org/pub/linux/kernel/people/davej/misc/graphs/

(old versions are in the parent dir before/after the last crapectomy)

    Dave.

-- 
| Dave Jones.        http://www.codemonkey.org.uk
| SuSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Erik van Konijnenburg]

With pagemap.h being the worst of the lot.  Why is this?
In principle it's just some declarations for functions that take
structure pointers as arguments and interesting defines like

	#define page_cache_get(x)       get_page(x)

You don't need include files to compile that: the compiler is perfectly
capable of translating a structure pointer without knowing what's 
inside the structure.

What hurts are the static inline functions; these access structure
fields and do need the structure definitions, sucking in all those
includes.  For pagemap.h, just three functions are responsible for
an 82Kb callgraph: page_cache_alloc, add_to_page_cache, 
wait_on_page_locked.

So what we do is remove all includes and the three evil inline functions
from pagemap.h and put them in pagemap-bonus.h.  Include pagemap-bonus.h
only in the 10 or so files that use it.

Alternatively, put the meat of pagemap.h in a pagemap-diet.h, and leave
the includes and static inlines in pagemap.h; that would make for easier
transition to faster compile times.  I guess something similar can be done 
for the other big three.

The plan would be not to flatten the complete include graph,
but only to avoid the overhead of deeply nested include hierarchies
that are caused by static inline.

Regards,
Erik

On Saturday 04 May 2002 01:39 am, Dave Jones wrote:
> On Fri, May 03, 2002 at 04:06:05PM -0700, Andrew Morton wrote:
>  > Part of my uncertainty here is that we just don't seem to
>  > have a "plan".  Is the objective to completely flatten
>  > the include heirarchy, no nested includes, and make all
>  > .c files include all headers to which they (and their included
>  > headers) refer?
>  > 
>  > That's pretty aggressive, but I think it's the only sane
>  > objective.
> 
> <linux/fs.h>, <linux/mm.h>, <linux/sched.h>, <linux/pagemap.h>
> are usually the main culprits. Each of these suckers pulls in
> dozens and dozens of includes.

>     Dave.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Andrew Morton]

Erik van Konijnenburg wrote:
> 
> ...
> For pagemap.h, just three functions are responsible for
> an 82Kb callgraph: page_cache_alloc, add_to_page_cache,
> wait_on_page_locked.

inlines in headers are just a pita.  I know it gets people
all excited but I'd say: make 'em macros.

___add_to_page_cache() can be just uninlined.  I intend
to gang-add pages into the cache and LRU anyway, so that
function should become for "occasional use only" anyway.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Christoph Hellwig]

On Tue, May 14, 2002 at 01:25:43PM -0700, Andrew Morton wrote:
> Erik van Konijnenburg wrote:
> > 
> > ...
> > For pagemap.h, just three functions are responsible for
> > an 82Kb callgraph: page_cache_alloc, add_to_page_cache,
> > wait_on_page_locked.
> 
> inlines in headers are just a pita.  I know it gets people
> all excited but I'd say: make 'em macros.
> 
> ___add_to_page_cache() can be just uninlined.  I intend
> to gang-add pages into the cache and LRU anyway, so that
> function should become for "occasional use only" anyway.

I'm all for it.  I nfact I have a patch to rename it to link_to_page_cache()
and make it an uninline FASTCALL pending, waiting for the buffer_head.h
patch going to Linus first.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/

Re: page-flags.h

[Daniel Phillips]

On Tuesday 14 May 2002 22:25, Andrew Morton wrote:
> inlines in headers are just a pita.  I know it gets people
> all excited but I'd say: make 'em macros.

Responding to this old, old message - I strongly disagree.  Macros just suck 
too much, because of type safety and self-documentation issues.  Not only 
that, but using them extensively just lets the header inclusion order madness 
degenerate further.

Anyway, header inclusion order is a solved problem as far as I'm concerned, 
please see my patches/posts with 'early page' subject line, on lkml.  The 
winning strategy is to separate data declarations from function declarations, 
and automatically include the former in the latter.

It's true that I haven't brought these patches forward to 2.5, and for that I 
can be faulted.  There's still time though...

-- 
Daniel
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/