[RFC PATCH 0/2] minor mmu_gather patches

[RFC PATCH 0/2] minor mmu_gather patches

[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.

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?

Thanks,
Nick


Nicholas Piggin (2):
  mm: move tlb_table_flush to tlb_flush_mmu_free
  mm: mmu_notifier fix for tlb_end_vma

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

-- 
2.17.0

[RFC PATCH 1/2] mm: move tlb_table_flush to tlb_flush_mmu_free

[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 <npiggin@gmail.com>
---
 mm/memory.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 19f47d7b9b86..7c58310734eb 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -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;
-- 
2.17.0

Re: [RFC PATCH 1/2] mm: move tlb_table_flush to tlb_flush_mmu_free

[Will Deacon]

On Thu, Aug 23, 2018 at 06:47:08PM +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>
> ---
>  mm/memory.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)

Looks good to me, thanks:

Acked-by: Will Deacon <will.deacon@arm.com>

Will

> diff --git a/mm/memory.c b/mm/memory.c
> index 19f47d7b9b86..7c58310734eb 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -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;
> -- 
> 2.17.0
> 

Re: [RFC PATCH 1/2] mm: move tlb_table_flush to tlb_flush_mmu_free

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 694 bytes --]

On Thu, 2018-08-23 at 18:47 +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>

Reviewed-by: Rik van Riel <riel@surriel.com>

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.

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH 1/2] mm: move tlb_table_flush to tlb_flush_mmu_free

[Will Deacon]

On Thu, Sep 06, 2018 at 04:29:59PM -0400, Rik van Riel wrote:
> On Thu, 2018-08-23 at 18:47 +1000, Nicholas Piggin wrote:
> > 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 <npiggin@gmail.com>
> 
> Reviewed-by: Rik van Riel <riel@surriel.com>
> 
> 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.

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

Will

Re: [RFC PATCH 1/2] mm: move tlb_table_flush to tlb_flush_mmu_free

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 1077 bytes --]

On Fri, 2018-09-07 at 14:44 +0100, Will Deacon wrote:
> On Thu, Sep 06, 2018 at 04:29:59PM -0400, Rik van Riel wrote:
> > On Thu, 2018-08-23 at 18:47 +1000, Nicholas Piggin wrote:
> > > 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 <npiggin@gmail.com>
> > 
> > Reviewed-by: Rik van Riel <riel@surriel.com>
> > 
> > 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.
> 
> It's already in mainline with a cc stable afaict.

Sure enough, it is.

I guess I have too many kernel trees on this
system, and was looking at the wrong one somehow.

-- 
All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma

[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.

Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/asm-generic/tlb.h | 17 +++++++++++++----
 mm/memory.c               | 10 ----------
 2 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index e811ef7b8350..79cb76b89c26 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -15,6 +15,7 @@
 #ifndef _ASM_GENERIC__TLB_H
 #define _ASM_GENERIC__TLB_H
 
+#include <linux/mmu_notifier.h>
 #include <linux/swap.h>
 #include <asm/pgalloc.h>
 #include <asm/tlbflush.h>
@@ -138,6 +139,16 @@ static inline void __tlb_reset_range(struct mmu_gather *tlb)
 	}
 }
 
+static inline 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);
+	__tlb_reset_range(tlb);
+}
+
 static inline void tlb_remove_page_size(struct mmu_gather *tlb,
 					struct page *page, int page_size)
 {
@@ -186,10 +197,8 @@ static inline void tlb_remove_check_page_size_change(struct mmu_gather *tlb,
 
 #define __tlb_end_vma(tlb, vma)					\
 	do {							\
-		if (!tlb->fullmm && tlb->end) {			\
-			tlb_flush(tlb);				\
-			__tlb_reset_range(tlb);			\
-		}						\
+		if (!tlb->fullmm)				\
+			tlb_flush_mmu_tlbonly(tlb);		\
 	} while (0)
 
 #ifndef tlb_end_vma
diff --git a/mm/memory.c b/mm/memory.c
index 7c58310734eb..c4a7b6cb17c8 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -238,16 +238,6 @@ void arch_tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm,
 	__tlb_reset_range(tlb);
 }
 
-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);
-	__tlb_reset_range(tlb);
-}
-
 static void tlb_flush_mmu_free(struct mmu_gather *tlb)
 {
 	struct mmu_gather_batch *batch;
-- 
2.17.0

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma

[Catalin Marinas]

On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> 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.

We only care about notifiers for KVM but I think it only makes use of
mmu_notifier_invalidate_range_(start|end) which are not affected by the
range reset in mmu_gather.

Your patch looks ok from an arm64 perspective (it would be good if Will
has a look as well since he was the last to touch this part for arm64).

-- 
Catalin

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma

[Will Deacon]

On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>
> ---
>  include/asm-generic/tlb.h | 17 +++++++++++++----
>  mm/memory.c               | 10 ----------
>  2 files changed, 13 insertions(+), 14 deletions(-)

I think we only use the notifiers in the KVM code, which appears to leave
the ->invalidate_range() callback empty, so that at least explains why we
haven't run into problems here.

But the change looks correct to me, so:

Acked-by: Will Deacon <will.deacon@arm.com>

Thanks,

Will

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Guenter Roeck]

