[PATCH] include linux/pagemap.h in asm-generic/tlb.h

[PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Jeff Dike]

[ This looks non-urgent to me ]

Without linux/pagemap.h, asm-generic/tlb.h is missing declarations of
page_cache_release and release_pages.

Signed-off-by: Jeff Dike <jdike@linux.intel.com>
--
 include/asm-generic/tlb.h |    1 +
 1 file changed, 1 insertion(+)

Index: linux-2.6.22/include/asm-generic/tlb.h
===================================================================
--- linux-2.6.22.orig/include/asm-generic/tlb.h	2007-07-08 19:32:17.000000000 -0400
+++ linux-2.6.22/include/asm-generic/tlb.h	2007-08-22 17:29:45.000000000 -0400
@@ -13,6 +13,7 @@
 #ifndef _ASM_GENERIC__TLB_H
 #define _ASM_GENERIC__TLB_H
 
+#include <linux/pagemap.h>
 #include <linux/swap.h>
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Rob Landley]

On Friday 24 August 2007 1:46:16 pm Jeff Dike wrote:
> [ This looks non-urgent to me ]
>
> Without linux/pagemap.h, asm-generic/tlb.h is missing declarations of
> page_cache_release and release_pages.

It may be non-urgent to you, but it still broke my build. :)

Acked-by: Rob Landley <rob@landley.net>

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Jeff Dike]

On Fri, Aug 24, 2007 at 03:51:41PM -0500, Rob Landley wrote:
> It may be non-urgent to you, but it still broke my build. :)
> 
> Acked-by: Rob Landley <rob@landley.net>

Heh, it doesn't seem like -stable material to me, but if there aren't
any screams while it sits in -mm, maybe it's 2.6.23 material.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Rob Landley]

On Friday 24 August 2007 5:46:51 pm Jeff Dike wrote:
> On Fri, Aug 24, 2007 at 03:51:41PM -0500, Rob Landley wrote:
> > It may be non-urgent to you, but it still broke my build. :)
> >
> > Acked-by: Rob Landley <rob@landley.net>
>
> Heh, it doesn't seem like -stable material to me, but if there aren't
> any screams while it sits in -mm, maybe it's 2.6.23 material.

I believe 2.6.22 built for me without the patch.  It was 2.6.23-rc3 that 
needed it.

(I have a bad head cold right now so I'm not going back to confirm this 
belief.  I am, instead, making tea.)

> 				Jeff

Rob
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Andrew Morton]

On Fri, 24 Aug 2007 14:46:16 -0400
Jeff Dike <jdike@addtoit.com> wrote:

> [ This looks non-urgent to me ]
> 
> Without linux/pagemap.h, asm-generic/tlb.h is missing declarations of
> page_cache_release and release_pages.
> 
> Signed-off-by: Jeff Dike <jdike@linux.intel.com>
> --
>  include/asm-generic/tlb.h |    1 +
>  1 file changed, 1 insertion(+)
> 
> Index: linux-2.6.22/include/asm-generic/tlb.h
> ===================================================================
> --- linux-2.6.22.orig/include/asm-generic/tlb.h	2007-07-08 19:32:17.000000000 -0400
> +++ linux-2.6.22/include/asm-generic/tlb.h	2007-08-22 17:29:45.000000000 -0400
> @@ -13,6 +13,7 @@
>  #ifndef _ASM_GENERIC__TLB_H
>  #define _ASM_GENERIC__TLB_H
>  
> +#include <linux/pagemap.h>
>  #include <linux/swap.h>
>  #include <asm/pgalloc.h>
>  #include <asm/tlbflush.h>

This is worrisome.  If you look at pagemap.h, it includes a pile of things
which could easily themsleves try to include tlb.h via some path or
another.  I fear that this patch will cause explosions with some config
and/or architecture.

If you like, tlb.h is a low-level sort of thing whereas pagemap.h is a
higher-level VFS thing.  It is more appropriate that pagemap.h be including
tlb.h.


So I think a better fix would be better, but I'm not able to suggest what,
as there is little detail about the failure here and I can find no mention
of page_cache_release and release_pages in asm-generic/tlb.h.  Presumably
they're getting pulled in via some macro and then instantiated in some
inline function or something?

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Jeff Dike]

On Fri, Aug 24, 2007 at 05:20:27PM -0700, Andrew Morton wrote:
> This is worrisome.  If you look at pagemap.h, it includes a pile of things
> which could easily themsleves try to include tlb.h via some path or
> another.  I fear that this patch will cause explosions with some config
> and/or architecture.

I agree, I looked at the includes and had the same thoughts.  I sent
it in because there is a problem here, and it fixes a UML config
without noticably breaking anything else.

