asm-generic/tlb.h and check_pgt_cache()

asm-generic/tlb.h and check_pgt_cache()

[Haavard Skinnemoen]

Hi,

Commit a5a19c63f4e55e32dc0bc3d936d7f94793d8b380 from x86.git seems to
have broken several architectures, including alpha (fixed by
c18d1250c7425dddd2633ce4eaf03d5015e68a0f) and avr32 (not fixed yet).

The problem seems to be that asm-generic/tlb.h references
check_pgt_cache(), which is defined in asm/pgalloc.h on most
architectures, so removing that include seems like the wrong thing to
do. x86, however, defines it in asm/pgtable.h which is apparently
included indirectly through other headers.

One way to fix this would be to move the check_pgt_cache() definition
over to asm/pgtable.h, but I suspect this would complicate things a lot
on architectures that use quicklists since they need the QUICK_*
definitions from pgalloc.h in order to implement check_pgt_cache. I
have patches that make avr32 use quicklists as well, so I'm a bit
hesitant to do this.

Another way to fix it would be to include asm/pgalloc.h elsewhere, e.g.
from asm/tlb.h right before including asm-generic/tlb.h. Or perhaps we
should move check_pgt_cache() into asm/tlb.h on all architectures and
include asm/pgalloc.h as needed?

I don't know how many architectures are currently broken -- if it's
only avr32, I can probably come up with a way to fix it on my own. But
if there are others, I thought it might be a good idea to coordinate
things.

Haavard

Re: asm-generic/tlb.h and check_pgt_cache()

[Paul Mundt]

On Thu, Jan 31, 2008 at 12:54:25PM +0100, Haavard Skinnemoen wrote:
> Another way to fix it would be to include asm/pgalloc.h elsewhere, e.g.
> from asm/tlb.h right before including asm-generic/tlb.h. Or perhaps we
> should move check_pgt_cache() into asm/tlb.h on all architectures and
> include asm/pgalloc.h as needed?
> 
> I don't know how many architectures are currently broken -- if it's
> only avr32, I can probably come up with a way to fix it on my own. But
> if there are others, I thought it might be a good idea to coordinate
> things.
> 
sh broke also, and simply moving the check_pgt_cache() definition wasn't
really a sane option due to the quicklist dependence, as you noted. Some
folks seem to have just done check_pgt_cache() in asm/tlb.h (ie, FR-V), I
opted to just include pgalloc.h from tlb.h. Though if this gets fixed
another way, that's fine too.

Re: asm-generic/tlb.h and check_pgt_cache()

[Adrian Bunk]

On Thu, Jan 31, 2008 at 12:54:25PM +0100, Haavard Skinnemoen wrote:
> Hi,
> 
> Commit a5a19c63f4e55e32dc0bc3d936d7f94793d8b380 from x86.git seems to
> have broken several architectures, including alpha (fixed by
> c18d1250c7425dddd2633ce4eaf03d5015e68a0f) and avr32 (not fixed yet).
> 
> The problem seems to be that asm-generic/tlb.h references
> check_pgt_cache(), which is defined in asm/pgalloc.h on most
> architectures, so removing that include seems like the wrong thing to
> do. x86, however, defines it in asm/pgtable.h which is apparently
> included indirectly through other headers.
> 
> One way to fix this would be to move the check_pgt_cache() definition
> over to asm/pgtable.h, but I suspect this would complicate things a lot
> on architectures that use quicklists since they need the QUICK_*
> definitions from pgalloc.h in order to implement check_pgt_cache. I
> have patches that make avr32 use quicklists as well, so I'm a bit
> hesitant to do this.
> 
> Another way to fix it would be to include asm/pgalloc.h elsewhere, e.g.
> from asm/tlb.h right before including asm-generic/tlb.h. Or perhaps we
> should move check_pgt_cache() into asm/tlb.h on all architectures and
> include asm/pgalloc.h as needed?
> 
> I don't know how many architectures are currently broken -- if it's
> only avr32, I can probably come up with a way to fix it on my own. But
> if there are others, I thought it might be a good idea to coordinate
> things.

At least blackfin and m32r suffer from the same compile breakage.

> Haavard

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: asm-generic/tlb.h and check_pgt_cache()

[Adrian Bunk]

