tlb_finish_mmu() bogisity

tlb_finish_mmu() bogisity

[Al Viro]

        tlb->need_flush += &__get_cpu_var(quicklist)[0].nr_pages != 0;
makes no sense whatsoever.  How the hell can you ever get the address of
__get_cpu_var(quicklist)[0].nr_pages to be NULL?  Postfix operators have
higher precedence than prefix ones, so that's
	&(((__get_cpu_var(quicklist))[0]).nr_pages)

What did you intend here?  s/&//, perhaps?

Re: tlb_finish_mmu() bogisity

[Linus Torvalds]

On Sun, 23 Dec 2007, Al Viro wrote:
>
>         tlb->need_flush += &__get_cpu_var(quicklist)[0].nr_pages != 0;
> makes no sense whatsoever.  How the hell can you ever get the address of
> __get_cpu_var(quicklist)[0].nr_pages to be NULL?  Postfix operators have
> higher precedence than prefix ones, so that's
> 	&(((__get_cpu_var(quicklist))[0]).nr_pages)
> 
> What did you intend here?  s/&//, perhaps?

I think that thing is bogus in other ways. It plays games with 
"needs_flush", just because it seems to want the generic code to then call 
"check_pgt_cache()", not because it actually wants any flushing to take 
place.

But we already call "check_pgt_cache()" there in tlb_finish_mmu() 
unconditionally, so I think the whole patch was utter crap.

Or is there any other reason going on here? That quicklist stuff has been 
totally broken, it's done too many totally invalid things to really be 
worth even keeping. We should get rid of that unmaintainable hack, and if 
it has a real performance upside, we should make it part of the *native* 
mmu-gather infrastructure, instead of maintaining it as some separate and 
buggy/unmaintainable piece-of-sh*t code.

		Linus

Re: tlb_finish_mmu() bogisity

[Christoph Lameter]

On Sun, 23 Dec 2007, Linus Torvalds wrote:

> > What did you intend here?  s/&//, perhaps?
> 
> I think that thing is bogus in other ways. It plays games with 
> "needs_flush", just because it seems to want the generic code to then call 
> "check_pgt_cache()", not because it actually wants any flushing to take 
> place.

It can only free the pages on the quicklist after the flush. So it needs 
to set need_flush to trigger the flush before check_pgt_cache can trim the 
quicklist. If need_flush is not set then pages are freed in 
check_pgt_cache before their TLBs have been flushed.

> Or is there any other reason going on here? That quicklist stuff has been 
> totally broken, it's done too many totally invalid things to really be 
> worth even keeping. We should get rid of that unmaintainable hack, and if 
> it has a real performance upside, we should make it part of the *native* 
> mmu-gather infrastructure, instead of maintaining it as some separate and 
> buggy/unmaintainable piece-of-sh*t code.

Yup as we agreed before it would be best to make the TLB pieces part of 
the native mmu gather infrastructure. But then they vary quite a lot 
between arches.



Quicklist: Remove bogus &

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6/include/asm-generic/tlb.h
===================================================================
--- linux-2.6.orig/include/asm-generic/tlb.h	2007-12-24 10:53:52.000000000 -0800
+++ linux-2.6/include/asm-generic/tlb.h	2007-12-24 10:54:01.000000000 -0800
@@ -87,7 +87,7 @@ static inline void
 tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
 {
 #ifdef CONFIG_QUICKLIST
-	tlb->need_flush += &__get_cpu_var(quicklist)[0].nr_pages != 0;
+	tlb->need_flush += __get_cpu_var(quicklist)[0].nr_pages != 0;
 #endif
 	tlb_flush_mmu(tlb, start, end);
 

Re: tlb_finish_mmu() bogisity

[Christoph Lameter]

Argh. This is indeed bogus. The one reporting the problem states that the 
patch did not address the issue. The report was regarding pgd freeing 
which is handled slightly differently from pte frees.

[PATCH] Revert quicklist need->flush fix

Did not fix the reported issue. Apart from other weirdness this causes a 
bad link between the TLB flushing logic and the quicklists. If there is 
indeed an issue that an arch needs a tlb flush before free then the arch 
code needs to set tlb->need_flush before calling quicklist_free.

Signed-off-by: Christoph Lameter <clameter@sgi.com>

Index: linux-2.6/include/asm-generic/tlb.h
===================================================================
--- linux-2.6.orig/include/asm-generic/tlb.h	2007-12-26 12:26:32.000000000 -0800
+++ linux-2.6/include/asm-generic/tlb.h	2007-12-26 12:26:39.000000000 -0800
@@ -86,9 +86,6 @@ tlb_flush_mmu(struct mmu_gather *tlb, un
 static inline void
 tlb_finish_mmu(struct mmu_gather *tlb, unsigned long start, unsigned long end)
 {
-#ifdef CONFIG_QUICKLIST
-	tlb->need_flush += &__get_cpu_var(quicklist)[0].nr_pages != 0;
-#endif
 	tlb_flush_mmu(tlb, start, end);
 
 	/* keep the page table cache within bounds */