On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>
> Acked-by: Will Deacon <will.deacon@arm.com>

This patch breaks riscv builds in mainline.

Building riscv:defconfig ... failed
--------------
Error log:
In file included from riscv/include/asm/tlb.h:17:0,
                 from arch/riscv/include/asm/pgalloc.h:19,
                 from riscv/mm/fault.c:30:
include/asm-generic/tlb.h: In function 'tlb_flush_mmu_tlbonly':
include/asm-generic/tlb.h:147:2: error: implicit declaration of function 'tlb_flush'

In file included from arch/riscv/include/asm/pgalloc.h:19:0,
		from arch/riscv/mm/fault.c:30:
arch/riscv/include/asm/tlb.h: At top level:
arch/riscv/include/asm/tlb.h:19:20: warning: conflicting types for 'tlb_flush'

Guenter

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Will Deacon]

On Fri, Aug 24, 2018 at 06:07:22AM -0700, Guenter Roeck wrote:
> On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> > 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 <npiggin@gmail.com>
> > Acked-by: Will Deacon <will.deacon@arm.com>
> 
> This patch breaks riscv builds in mainline.

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

Will

--->*

diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
index c229509288ea..5017060be63c 100644
--- a/arch/riscv/include/asm/tlb.h
+++ b/arch/riscv/include/asm/tlb.h
@@ -14,11 +14,11 @@
 #ifndef _ASM_RISCV_TLB_H
 #define _ASM_RISCV_TLB_H
 
-#include <asm-generic/tlb.h>
-
 static inline void tlb_flush(struct mmu_gather *tlb)
 {
 	flush_tlb_mm(tlb->mm);
 }
 
+#include <asm-generic/tlb.h>
+
 #endif /* _ASM_RISCV_TLB_H */

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Guenter Roeck]

On Fri, Aug 24, 2018 at 02:10:27PM +0100, Will Deacon wrote:
> On Fri, Aug 24, 2018 at 06:07:22AM -0700, Guenter Roeck wrote:
> > On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> > > 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 <npiggin@gmail.com>
> > > Acked-by: Will Deacon <will.deacon@arm.com>
> > 
> > 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);
                  ^
./arch/riscv/include/asm/tlbflush.h:58:35: note: in definition of macro ‘flush_tlb_mm’
  sbi_remote_sfence_vma(mm_cpumask(mm)->bits, 0, -1)

Note that reverting the offending patch does fix the problem,
so there is no secondary problem lurking around.

Guenter

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Will Deacon]

On Fri, Aug 24, 2018 at 06:24:19AM -0700, Guenter Roeck wrote:
> On Fri, Aug 24, 2018 at 02:10:27PM +0100, Will Deacon wrote:
> > On Fri, Aug 24, 2018 at 06:07:22AM -0700, Guenter Roeck wrote:
> > > On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> > > > 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 <npiggin@gmail.com>
> > > > Acked-by: Will Deacon <will.deacon@arm.com>
> > > 
> > > 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);
>                   ^

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 <will.deacon@arm.com>
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 <linux@roeck-us.net>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 arch/riscv/include/asm/tlb.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
index c229509288ea..a3d1380ad970 100644
--- a/arch/riscv/include/asm/tlb.h
+++ b/arch/riscv/include/asm/tlb.h
@@ -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);
 }
 
+#include <asm-generic/tlb.h>
+
 #endif /* _ASM_RISCV_TLB_H */
-- 
2.7.4

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Will Deacon]