On Thu, Jan 31, 2008 at 03:03:28PM +0200, Adrian Bunk wrote:
> On Thu, Jan 31, 2008 at 12:54:25PM +0100, Haavard Skinnemoen wrote:
> > Hi,
> > 
> > Commit a5a19c63f4e55e32dc0bc3d936d7f94793d8b380 from x86.git seems to
> > have broken several architectures, including alpha (fixed by
> > c18d1250c7425dddd2633ce4eaf03d5015e68a0f) and avr32 (not fixed yet).
> > 
> > The problem seems to be that asm-generic/tlb.h references
> > check_pgt_cache(), which is defined in asm/pgalloc.h on most
> > architectures, so removing that include seems like the wrong thing to
> > do. x86, however, defines it in asm/pgtable.h which is apparently
> > included indirectly through other headers.
> > 
> > One way to fix this would be to move the check_pgt_cache() definition
> > over to asm/pgtable.h, but I suspect this would complicate things a lot
> > on architectures that use quicklists since they need the QUICK_*
> > definitions from pgalloc.h in order to implement check_pgt_cache. I
> > have patches that make avr32 use quicklists as well, so I'm a bit
> > hesitant to do this.
> > 
> > Another way to fix it would be to include asm/pgalloc.h elsewhere, e.g.
> > from asm/tlb.h right before including asm-generic/tlb.h. Or perhaps we
> > should move check_pgt_cache() into asm/tlb.h on all architectures and
> > include asm/pgalloc.h as needed?
> > 
> > I don't know how many architectures are currently broken -- if it's
> > only avr32, I can probably come up with a way to fix it on my own. But
> > if there are others, I thought it might be a good idea to coordinate
> > things.
> 
> At least blackfin and m32r suffer from the same compile breakage.

And it seems also uml.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: asm-generic/tlb.h and check_pgt_cache()

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
> On Thu, Jan 31, 2008 at 12:54:25PM +0100, Haavard Skinnemoen wrote:
>   
>> Hi,
>>
>> Commit a5a19c63f4e55e32dc0bc3d936d7f94793d8b380 from x86.git seems to
>> have broken several architectures, including alpha (fixed by
>> c18d1250c7425dddd2633ce4eaf03d5015e68a0f) and avr32 (not fixed yet).
>>
>> The problem seems to be that asm-generic/tlb.h references
>> check_pgt_cache(), which is defined in asm/pgalloc.h on most
>> architectures, so removing that include seems like the wrong thing to
>> do. x86, however, defines it in asm/pgtable.h which is apparently
>> included indirectly through other headers.
>>
>> One way to fix this would be to move the check_pgt_cache() definition
>> over to asm/pgtable.h, but I suspect this would complicate things a lot
>> on architectures that use quicklists since they need the QUICK_*
>> definitions from pgalloc.h in order to implement check_pgt_cache. I
>> have patches that make avr32 use quicklists as well, so I'm a bit
>> hesitant to do this.
>>
>> Another way to fix it would be to include asm/pgalloc.h elsewhere, e.g.
>> from asm/tlb.h right before including asm-generic/tlb.h. Or perhaps we
>> should move check_pgt_cache() into asm/tlb.h on all architectures and
>> include asm/pgalloc.h as needed?
>>
>> I don't know how many architectures are currently broken -- if it's
>> only avr32, I can probably come up with a way to fix it on my own. But
>> if there are others, I thought it might be a good idea to coordinate
>> things.
>>     
>
> At least blackfin and m32r suffer from the same compile breakage.
>   

Huh, interesting: avr32, sh, blackfin and m32r - all a similar class of 
architectures.  Does this show a genealogical relationship?

Anyway, sorry about the breakage.  There's a cyclic dependency between 
asm-generic/tlb.h and asm/pgalloc.h, since __*_free_tlb (often) uses 
tlb_remove_page().

 From the look of it:
avr32 - no-op check_pgt_list
sh - uses quicklist_*, and defines QUICK_* (unique among architectures)
blackfin - how does this break? asm-blackfin/pgalloc.h is more or less no-op
m32r - no-op check_pgt_list
um - no-op check_pgt_list

I'm guessing in blackfin's case, the breakage is some indirect 
dependency on asm-blackfin/pgalloc.h via asm/tlb.h->asm-generic/tlb.h 
rather than a specific check_pgt_list() problem.  Adrian, is that 
right?  That should be fixable by putting #include <asm/pgalloc.h> in 
the appropriate places.

um also has a fairly simply pgalloc.h with no dependency on 
asm-generic/tlb.h, so I assume the breakage there is also the result of 
an indirect pgalloc.h dependency.

For avr32 and m32r there should be no problem in moving the no-op 
check_pgt_list() definition to somewhere else, like asm-*/pgtable.h.

The only tricky case seems to be sh.  That needs a bit more thought.

    J

Re: asm-generic/tlb.h and check_pgt_cache()

[Adrian Bunk]

