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?
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
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 <[email protected]>
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);
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 <[email protected]>
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 */