On Fri, Aug 24, 2018 at 02:34:27PM +0100, Will Deacon wrote:
> On Fri, Aug 24, 2018 at 06:24:19AM -0700, Guenter Roeck wrote:
> > On Fri, Aug 24, 2018 at 02:10:27PM +0100, Will Deacon wrote:
> > > On Fri, Aug 24, 2018 at 06:07:22AM -0700, Guenter Roeck wrote:
> > > > On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
> > > > > 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 <npiggin@gmail.com>
> > > > > Acked-by: Will Deacon <will.deacon@arm.com>
> > > > 
> > > > 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);
> >                   ^
> 
> 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 <will.deacon@arm.com>
> 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 <linux@roeck-us.net>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  arch/riscv/include/asm/tlb.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
> index c229509288ea..a3d1380ad970 100644
> --- a/arch/riscv/include/asm/tlb.h
> +++ b/arch/riscv/include/asm/tlb.h
> @@ -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

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Guenter Roeck]

On Fri, Aug 24, 2018 at 02:50:48PM +0100, Will Deacon wrote:
> > 
> > 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.
> > 

Still doesn't work.

CC      mm/memory.o
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:21:18: error:
	dereferencing pointer to incomplete type ‘struct mmu_gather’ flush_tlb_mm(tlb->mm);

Problem is that struct mmu_gather is dereferenced in tlb_flush().

Guenter

> > Will
> > 
> > --->8
> > 
> > From adb9be33d68320edcda80d540a97a647792894d2 Mon Sep 17 00:00:00 2001
> > From: Will Deacon <will.deacon@arm.com>
> > 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 <linux@roeck-us.net>
> > Signed-off-by: Will Deacon <will.deacon@arm.com>
> > ---
> >  arch/riscv/include/asm/tlb.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
> > index c229509288ea..a3d1380ad970 100644
> > --- a/arch/riscv/include/asm/tlb.h
> > +++ b/arch/riscv/include/asm/tlb.h
> > @@ -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

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Guenter Roeck]

On 08/24/2018 06:50 AM, Will Deacon wrote:

>> -#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 ?

Thanks,
Guenter

---
diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
index c229509288ea..439dc7072e05 100644
--- a/arch/riscv/include/asm/tlb.h
+++ b/arch/riscv/include/asm/tlb.h
@@ -14,6 +14,10 @@
  #ifndef _ASM_RISCV_TLB_H
  #define _ASM_RISCV_TLB_H

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

  static inline void tlb_flush(struct mmu_gather *tlb)

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Will Deacon]

On Fri, Aug 24, 2018 at 07:06:51AM -0700, Guenter Roeck wrote:
> On 08/24/2018 06:50 AM, Will Deacon wrote:
> 
> >>-#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.

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

> 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 :)

Will

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Guenter Roeck]

On Fri, Aug 24, 2018 at 03:25:33PM +0100, Will Deacon wrote:
> On Fri, Aug 24, 2018 at 07:06:51AM -0700, Guenter Roeck wrote:
> > On 08/24/2018 06:50 AM, Will Deacon wrote:
> > 
> > >>-#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.
> 
> struct mmu_gather comes in via asm-generic/tlb.h.
> 
From linux/mm.h, really, which happens to be included before
asm/tlb.h is included (see arch/riscv/include/asm/pgalloc.h).
I kept it to be future-proof.

> > 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 :)
> 
Done. We'll see if I am any better.

Guenter

Re: [RFC PATCH 2/2] mm: mmu_notifier fix for tlb_end_vma (build failures)

[Palmer Dabbelt]

On Fri, 24 Aug 2018 06:50:48 PDT (-0700), Will Deacon wrote:
> On Fri, Aug 24, 2018 at 02:34:27PM +0100, Will Deacon wrote:
>> On Fri, Aug 24, 2018 at 06:24:19AM -0700, Guenter Roeck wrote:
>> > On Fri, Aug 24, 2018 at 02:10:27PM +0100, Will Deacon wrote:
>> > > On Fri, Aug 24, 2018 at 06:07:22AM -0700, Guenter Roeck wrote:
>> > > > On Thu, Aug 23, 2018 at 06:47:09PM +1000, Nicholas Piggin wrote:
>> > > > > 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 <npiggin@gmail.com>
>> > > > > Acked-by: Will Deacon <will.deacon@arm.com>
>> > > >
>> > > > 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);
>> >                   ^
>>
>> 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.

