x86/mm/tlb: ignore f->new_tlb_gen when zero

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

In addition, add the missing unlikely() and jump to get tracing right.

Fixes aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible")

Reported-by Hugh Dickins <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 arch/x86/mm/tlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/mm/tlb.c +0 -0

index d9314cc8b81f..d81b4084bb8a 100644
@@ -771,14 +771,14 @@ static void flush_tlb_func(void *info)
 		return;
 	}
 

Quoted 8 lines

arch/x86/mm/tlb.c +0 -0

index d9314cc8b81f..d81b4084bb8a 100644
@@ -771,14 +771,14 @@ static void flush_tlb_func(void *info)
 		return;
 	}
 

arch/x86/mm/tlb.c static void flush_tlb_func(void *info) · line 781

Quoted 10 lines

arch/x86/mm/tlb.c +2 -2

-	if (f->new_tlb_gen <= local_tlb_gen) {
+	if (unlikely(f->new_tlb_gen != 0 && f->new_tlb_gen <= local_tlb_gen)) {
 		/*
 		 * The TLB is already up to date in respect to f->new_tlb_gen.
 		 * While the core might be still behind mm_tlb_gen, checking
 		 * mm_tlb_gen unnecessarily would have negative caching effects
 		 * so avoid it.
 		 */
-		return;
+		goto done;

Does this affect the performance numbers from aa44284960d5 ("x86/mm/tlb:
Avoid reading mm_tlb_gen when possible")?

⚠ External Email

Quoted 8 lines · David Hildenbrand

Quoted 8 lines

arch/x86/mm/tlb.c +0 -0

index d9314cc8b81f..d81b4084bb8a 100644
@@ -771,14 +771,14 @@ static void flush_tlb_func(void *info)
            return;
    }
 

Quoted 12 lines

arch/x86/mm/tlb.c static void flush_tlb_func(void *info) · line 781

Quoted 10 lines

arch/x86/mm/tlb.c +2 -2

-    if (f->new_tlb_gen <= local_tlb_gen) {
+    if (unlikely(f->new_tlb_gen != 0 && f->new_tlb_gen <= local_tlb_gen)) {
            /*
             * The TLB is already up to date in respect to f->new_tlb_gen.
             * While the core might be still behind mm_tlb_gen, checking
             * mm_tlb_gen unnecessarily would have negative caching effects
             * so avoid it.
             */
-            return;
+            goto done;

Does this affect the performance numbers from aa44284960d5 ("x86/mm/tlb:
Avoid reading mm_tlb_gen when possible")?

It depends on how many batched flushes that workload had. From the looks of it, they're all one page:

       madvise(addr + i, pgsize, MADV_DONTNEED);

so there shouldn't be *much* batching in play. But, it wouldn't hurt to re-run them in either case.

Since the bug was not during MADV_DONTNEED there is no reason for the results to be any different.

Famous last words?

⚠ External Email

Quoted 3 lines · Nadav Amit

Since the bug was not during MADV_DONTNEED there is no reason for the results to be any different.

Quoted 1 line

Famous last words?

Considering that your patch broke the kernel a way that surprised us all, I think caution is warranted. Re-running a microbenchmark that takes five minutes and stresses things a bit is the least you can do, I think.

Quoted 8 lines · Dave Hansen

⚠ External Email

Quoted 4 lines · Nadav Amit

Since the bug was not during MADV_DONTNEED there is no reason for the results to be any different.

Famous last words?

Quoted 4 lines

Considering that your patch broke the kernel a way that surprised us all, I think caution is warranted. Re-running a microbenchmark that takes five minutes and stresses things a bit is the least you can do, I think.

I will send it later today. I was just pointing that the failing code-path is different than the one I measured.

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

In addition, add the missing unlikely() and jump to get tracing right.

 arch/x86/mm/tlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/mm/tlb.c +1 -1

index d9314cc8b81f..d81b4084bb8a 100644
@@ -771,14 +771,14 @@ static void flush_tlb_func(void *info)
 		return;
 	}
 
-	if (f->new_tlb_gen <= local_tlb_gen) {
+	if (unlikely(f->new_tlb_gen != 0 && f->new_tlb_gen <= local_tlb_gen)) {
 		/*

⚠ External Email

On 7/7/22 17:30, Nadav Amit wrote:

You might want to fix the clock on the system from which you sent this. I was really scratching my head trying to figure out how you got this patch out before Hugh's bug report.

Quoted 11 lines

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Quoted 2 lines

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

It is, but the check below uses 'f->end == TLB_FLUSH_ALL' as the marker for f->new_tlb_gen being invalid. Being consistent seems like a good idea on this stuff.

Quoted 2 lines

In addition, add the missing unlikely() and jump to get tracing right.

There are currently five routes out of flush_tlb_func():
* Three early returns
* One 'goto done'
* One implicit return

The tracing code doesn't get run for any of the early returns, but that's intentional because they don't *actually* flush the TLB. We don't want to record that flush_tlb_func() flushed the TLB when it didn't. There's another tracepoint on the TLB_REMOTE_SEND_IPI side to tell where the flushes were requested.

That said, I think the

       if (unlikely(local_tlb_gen == mm_tlb_gen))
               goto done;

is arguably buggy, as is the 'goto done' in this hunk:

We might want to (eventually) think about doing something like the attached patch to make the skipped flushes explicit in the tracing and make the return paths out of this function more consistent.

Quoted 16 lines · Dave Hansen

On 7/7/22 17:30, Nadav Amit wrote:
You might want to fix the clock on the system from which you sent this. I was really scratching my head trying to figure out how you got this patch out before Hugh's bug report.

Quoted 11 lines

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Quoted 6 lines

Quoted 2 lines

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

It is, but the check below uses 'f->end == TLB_FLUSH_ALL' as the marker for f->new_tlb_gen being invalid. Being consistent seems like a good idea on this stuff.

If we get a request to do a flush, regardless whether full or partial, that logically we have already done, there is not reason to do it.

I therefore do not see a reason to look on f->end. I think that looking at the generation is very intuitive. If you want, I can add a constant such as TLB_GENERATION_INVALID.

⚠ External Email

Quoted 24 lines · Nadav Amit

Quoted 16 lines · Dave Hansen

On 7/7/22 17:30, Nadav Amit wrote:
You might want to fix the clock on the system from which you sent this. I was really scratching my head trying to figure out how you got this patch out before Hugh's bug report.

Quoted 11 lines

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Quoted 6 lines

Quoted 2 lines

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

It is, but the check below uses 'f->end == TLB_FLUSH_ALL' as the marker for f->new_tlb_gen being invalid. Being consistent seems like a good idea on this stuff.

Quoted 6 lines

If we get a request to do a flush, regardless whether full or partial, that logically we have already done, there is not reason to do it.

I therefore do not see a reason to look on f->end. I think that looking at the generation is very intuitive. If you want, I can add a constant such as TLB_GENERATION_INVALID.

That's a good point.

But, _my_ point was that there was only really one read site of f->new_tlb_gen in flush_tlb_func(). That site is guarded by the "f->end != TLB_FLUSH_ALL" check which prevented it from making the same error that your patch did.

Whatever we do, it would be nice to have a *single* way to check for "does f->new_tlb_gen have an actual, valid bit of tlb gen data in it?"

Using something like TLB_GENERATION_INVALID seems reasonable to me.

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

In addition, add the missing unlikely() and jump to get tracing right.

Fixes aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible")

Reported-by Hugh Dickins <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

---
 arch/x86/mm/tlb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/x86/mm/tlb.c +2 -2

index d9314cc8b81f..d81b4084bb8a 100644
@@ -771,14 +771,14 @@ static void flush_tlb_func(void *info)
 		return;
 	}
 
-	if (f->new_tlb_gen <= local_tlb_gen) {
+	if (unlikely(f->new_tlb_gen != 0 && f->new_tlb_gen <= local_tlb_gen)) {
 		/*
 		 * The TLB is already up to date in respect to f->new_tlb_gen.
 		 * While the core might be still behind mm_tlb_gen, checking
 		 * mm_tlb_gen unnecessarily would have negative caching effects
 		 * so avoid it.
 		 */
-		return;
+		goto done;
 	}
 
 	/*

Signature · 1 line

2.25.1

⚠ External Email

Quoted 14 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

Quoted 8 lines

In addition, add the missing unlikely() and jump to get tracing right.

Fixes aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible")

Reported-by Hugh Dickins <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

Thanks a lot for your rapid response and thinking it through (before I got around to any "nopcid" or "nopti" experiments).

I've been testing this one for a few hours now, and no problems seen. I expect you'll be sending another version, maybe next week, meeting Dave's concerns; but wanted to reassure that you have correctly identified the issue and fixed it with this - thanks.

Quoted 30 lines · Hugh Dickins

⚠ External Email

Quoted 14 lines · Nadav Amit

From: Nadav Amit <[email protected]>

Commit aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible") introduced an optimization of skipping the flush if the TLB generation that is flushed (as provided in flush_tlb_info) was already flushed.

However, arch_tlbbatch_flush() does not provide any generation in flush_tlb_info. As a result, try_to_unmap_one() would not perform any TLB flushes.

Fix it by checking whether f->new_tlb_gen is nonzero. Zero value is anyhow is an invalid generation value.

Quoted 8 lines

In addition, add the missing unlikely() and jump to get tracing right.

Fixes aa44284960d5 ("x86/mm/tlb: Avoid reading mm_tlb_gen when possible")

Reported-by Hugh Dickins <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Peter Zijlstra (Intel) <[email protected]>

Cc Andy Lutomirski <[email protected]>

Signed-off-by Nadav Amit <[email protected]>

Thanks a lot for your rapid response and thinking it through (before I got around to any "nopcid" or "nopti" experiments).

Quoted 4 lines

I've been testing this one for a few hours now, and no problems seen. I expect you'll be sending another version, maybe next week, meeting Dave's concerns; but wanted to reassure that you have correctly identified the issue and fixed it with this - thanks.

Thanks, Hugh. Sorry again for my mistake.

Can I please have your “Tested-by”?