On Thu, Jan 31, 2008 at 08:31:01AM -0800, Jeremy Fitzhardinge wrote:
> Adrian Bunk wrote:
>> On Thu, Jan 31, 2008 at 12:54:25PM +0100, Haavard Skinnemoen wrote:
>>   
>>> Hi,
>>>
>>> Commit a5a19c63f4e55e32dc0bc3d936d7f94793d8b380 from x86.git seems to
>>> have broken several architectures, including alpha (fixed by
>>> c18d1250c7425dddd2633ce4eaf03d5015e68a0f) and avr32 (not fixed yet).
>>>
>>> The problem seems to be that asm-generic/tlb.h references
>>> check_pgt_cache(), which is defined in asm/pgalloc.h on most
>>> architectures, so removing that include seems like the wrong thing to
>>> do. x86, however, defines it in asm/pgtable.h which is apparently
>>> included indirectly through other headers.
>>>
>>> One way to fix this would be to move the check_pgt_cache() definition
>>> over to asm/pgtable.h, but I suspect this would complicate things a lot
>>> on architectures that use quicklists since they need the QUICK_*
>>> definitions from pgalloc.h in order to implement check_pgt_cache. I
>>> have patches that make avr32 use quicklists as well, so I'm a bit
>>> hesitant to do this.
>>>
>>> Another way to fix it would be to include asm/pgalloc.h elsewhere, e.g.
>>> from asm/tlb.h right before including asm-generic/tlb.h. Or perhaps we
>>> should move check_pgt_cache() into asm/tlb.h on all architectures and
>>> include asm/pgalloc.h as needed?
>>>
>>> I don't know how many architectures are currently broken -- if it's
>>> only avr32, I can probably come up with a way to fix it on my own. But
>>> if there are others, I thought it might be a good idea to coordinate
>>> things.
>>>     
>>
>> At least blackfin and m32r suffer from the same compile breakage.
>>   
>
> Huh, interesting: avr32, sh, blackfin and m32r - all a similar class of  
> architectures.  Does this show a genealogical relationship?
>
> Anyway, sorry about the breakage.  There's a cyclic dependency between  
> asm-generic/tlb.h and asm/pgalloc.h, since __*_free_tlb (often) uses  
> tlb_remove_page().
>
> From the look of it:
> avr32 - no-op check_pgt_list
> sh - uses quicklist_*, and defines QUICK_* (unique among architectures)
> blackfin - how does this break? asm-blackfin/pgalloc.h is more or less no-op
> m32r - no-op check_pgt_list
> um - no-op check_pgt_list
>
> I'm guessing in blackfin's case, the breakage is some indirect  
> dependency on asm-blackfin/pgalloc.h via asm/tlb.h->asm-generic/tlb.h  
> rather than a specific check_pgt_list() problem.  Adrian, is that right?  
>...

blackfin is:

<--  snip  -->

...
  CC      mm/nommu.o
In file included from include2/asm/tlb.h:14,
                 from 
/home/bunk/linux/kernel-2.6/git/linux-2.6/mm/nommu.c:33:
/home/bunk/linux/kernel-2.6/git/linux-2.6/include/asm-generic/tlb.h: In function 'tlb_finish_mmu':
/home/bunk/linux/kernel-2.6/git/linux-2.6/include/asm-generic/tlb.h:91: error: implicit declaration of function 'check_pgt_cache'
make[2]: *** [mm/nommu.o] Error 1

<--  snip  -->

>    J

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: asm-generic/tlb.h and check_pgt_cache()

[Jeremy Fitzhardinge]

Adrian Bunk wrote:
> <--  snip  -->
>
> ...
>   CC      mm/nommu.o
> In file included from include2/asm/tlb.h:14,
>                  from 
> /home/bunk/linux/kernel-2.6/git/linux-2.6/mm/nommu.c:33:
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/asm-generic/tlb.h: In function 'tlb_finish_mmu':
> /home/bunk/linux/kernel-2.6/git/linux-2.6/include/asm-generic/tlb.h:91: error: implicit declaration of function 'check_pgt_cache'
> make[2]: *** [mm/nommu.o] Error 1
>
> <--  snip  -->
>
>   

Right, OK.  Well, I think Ingo reverted my header manipulations, so it 
should all be back to normal.

    J

Re: asm-generic/tlb.h and check_pgt_cache()

[Paul Mundt]