FWIW, Arnd built them last time he updated the cross tools so you should be 
able to get GCC 8.1.0 for RISC-V from there.  I use this make.cross script that 
I stole from the Intel 0-day robot

    https://github.com/palmer-dabbelt/home/blob/master/.local/src/local-scripts/make.cross.bash

If I'm reading it correctly the tools come from here

    http://cdn.kernel.org/pub/tools/crosstool/files/bin/x86_64/8.1.0/x86_64-gcc-8.1.0-nolibc-riscv64-linux.tar.gz

I use the make.cross script as it makes it super easy to test my 
across-the-tree patches on other people's ports.

>>
>> Will
>>
>> --->8
>>
>> From adb9be33d68320edcda80d540a97a647792894d2 Mon Sep 17 00:00:00 2001
>> From: Will Deacon <will.deacon@arm.com>
>> 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 <linux@roeck-us.net>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>  arch/riscv/include/asm/tlb.h | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/riscv/include/asm/tlb.h b/arch/riscv/include/asm/tlb.h
>> index c229509288ea..a3d1380ad970 100644
>> --- a/arch/riscv/include/asm/tlb.h
>> +++ b/arch/riscv/include/asm/tlb.h
>> @@ -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

Re: [RFC PATCH 0/2] minor mmu_gather patches

[Linus Torvalds]

On Thu, Aug 23, 2018 at 1:47 AM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> 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?

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.

                     Linus

Re: [RFC PATCH 0/2] minor mmu_gather patches

[Linus Torvalds]

On Thu, Aug 23, 2018 at 12:15 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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.

              Linus

Re: [RFC PATCH 0/2] minor mmu_gather patches

[Will Deacon]

Hi Linus,

On Thu, Aug 23, 2018 at 12:37:58PM -0700, Linus Torvalds wrote:
> On Thu, Aug 23, 2018 at 12:15 PM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> >
> > 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.

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 <will.deacon@arm.com>
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 <will.deacon@arm.com>
---
 arch/arm64/include/asm/tlb.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
index 0ad1cf233470..a3233167be60 100644
--- a/arch/arm64/include/asm/tlb.h
+++ b/arch/arm64/include/asm/tlb.h
@@ -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)
-- 
2.7.4

Re: [RFC PATCH 0/2] minor mmu_gather patches

[Nicholas Piggin]

On Fri, 24 Aug 2018 00:27:05 +0100
Will Deacon <will.deacon@arm.com> wrote:

> Hi Linus,
> 
> On Thu, Aug 23, 2018 at 12:37:58PM -0700, Linus Torvalds wrote:
> > On Thu, Aug 23, 2018 at 12:15 PM Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:  
> > >
> > > 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.  
> 
> 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?

Sorry yeah I had the sign offs but should have clear I hadn't fully
build tested them. It's reasonable for reviewers to assume basic
diligence was done and concentrate on the logic rather than build
trivialities. So that's my bad. Thanks for the fix.

Thanks,
Nick

> 
> Cheers,
> 
> Will
> 
> --->8  
> 
> From 48ea452db90a91ff2b62a94277daf565e715a126 Mon Sep 17 00:00:00 2001
> From: Will Deacon <will.deacon@arm.com>
> 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 <will.deacon@arm.com>
> ---
>  arch/arm64/include/asm/tlb.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/tlb.h b/arch/arm64/include/asm/tlb.h
> index 0ad1cf233470..a3233167be60 100644
> --- a/arch/arm64/include/asm/tlb.h
> +++ b/arch/arm64/include/asm/tlb.h
> @@ -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)

Re: [RFC PATCH 0/2] minor mmu_gather patches

[Nicholas Piggin]

On Thu, 23 Aug 2018 12:15:37 -0700
Linus Torvalds <torvalds@linux-foundation.org> wrote:

> On Thu, Aug 23, 2018 at 1:47 AM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > 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?  
> 
> 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.

I think the end result looks okay, modulo my build screw up --
at least the generic code. Thanks for putting it together.

Thanks,
Nick

Re: [RFC PATCH 0/2] minor mmu_gather patches

[Peter Zijlstra]

On Thu, Aug 23, 2018 at 12:15:37PM -0700, Linus Torvalds wrote:
> 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.

Sorry; I got distracted by building a bunk bed for the kids -- and
somehow these things always take way more time than expected.

Anyway, the code looks good to me. Thanks all!