> So I think a better fix would be better, but I'm not able to suggest what,
> as there is little detail about the failure here and I can find no mention
> of page_cache_release and release_pages in asm-generic/tlb.h.  Presumably
> they're getting pulled in via some macro and then instantiated in some
> inline function or something?

Yes, it's straightforward - 

tlb.h:

static inline void
tlb_flush_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
{
	...
		free_pages_and_swap_cache(tlb->pages, tlb->nr);
	...
}

swap.h:
#define free_pages_and_swap_cache(pages, nr) \
	release_pages((pages), (nr), 0);

Feel free to drop it, and I'll look at this some more.

				Jeff

-- 
Work email - jdike at linux dot intel dot com

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Andrew Morton]

On Fri, 24 Aug 2007 22:15:33 -0400 Jeff Dike <jdike@addtoit.com> wrote:

> > So I think a better fix would be better, but I'm not able to suggest what,
> > as there is little detail about the failure here and I can find no mention
> > of page_cache_release and release_pages in asm-generic/tlb.h.  Presumably
> > they're getting pulled in via some macro and then instantiated in some
> > inline function or something?
> 
> Yes, it's straightforward - 
> 
> tlb.h:
> 
> static inline void
> tlb_flush_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
> {
> 	...
> 		free_pages_and_swap_cache(tlb->pages, tlb->nr);
> 	...
> }
> 
> swap.h:
> #define free_pages_and_swap_cache(pages, nr) \
> 	release_pages((pages), (nr), 0);

Oh that damn thing again.  It's a regular source of include breakage.

I suppose we could just uninline tlb_flush_mmu().

Re: [PATCH] include linux/pagemap.h in asm-generic/tlb.h

[Rob Landley]

On Friday 24 August 2007 7:20:27 pm Andrew Morton wrote:
> On Fri, 24 Aug 2007 14:46:16 -0400
>
> Jeff Dike <jdike@addtoit.com> wrote:
> > [ This looks non-urgent to me ]
> >
> > Without linux/pagemap.h, asm-generic/tlb.h is missing declarations of
> > page_cache_release and release_pages.
> >
> > Signed-off-by: Jeff Dike <jdike@linux.intel.com>
> > --
> >  include/asm-generic/tlb.h |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > Index: linux-2.6.22/include/asm-generic/tlb.h
> > ===================================================================
> > --- linux-2.6.22.orig/include/asm-generic/tlb.h	2007-07-08
> > 19:32:17.000000000 -0400 +++
> > linux-2.6.22/include/asm-generic/tlb.h	2007-08-22 17:29:45.000000000
> > -0400 @@ -13,6 +13,7 @@
> >  #ifndef _ASM_GENERIC__TLB_H
> >  #define _ASM_GENERIC__TLB_H
> >
> > +#include <linux/pagemap.h>
> >  #include <linux/swap.h>
> >  #include <asm/pgalloc.h>
> >  #include <asm/tlbflush.h>
>
> This is worrisome.  If you look at pagemap.h, it includes a pile of things
> which could easily themsleves try to include tlb.h via some path or
> another.  I fear that this patch will cause explosions with some config
> and/or architecture.
>
> If you like, tlb.h is a low-level sort of thing whereas pagemap.h is a
> higher-level VFS thing.  It is more appropriate that pagemap.h be including
> tlb.h.
>
>
> So I think a better fix would be better, but I'm not able to suggest what,
> as there is little detail about the failure here and I can find no mention
> of page_cache_release and release_pages in asm-generic/tlb.h.

To reproduce it, do this in -rc3:

cat > mini.conf << EOF
CONFIG_MODE_SKAS=y
CONFIG_BINFMT_ELF=y
CONFIG_HOSTFS=y
CONFIG_SYSCTL=y
CONFIG_STDERR_CONSOLE=y
CONFIG_UNIX98_PTYS=y
CONFIG_BLK_DEV_LOOP=y
CONFIG_LBD=y
CONFIG_EXT2_FS=y
CONFIG_PROC_FS=y
EOF
make ARCH=um allnoconfig KCONFIG_ALLCONFIG=mini.conf
make ARCH=um

Rob

(P.S. I note that in order for CONFIG_BLK_DEV_LOOP to actually trigger work 
now, I have to add CONFIG_BLK_DEV=y to the above, which I didn't have to do 
in 2.6.22 or in any previous version all the way back to 2.6.12.  Not a major 
regression, but still a bit of a surprise.  That said, the above is what 
triggered the break for me.)
-- 
"One of my most productive days was throwing away 1000 lines of code."
  - Ken Thompson.