On Thu, Jan 31, 2008 at 08:31:01AM -0800, Jeremy Fitzhardinge wrote:
> Anyway, sorry about the breakage.  There's a cyclic dependency between 
> asm-generic/tlb.h and asm/pgalloc.h, since __*_free_tlb (often) uses 
> tlb_remove_page().
> 
> From the look of it:
> avr32 - no-op check_pgt_list
> sh - uses quicklist_*, and defines QUICK_* (unique among architectures)
> blackfin - how does this break? asm-blackfin/pgalloc.h is more or less no-op
> m32r - no-op check_pgt_list
> um - no-op check_pgt_list
> 
> I'm guessing in blackfin's case, the breakage is some indirect 
> dependency on asm-blackfin/pgalloc.h via asm/tlb.h->asm-generic/tlb.h 
> rather than a specific check_pgt_list() problem.  Adrian, is that 
> right?  That should be fixable by putting #include <asm/pgalloc.h> in 
> the appropriate places.
> 
> um also has a fairly simply pgalloc.h with no dependency on 
> asm-generic/tlb.h, so I assume the breakage there is also the result of 
> an indirect pgalloc.h dependency.
> 
> For avr32 and m32r there should be no problem in moving the no-op 
> check_pgt_list() definition to somewhere else, like asm-*/pgtable.h.
> 
> The only tricky case seems to be sh.  That needs a bit more thought.
> 
Yes, sh has the __pte_free_tlb() wrapping to tlb_remove_page(), but there
is nothing directly in asm/pgalloc.h that depends on __pte_free_tlb(), so
maybe the easiest solution is to just have asm/pgalloc.h included by
asm/tlb.h for the check_pgt_cache() def and move __pte_free_tlb() in to
asm/tlb.h directly. One could argue that the __x_free_tlb() routines make
more sense in asm/tlb.h just in terms of namespace anyways.

It's a bit ugly, but if sh is the odd one out here I can live with it.
This seems to work out ok at least..

---

 include/asm-sh/pgalloc.h |    4 ----
 include/asm-sh/tlb.h     |    7 ++++++-
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/include/asm-sh/pgalloc.h b/include/asm-sh/pgalloc.h
index 18b613c..f3b2e56 100644
--- a/include/asm-sh/pgalloc.h
+++ b/include/asm-sh/pgalloc.h
@@ -64,15 +64,11 @@ static inline void pte_free(struct page *pte)
 	quicklist_free_page(QUICK_PT, NULL, pte);
 }
 
-#define __pte_free_tlb(tlb,pte) tlb_remove_page((tlb),(pte))
-
 /*
  * allocating and freeing a pmd is trivial: the 1-entry pmd is
  * inside the pgd, so has no extra memory associated with it.
  */
-
 #define pmd_free(x)			do { } while (0)
-#define __pmd_free_tlb(tlb,x)		do { } while (0)
 
 static inline void check_pgt_cache(void)
 {
diff --git a/include/asm-sh/tlb.h b/include/asm-sh/tlb.h
index 56ad1fb..6f3eda5 100644
--- a/include/asm-sh/tlb.h
+++ b/include/asm-sh/tlb.h
@@ -1,6 +1,8 @@
 #ifndef __ASM_SH_TLB_H
 #define __ASM_SH_TLB_H
 
+#include <asm/pgalloc.h>
+
 #ifdef CONFIG_SUPERH64
 # include "tlb_64.h"
 #endif
@@ -18,9 +20,12 @@
 /*
  * Flush whole TLBs for MM
  */
-#define tlb_flush(tlb)				flush_tlb_mm((tlb)->mm)
+#define tlb_flush(tlb)			flush_tlb_mm((tlb)->mm)
 
 #include <asm-generic/tlb.h>
 
+#define __pte_free_tlb(tlb,pte)		tlb_remove_page((tlb),(pte))
+#define __pmd_free_tlb(tlb,x)		do { } while (0)
+
 #endif /* __ASSEMBLY__ */
 #endif /* __ASM_SH_TLB_H */

Re: asm-generic/tlb.h and check_pgt_cache()

[Jeremy Fitzhardinge]

Paul Mundt wrote:
> Yes, sh has the __pte_free_tlb() wrapping to tlb_remove_page(), but there
> is nothing directly in asm/pgalloc.h that depends on __pte_free_tlb(), so
> maybe the easiest solution is to just have asm/pgalloc.h included by
> asm/tlb.h for the check_pgt_cache() def and move __pte_free_tlb() in to
> asm/tlb.h directly. One could argue that the __x_free_tlb() routines make
> more sense in asm/tlb.h just in terms of namespace anyways.
>
> It's a bit ugly, but if sh is the odd one out here I can live with it.
> This seems to work out ok at least..
>   

x86.git is fixed, so don't worry about it.

    J