minor mmu_gather patches

There is no need to call this from tlb_flush_mmu_tlbonly, it logically belongs with tlb_flush_mmu_free. This allows some code consolidation with a subsequent fix.

Signed-off-by Nicholas Piggin <[email protected]>

---
 mm/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

mm/memory.c +3 -3

index 19f47d7b9b86..7c58310734eb 100644
@@ -245,9 +245,6 @@ static void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
 
 	tlb_flush(tlb);
 	mmu_notifier_invalidate_range(tlb->mm, tlb->start, tlb->end);
-#ifdef CONFIG_HAVE_RCU_TABLE_FREE
-	tlb_table_flush(tlb);
-#endif
 	__tlb_reset_range(tlb);
 }
 

@@ -255,6 +252,9 @@ static void tlb_flush_mmu_free(struct mmu_gather *tlb)
 {
 	struct mmu_gather_batch *batch;
 
+#ifdef CONFIG_HAVE_RCU_TABLE_FREE
+	tlb_table_flush(tlb);
+#endif
 	for (batch = &tlb->local; batch && batch->nr; batch = batch->next) {
 		free_pages_and_swap_cache(batch->pages, batch->nr);
 		batch->nr = 0;

Signature · 1 line

2.17.0

There is no need to call this from tlb_flush_mmu_tlbonly, it logically belongs with tlb_flush_mmu_free. This allows some code consolidation with a subsequent fix.

Signed-off-by Nicholas Piggin <[email protected]>

Quoted 5 lines · Nicholas Piggin

There is no need to call this from tlb_flush_mmu_tlbonly, it logically belongs with tlb_flush_mmu_free. This allows some code consolidation with a subsequent fix.

Signed-off-by Nicholas Piggin <[email protected]>

Reviewed-by Rik van Riel <[email protected]>

This patch also fixes an infinite recursion bug
with CONFIG_HAVE_RCU_TABLE_FREE enabled, which
has this call trace:

tlb_table_flush
-> tlb_table_invalidate

     -> tlb_flush_mmu_tlbonly
        -> tlb_table_flush
           -> ... (infinite recursion)

This should probably be applied sooner rather than later.

Quoted 19 lines · Rik van Riel

Quoted 5 lines · Nicholas Piggin

There is no need to call this from tlb_flush_mmu_tlbonly, it logically belongs with tlb_flush_mmu_free. This allows some code consolidation with a subsequent fix.

Signed-off-by Nicholas Piggin <[email protected]>

Reviewed-by Rik van Riel <[email protected]>

This patch also fixes an infinite recursion bug
with CONFIG_HAVE_RCU_TABLE_FREE enabled, which
has this call trace:

tlb_table_flush
-> tlb_table_invalidate

     -> tlb_flush_mmu_tlbonly
        -> tlb_table_flush
           -> ... (infinite recursion)

Quoted 2 lines

This should probably be applied sooner rather than later.

It's already in mainline with a cc stable afaict.

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Signed-off-by Nicholas Piggin <[email protected]>

---
 include/asm-generic/tlb.h | 17 +++++++++++++----
 mm/memory.c               | 10 ----------
 2 files changed, 13 insertions(+), 14 deletions(-)

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Signed-off-by Nicholas Piggin <[email protected]>

Acked-by Will Deacon <[email protected]>

Quoted 5 lines · Nicholas Piggin

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

Quoted 5 lines

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Signed-off-by Nicholas Piggin <[email protected]>

Acked-by Will Deacon <[email protected]>

This patch breaks riscv builds in mainline.

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Nicholas Piggin

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Nicholas Piggin <[email protected]>

Acked-by Will Deacon <[email protected]>

This patch breaks riscv builds in mainline.

Looks very similar to the breakage we hit on arm64. diff below should fix it.

Quoted 10 lines · Will Deacon

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Nicholas Piggin

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Signed-off-by Nicholas Piggin <[email protected]>

Acked-by Will Deacon <[email protected]>

This patch breaks riscv builds in mainline.

Looks very similar to the breakage we hit on arm64. diff below should fix it.

Unfortunately it doesn't.

In file included from ./arch/riscv/include/asm/pgtable.h:26:0,

                 from ./include/linux/memremap.h:7,
                 from ./include/linux/mm.h:27,
                 from arch/riscv/mm/fault.c:23:
./arch/riscv/include/asm/tlb.h: In function ‘tlb_flush’:
./arch/riscv/include/asm/tlb.h:19:18: error: dereferencing pointer to incomplete type ‘struct mmu_gather’
  flush_tlb_mm(tlb->mm);
                  ^

Quoted 22 lines · Guenter Roeck

Quoted 10 lines · Will Deacon

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Nicholas Piggin

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Signed-off-by Nicholas Piggin <[email protected]>

Acked-by Will Deacon <[email protected]>

This patch breaks riscv builds in mainline.

Looks very similar to the breakage we hit on arm64. diff below should fix it.

Unfortunately it doesn't.

Quoted 8 lines

In file included from ./arch/riscv/include/asm/pgtable.h:26:0,

                 from ./include/linux/memremap.h:7,
                 from ./include/linux/mm.h:27,
                 from arch/riscv/mm/fault.c:23:
./arch/riscv/include/asm/tlb.h: In function ‘tlb_flush’:
./arch/riscv/include/asm/tlb.h:19:18: error: dereferencing pointer to incomplete type ‘struct mmu_gather’
  flush_tlb_mm(tlb->mm);
                  ^

Sorry, I was a bit quick of the mark there. You'll need a forward declaration for the paramater type. Here it is with a commit message, although still untested because I haven't got round to setting up a riscv toolchain yet.

Will

--->8

From adb9be33d68320edcda80d540a97a647792894d2 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Fri, 24 Aug 2018 14:33:48 +0100
Subject: [PATCH] riscv: tlb: Provide definition of tlb_flush() before
including tlb.h

As of commit fd1102f0aade ("mm: mmu_notifier fix for tlb_end_vma"), asm-generic/tlb.h now calls tlb_flush() from a static inline function, so we need to make sure that it's declared before #including the asm-generic header in the arch header.

Since tlb_flush() is a one-liner for riscv, we can define it before including asm-generic/tlb.h as long as we provide a forward declaration of struct mmu_gather.

Reported-by Guenter Roeck <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/riscv/include/asm/tlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/riscv/include/asm/tlb.h +0 -0

index c229509288ea..a3d1380ad970 100644

Quoted 6 lines

Sorry, I was a bit quick of the mark there. You'll need a forward declaration for the paramater type. Here it is with a commit message, although still untested because I haven't got round to setting up a riscv toolchain yet.

Quoted 30 lines

Will

--->8

From adb9be33d68320edcda80d540a97a647792894d2 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Fri, 24 Aug 2018 14:33:48 +0100
Subject: [PATCH] riscv: tlb: Provide definition of tlb_flush() before
including tlb.h

As of commit fd1102f0aade ("mm: mmu_notifier fix for tlb_end_vma"), asm-generic/tlb.h now calls tlb_flush() from a static inline function, so we need to make sure that it's declared before #including the asm-generic header in the arch header.

Since tlb_flush() is a one-liner for riscv, we can define it before including asm-generic/tlb.h as long as we provide a forward declaration of struct mmu_gather.

Reported-by Guenter Roeck <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/riscv/include/asm/tlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/riscv/include/asm/tlb.h +0 -0

index c229509288ea..a3d1380ad970 100644

arch/riscv/include/asm/tlb.h line 21

Quoted 9 lines

arch/riscv/include/asm/tlb.h +1 -1

@@ -14,11 +14,13 @@
 #ifndef _ASM_RISCV_TLB_H
 #define _ASM_RISCV_TLB_H
 
-#include <asm-generic/tlb.h>
+struct mmu_gather;
 
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 	flush_tlb_mm(tlb->mm);

Bah, didn't spot the dereference so this won't work either. You basically just need to copy what I did for arm64 in d475fac95779.

Will

Review

Quoted 6 lines

(quoted diff) +1 -1

-#include <asm-generic/tlb.h>
+struct mmu_gather;
 
  static inline void tlb_flush(struct mmu_gather *tlb)
  {
  	flush_tlb_mm(tlb->mm);

Bah, didn't spot the dereference so this won't work either. You basically just need to copy what I did for arm64 in d475fac95779.

Quoted 9 lines · Will Deacon

Review

Quoted 5 lines

(quoted diff) +1 -1

-#include <asm-generic/tlb.h>
+struct mmu_gather;
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 	flush_tlb_mm(tlb->mm);

Bah, didn't spot the dereference so this won't work either. You basically just need to copy what I did for arm64 in d475fac95779.

Yes, this seems to work. It doesn't really need "struct mmu_gather;" - I assume that is included from elsewhere - but I added it to be safe.

Can you send a full patch, or do you want me to do it ?

Quoted 12 lines · Guenter Roeck

Quoted 9 lines · Will Deacon

Review

Quoted 5 lines

(quoted diff) +1 -1

-#include <asm-generic/tlb.h>
+struct mmu_gather;
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 	flush_tlb_mm(tlb->mm);

Bah, didn't spot the dereference so this won't work either. You basically just need to copy what I did for arm64 in d475fac95779.

Quoted 2 lines

Yes, this seems to work. It doesn't really need "struct mmu_gather;" - I assume that is included from elsewhere - but I added it to be safe.

struct mmu_gather comes in via asm-generic/tlb.h.

Quoted 1 line

Can you send a full patch, or do you want me to do it ?

I'm evidently incapable of writing code today, so please go ahead :)

Quoted 32 lines · Will Deacon

Quoted 30 lines · Guenter Roeck

Quoted 10 lines · Will Deacon

Quoted 9 lines · Guenter Roeck

Quoted 8 lines · Nicholas Piggin

The generic tlb_end_vma does not call invalidate_range mmu notifier, and it resets resets the mmu_gather range, which means the notifier won't be called on part of the range in case of an unmap that spans multiple vmas.

ARM64 seems to be the only arch I could see that has notifiers and uses the generic tlb_end_vma. I have not actually tested it.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Signed-off-by Nicholas Piggin <[email protected]>

Acked-by Will Deacon <[email protected]>

This patch breaks riscv builds in mainline.

Looks very similar to the breakage we hit on arm64. diff below should fix it.

Unfortunately it doesn't.

In file included from ./arch/riscv/include/asm/pgtable.h:26:0,

                 from ./include/linux/memremap.h:7,
                 from ./include/linux/mm.h:27,
                 from arch/riscv/mm/fault.c:23:
./arch/riscv/include/asm/tlb.h: In function ‘tlb_flush’:
./arch/riscv/include/asm/tlb.h:19:18: error: dereferencing pointer to incomplete type ‘struct mmu_gather’
  flush_tlb_mm(tlb->mm);
                  ^

Quoted 4 lines

Sorry, I was a bit quick of the mark there. You'll need a forward declaration for the paramater type. Here it is with a commit message, although still untested because I haven't got round to setting up a riscv toolchain yet.

Quoted 31 lines

Will

--->8

From adb9be33d68320edcda80d540a97a647792894d2 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Fri, 24 Aug 2018 14:33:48 +0100
Subject: [PATCH] riscv: tlb: Provide definition of tlb_flush() before
including tlb.h

As of commit fd1102f0aade ("mm: mmu_notifier fix for tlb_end_vma"), asm-generic/tlb.h now calls tlb_flush() from a static inline function, so we need to make sure that it's declared before #including the asm-generic header in the arch header.

Since tlb_flush() is a one-liner for riscv, we can define it before including asm-generic/tlb.h as long as we provide a forward declaration of struct mmu_gather.

Reported-by Guenter Roeck <[email protected]>

Signed-off-by Will Deacon <[email protected]>

---
 arch/riscv/include/asm/tlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

arch/riscv/include/asm/tlb.h +0 -0

index c229509288ea..a3d1380ad970 100644

arch/riscv/include/asm/tlb.h line 21

Quoted 9 lines

arch/riscv/include/asm/tlb.h +1 -1

@@ -14,11 +14,13 @@
 #ifndef _ASM_RISCV_TLB_H
 #define _ASM_RISCV_TLB_H
 
-#include <asm-generic/tlb.h>
+struct mmu_gather;
 
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 	flush_tlb_mm(tlb->mm);

Bah, didn't spot the dereference so this won't work either. You basically just need to copy what I did for arm64 in d475fac95779.

Will

These are split from some patches I posted a while back, I was going to take a look and revive the series again after your fixes go in, but having another look, it may be that your "[PATCH 3/4] mm/tlb, x86/mm: Support invalidating TLB caches for RCU_TABLE_FREE" becomes easier after my patch 1.

And I'm not convinced patch 2 is not a real bug at least for ARM64, so it may be possible to squeeze it in if it's reviewed very carefully (I need to actually reproduce and trace it).

So not signed off by yet, but if you think it might be worth doing these with your changes, it could be a slightly cleaner end result?

So right now my "tlb-fixes" branch looks like this:
[..]

I'll do a few more test builds and boots, but I think I'm going to merge it in this cleaned-up and re-ordered form.

Quoted 6 lines · Linus Torvalds

So right now my "tlb-fixes" branch looks like this:
[..]

I'll do a few more test builds and boots, but I think I'm going to merge it in this cleaned-up and re-ordered form.

In the meantime, I decided to push out that branch in case anybody wants to look at it.

I may rebase it if I - or anybody else - find anything bad there, so consider it non-stable, but I think it's in its final shape modulo issues.

Hi Linus,

Quoted 9 lines · Linus Torvalds

Quoted 6 lines · Linus Torvalds

So right now my "tlb-fixes" branch looks like this:
[..]

I'll do a few more test builds and boots, but I think I'm going to merge it in this cleaned-up and re-ordered form.

Quoted 6 lines

In the meantime, I decided to push out that branch in case anybody wants to look at it.

I may rebase it if I - or anybody else - find anything bad there, so consider it non-stable, but I think it's in its final shape modulo issues.

Unfortunately, that branch doesn't build for arm64 because of Nick's patch moving tlb_flush_mmu_tlbonly() into tlb.h (which I acked!). It's a static inline which calls tlb_flush(), which in our case is also a static inline but one that is defined in our asm/tlb.h after including asm-generic/tlb.h.

Ah, just noticed you've pushed this to master! Please could you take the arm64 patch below on top, in order to fix the build?

Cheers,

Will

--->8

From 48ea452db90a91ff2b62a94277daf565e715a126 Mon Sep 17 00:00:00 2001
From: Will Deacon <[email protected]>
Date: Fri, 24 Aug 2018 00:23:04 +0100
Subject: [PATCH] arm64: tlb: Provide forward declaration of tlb_flush() before
including tlb.h

As of commit fd1102f0aade ("mm: mmu_notifier fix for tlb_end_vma"), asm-generic/tlb.h now calls tlb_flush() from a static inline function, so we need to make sure that it's declared before #including the asm-generic header in the arch header.

Signed-off-by Will Deacon <[email protected]>

---
 arch/arm64/include/asm/tlb.h | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/include/asm/tlb.h +2 -0

index 0ad1cf233470..a3233167be60 100644
@@ -33,6 +33,8 @@ static inline void __tlb_remove_table(void *_table)
 #define tlb_remove_entry(tlb, entry)	tlb_remove_page(tlb, entry)
 #endif /* CONFIG_HAVE_RCU_TABLE_FREE */
 
+static void tlb_flush(struct mmu_gather *tlb);
+
 #include <asm-generic/tlb.h>
 
 static inline void tlb_flush(struct mmu_gather *tlb)

Quoted 7 lines · Nicholas Piggin

These are split from some patches I posted a while back, I was going to take a look and revive the series again after your fixes go in, but having another look, it may be that your "[PATCH 3/4] mm/tlb, x86/mm: Support invalidating TLB caches for RCU_TABLE_FREE" becomes easier after my patch 1.

Quoted 6 lines

And I'm not convinced patch 2 is not a real bug at least for ARM64, so it may be possible to squeeze it in if it's reviewed very carefully (I need to actually reproduce and trace it).

So not signed off by yet, but if you think it might be worth doing these with your changes, it could be a slightly cleaner end result?

Actually, you did have sign-offs, and yes, that patch 1/2 does actually clean up and simplify the HAVE_RCU_TABLE_INVALIDATE fix, so I decided to mix these in with PeterZ's series.

And since it turns out that patch doesn't apparently matter for correctness and doesn't need to be backported to stable, I put it at the end of the series together with the x86 cleanup patch to avoid the unnecessary RCU-delayed freeing entirely for the non-PV case.

So right now my "tlb-fixes" branch looks like this:

x86/mm/tlb: Revert the recent lazy TLB patches

 *  mm: move tlb_table_flush to tlb_flush_mmu_free
 *  mm/tlb: Remove tlb_remove_table() non-concurrent condition
 *  mm/tlb, x86/mm: Support invalidating TLB caches for RCU_TABLE_FREE

mm: mmu_notifier fix for tlb_end_vma x86/mm: Only use tlb_remove_table() for paravirt

where the three starred patches are marked for stable.

The initial revert is for this merge window only, and the two last patches are cleanups and fixes but shouldn't matter for correctness in stable.

PeterZ - your "mm/tlb, x86/mm: Support invalidating TLB caches for RCU_TABLE_FREE" patch looks exactly the same, but it now no longer has the split of tlb_flush_mmu_tlbonly(), since with Nick's patch to move the call to tlb_table_flush(tlb) into tlb_flush_mmu_free, there's no need for the separate double-underscore version.

I hope nothing I did screwed things up. It all looks sane to me. Famous last words.

I'll do a few more test builds and boots, but I think I'm going to merge it in this cleaned-up and re-ordered form.

PeterZ - your "mm/tlb, x86/mm: Support invalidating TLB caches for RCU_TABLE_FREE" patch looks exactly the same, but it now no longer has the split of tlb_flush_mmu_tlbonly(), since with Nick's patch to move the call to tlb_table_flush(tlb) into tlb_flush_mmu_free, there's no need for the separate double-underscore version.

I hope nothing I did screwed things up. It all looks sane to me. Famous last words.