post-3.18 performance regression in TLB flushing code

https://github.com/antonblanchard/will-it-scale/blob/master/tests/brk1.c

I'm running the 'brk1' test from will-it-scale:

Quoted 1 line

https://github.com/antonblanchard/will-it-scale/blob/master/tests/brk1.c

on a 8-socket/160-thread system. It's seeing about a 6% drop in performance (263M -> 247M ops/sec at 80-threads) from this commit:

	commit fb7332a9fedfd62b1ba6530c86f39f0fa38afd49
	Author: Will Deacon <[email protected]>
	Date:   Wed Oct 29 10:03:09 2014 +0000

	 mmu_gather: move minimal range calculations into generic code

tlb_finish_mmu() goes up about 9x in the profiles (~0.4%->3.6%) and tlb_flush_mmu_free() takes about 3.1% of CPU time with the patch applied, but does not show up at all on the commit before.

This isn't a major regression, but it is rather unfortunate for a patch that is apparently a code cleanup. It also _looks_ to show up even when things are single-threaded, although I haven't looked at it in detail.

I suspect the tlb->need_flush logic was serving some role that the modified code isn't capturing like in this hunk:

mm/memory.c static void tlb_flush_mmu_free(struct mm · line 266

Quoted 7 lines

void tlb_flush_mmu(struct mmu_gather *tlb)
{

mm/memory.c renamed from mm/memory.c~fix-old-need_flush-logic +0 -2

-       if (!tlb->need_flush)
-               return;
        tlb_flush_mmu_tlbonly(tlb);
        tlb_flush_mmu_free(tlb);
 }

tlb_flush_mmu_tlbonly() has tlb->end check (which replaces the ->need_flush logic), but tlb_flush_mmu_free() does not.

If we add a !tlb->end (patch attached) to tlb_flush_mmu(), that gets us back up to ~258M ops/sec, but that's still ~2% down from where we started.

I think there are a couple of things you could try to see if that 2% comes back:

* Revert the patch and try the one here [1] instead (which only does part

    (1) of the above).

-- or --

* Instead of adding the tlb->end check to tlb_flush_mmu, add it to tlb_flush_mmu_free

Quoted 11 lines · Will Deacon

I think there are a couple of things you could try to see if that 2% comes back:

* Revert the patch and try the one here [1] instead (which only does part

    (1) of the above).

-- or --

* Instead of adding the tlb->end check to tlb_flush_mmu, add it to tlb_flush_mmu_free

or just move the check back to tlb_flush_mmu() where it belongs.

I don't see why you moved it to "tlb_flush_mmu_tlbonly()" in the first place, or why you'd now want to add it to tlb_flush_mmu_free().

Both of those helper functions have two callers:

- tlb_flush_mmu(). Doing it here (instead of in the helper functions)
is the right thing to do

- the "force_flush" case: we know we have added at least one page to
the TLB state so checking for it is pointless.

So I'm not seeing why you wanted to do it in tlb_flush_mmu_tlbonly(), and now add it to tlb_flush_mmu_free(). That seems bogus.

So why not just this trivial patch, to make the logic be the same it used to be (just using "end > 0" instead of the old "need_flush")?

mm/memory.c +0 -3

index c3b9097251c5..6efe36a998ba 100644
@@ -235,9 +235,6 @@ void tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm, unsigned long
 
 static void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
 {
-	if (!tlb->end)
-		return;
-
 	tlb_flush(tlb);
 	mmu_notifier_invalidate_range(tlb->mm, tlb->start, tlb->end);
 #ifdef CONFIG_HAVE_RCU_TABLE_FREE

Quoted 3 lines · Linus Torvalds

On Wed, Dec 17, 2014 at 2:08 AM, Will Deacon <[email protected]> wrote:
So why not just this trivial patch, to make the logic be the same it used to be (just using "end > 0" instead of the old "need_flush")?

Looks fine to me... Dave?

These things are also a little bit noisy, so we're well within the margin of error with Linus's fix.

This also holds up on the large system.