x86/irq: do not substract irq_tlb_count from irq_call_count

PATCH In review Aaron Lu 4 participants 6 messages Aug 11, 2016 – Aug 12, 2016 x86

Share & tools

Aaron Lu

author

2016-08-11 07:44 UTC #0 lore raw

PATCH 1/1 x86/irq: do not substract irq_tlb_count from irq_call_count

Since commit 52aec3308db8 ("x86/tlb: replace INVALIDATE_TLB_VECTOR by CALL_FUNCTION_VECTOR"), the tlb remote shootdown is done through call function vector. That commit didn't take care of irq_tlb_count so later commit fd0f5869724f ("x86: Distinguish TLB shootdown interrupts from other functions call interrupts") tried to fix it.

The fix assumes every increase of irq_tlb_count has a corresponding increase of irq_call_count. So the irq_call_count is always bigger than irq_tlb_count and we could substract irq_tlb_count from irq_call_count.

Unfortunately this is not true for the smp_call_function_single case. The IPI is only sent if the target CPU's call_single_queue is empty when adding a csd into it in generic_exec_single. That means if two threads are both adding flush tlb csds to the same CPU's call_single_queue, only one IPI is sent. In other words, the irq_call_count is incremented by 1 but irq_tlb_count is incremented by 2. Over time, irq_tlb_count will be bigger than irq_call_count and the substract will produce a very large irq_call_count value due to overflow.

Considering that:
1 it's not worth to send more IPIs for the sake of accurate counting of
irq_call_count in generic_exec_single;
2 it's not easy to tell if the call function interrupt is for TLB
shootdown in __smp_call_function_single_interrupt.
Not to exclude TLB shootdown from call function count seems to be the simplest fix and this patch just did that.

This is found by LKP's cyclic performance regression tracking recently with the vm-scalability test suite. I have bisected to commit 0a7ce4b5a632 ("mm/rmap: share the i_mmap_rwsem"). This commit didn't do anything wrong but revealed the irq_call_count problem. IIUC, the commit makes rwc->remap_one in rmap_walk_file concurrent with multiple threads. When remap_one is try_to_unmap_one, then multiple threads could queue flush tlb to the same CPU but only one IPI will be sent.

Since the commit enter Linux v3.19, the counting problem only shows up from v3.19. Considering this is a behaviour change, I'm not sure if I should add the stable tag here.

Signed-off-by Aaron Lu <[email protected]>

---
 arch/x86/include/asm/hardirq.h | 4 ----
 arch/x86/kernel/irq.c          | 3 +--
 2 files changed, 1 insertion(+), 6 deletions(-)

arch/x86/include/asm/hardirq.h +0 -4

index 7178043b0e1d..59405a248fc2 100644
@@ -22,10 +22,6 @@ typedef struct {
 #ifdef CONFIG_SMP
 	unsigned int irq_resched_count;
 	unsigned int irq_call_count;
-	/*
-	 * irq_tlb_count is double-counted in irq_call_count, so it must be
-	 * subtracted from irq_call_count when displaying irq_call_count
-	 */
 	unsigned int irq_tlb_count;
 #endif
 #ifdef CONFIG_X86_THERMAL_VECTOR

arch/x86/kernel/irq.c +1 -2

index 61521dc19c10..9f669fdd2010 100644
@@ -102,8 +102,7 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 	seq_puts(p, "  Rescheduling interrupts\n");
 	seq_printf(p, "%*s: ", prec, "CAL");
 	for_each_online_cpu(j)
-		seq_printf(p, "%10u ", irq_stats(j)->irq_call_count -
-					irq_stats(j)->irq_tlb_count);
+		seq_printf(p, "%10u ", irq_stats(j)->irq_call_count);
 	seq_puts(p, "  Function call interrupts\n");
 	seq_printf(p, "%*s: ", prec, "TLB");
 	for_each_online_cpu(j)

Signature · 1 line

2.5.5

Ingo Molnar

2016-08-11 09:13 UTC #1 lore raw

Quoted 7 lines · Aaron Lu

This is found by LKP's cyclic performance regression tracking recently with the vm-scalability test suite. I have bisected to commit 0a7ce4b5a632 ("mm/rmap: share the i_mmap_rwsem"). This commit didn't do anything wrong but revealed the irq_call_count problem. IIUC, the commit makes rwc->remap_one in rmap_walk_file concurrent with multiple threads. When remap_one is try_to_unmap_one, then multiple threads could queue flush tlb to the same CPU but only one IPI will be sent.

Note, for some reason the commit ID you used is wrong, the real one is:

3dec0ba0be6a ("mm/rmap: share the i_mmap_rwsem")

I have fixed this in the changelog.

Thanks,

Ingo

Aaron Lu

author

2016-08-11 09:17 UTC #2 lore raw

Quoted 11 lines · Ingo Molnar

Quoted 7 lines · Aaron Lu
This is found by LKP's cyclic performance regression tracking recently with the vm-scalability test suite. I have bisected to commit 0a7ce4b5a632 ("mm/rmap: share the i_mmap_rwsem"). This commit didn't do anything wrong but revealed the irq_call_count problem. IIUC, the commit makes rwc->remap_one in rmap_walk_file concurrent with multiple threads. When remap_one is try_to_unmap_one, then multiple threads could queue flush tlb to the same CPU but only one IPI will be sent.

Quoted 1 line

Note, for some reason the commit ID you used is wrong, the real one is:

Oops, I looked at a test branch where I cherry-picked that commit, sorry.

Quoted 4 lines

3dec0ba0be6a ("mm/rmap: share the i_mmap_rwsem")
I have fixed this in the changelog.

Thanks!

tip-bot for Aaron Lu

merge bot

2016-08-11 11:59 UTC #3 lore raw

[tip:x86/urgent] x86/irq: Do not substract irq_tlb_count from irq_call_count

In reply to Aaron Lu

x86/irq: Do not substract irq_tlb_count from irq_call_count

Commit-ID:  82ba4faca1bffad429f15c90c980ffd010366c25
Gitweb:     http://git.kernel.org/tip/82ba4faca1bffad429f15c90c980ffd010366c25
Author:     Aaron Lu <[email protected]>
AuthorDate: Thu, 11 Aug 2016 15:44:30 +0800
Committer:  Ingo Molnar <[email protected]>
CommitDate: Thu, 11 Aug 2016 11:14:59 +0200

x86/irq: Do not substract irq_tlb_count from irq_call_count

Since commit:

52aec3308db8 ("x86/tlb: replace INVALIDATE_TLB_VECTOR by CALL_FUNCTION_VECTOR")

the TLB remote shootdown is done through call function vector. That commit didn't take care of irq_tlb_count, which a later commit:

fd0f5869724f ("x86: Distinguish TLB shootdown interrupts from other functions call interrupts")

... tried to fix.

Unfortunately this is not true for the smp_call_function_single() case. The IPI is only sent if the target CPU's call_single_queue is empty when adding a csd into it in generic_exec_single. That means if two threads are both adding flush tlb csds to the same CPU's call_single_queue, only one IPI is sent. In other words, the irq_call_count is incremented by 1 but irq_tlb_count is incremented by 2. Over time, irq_tlb_count will be bigger than irq_call_count and the substract will produce a very large irq_call_count value due to overflow.

Considering that:

1) it's not worth to send more IPIs for the sake of accurate counting of

     irq_call_count in generic_exec_single();

2) it's not easy to tell if the call function interrupt is for TLB shootdown in __smp_call_function_single_interrupt().

Not to exclude TLB shootdown from call function count seems to be the simplest fix and this patch just does that.

This bug was found by LKP's cyclic performance regression tracking recently with the vm-scalability test suite. I have bisected to commit:

3dec0ba0be6a ("mm/rmap: share the i_mmap_rwsem")

This commit didn't do anything wrong but revealed the irq_call_count problem. IIUC, the commit makes rwc->remap_one in rmap_walk_file concurrent with multiple threads. When remap_one is try_to_unmap_one(), then multiple threads could queue flush TLB to the same CPU but only one IPI will be sent.

Since the commit was added in Linux v3.19, the counting problem only shows up from v3.19 onwards.

Signed-off-by Aaron Lu <[email protected]>

Cc Alex Shi <[email protected]>

Cc Andy Lutomirski <[email protected]>

Cc Borislav Petkov <[email protected]>

Cc Brian Gerst <[email protected]>

Cc Davidlohr Bueso <[email protected]>

Cc Denys Vlasenko <[email protected]>

Cc H. Peter Anvin <[email protected]>

Cc Huang Ying <[email protected]>

Cc Josh Poimboeuf <[email protected]>

Cc Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Thomas Gleixner <[email protected]>

Cc Tomoki Sekiyama <[email protected]>

Link http://lkml.kernel.org/r/[email protected] open here

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/include/asm/hardirq.h | 4 ----
 arch/x86/kernel/irq.c          | 3 +--
 2 files changed, 1 insertion(+), 6 deletions(-)

arch/x86/include/asm/hardirq.h +0 -4

index 7178043..59405a2 100644
@@ -22,10 +22,6 @@ typedef struct {
 #ifdef CONFIG_SMP
 	unsigned int irq_resched_count;
 	unsigned int irq_call_count;
-	/*
-	 * irq_tlb_count is double-counted in irq_call_count, so it must be
-	 * subtracted from irq_call_count when displaying irq_call_count
-	 */
 	unsigned int irq_tlb_count;
 #endif
 #ifdef CONFIG_X86_THERMAL_VECTOR

arch/x86/kernel/irq.c +1 -2

index 61521dc..9f669fd 100644
@@ -102,8 +102,7 @@ int arch_show_interrupts(struct seq_file *p, int prec)
 	seq_puts(p, "  Rescheduling interrupts\n");
 	seq_printf(p, "%*s: ", prec, "CAL");
 	for_each_online_cpu(j)
-		seq_printf(p, "%10u ", irq_stats(j)->irq_call_count -
-					irq_stats(j)->irq_tlb_count);
+		seq_printf(p, "%10u ", irq_stats(j)->irq_call_count);
 	seq_puts(p, "  Function call interrupts\n");
 	seq_printf(p, "%*s: ", prec, "TLB");
 	for_each_online_cpu(j)

Huang, Ying

2016-08-11 15:13 UTC #4 lore raw

In reply to Aaron Lu

Quoted 35 lines · Aaron Lu

Since commit 52aec3308db8 ("x86/tlb: replace INVALIDATE_TLB_VECTOR by CALL_FUNCTION_VECTOR"), the tlb remote shootdown is done through call function vector. That commit didn't take care of irq_tlb_count so later commit fd0f5869724f ("x86: Distinguish TLB shootdown interrupts from other functions call interrupts") tried to fix it.
The fix assumes every increase of irq_tlb_count has a corresponding increase of irq_call_count. So the irq_call_count is always bigger than irq_tlb_count and we could substract irq_tlb_count from irq_call_count.
Unfortunately this is not true for the smp_call_function_single case. The IPI is only sent if the target CPU's call_single_queue is empty when adding a csd into it in generic_exec_single. That means if two threads are both adding flush tlb csds to the same CPU's call_single_queue, only one IPI is sent. In other words, the irq_call_count is incremented by 1 but irq_tlb_count is incremented by 2. Over time, irq_tlb_count will be bigger than irq_call_count and the substract will produce a very large irq_call_count value due to overflow.
Considering that:
1 it's not worth to send more IPIs for the sake of accurate counting of
irq_call_count in generic_exec_single;
2 it's not easy to tell if the call function interrupt is for TLB
shootdown in __smp_call_function_single_interrupt.
Not to exclude TLB shootdown from call function count seems to be the simplest fix and this patch just did that.
This is found by LKP's cyclic performance regression tracking recently with the vm-scalability test suite. I have bisected to commit 0a7ce4b5a632 ("mm/rmap: share the i_mmap_rwsem"). This commit didn't do anything wrong but revealed the irq_call_count problem. IIUC, the commit makes rwc->remap_one in rmap_walk_file concurrent with multiple threads. When remap_one is try_to_unmap_one, then multiple threads could queue flush tlb to the same CPU but only one IPI will be sent.

Quoted 5 lines

Since the commit enter Linux v3.19, the counting problem only shows up from v3.19. Considering this is a behaviour change, I'm not sure if I should add the stable tag here.
Signed-off-by Aaron Lu <[email protected]>

Thanks for fix. You forget to add :)

Reported-by "Huang, Ying" <[email protected]>

Best Regards,
Huang, Ying

Aaron Lu

author

2016-08-12 02:16 UTC #5 lore raw

Quoted 41 lines · Ying

Quoted 39 lines · Aaron Lu
Since commit 52aec3308db8 ("x86/tlb: replace INVALIDATE_TLB_VECTOR by CALL_FUNCTION_VECTOR"), the tlb remote shootdown is done through call function vector. That commit didn't take care of irq_tlb_count so later commit fd0f5869724f ("x86: Distinguish TLB shootdown interrupts from other functions call interrupts") tried to fix it.
The fix assumes every increase of irq_tlb_count has a corresponding increase of irq_call_count. So the irq_call_count is always bigger than irq_tlb_count and we could substract irq_tlb_count from irq_call_count.
Unfortunately this is not true for the smp_call_function_single case. The IPI is only sent if the target CPU's call_single_queue is empty when adding a csd into it in generic_exec_single. That means if two threads are both adding flush tlb csds to the same CPU's call_single_queue, only one IPI is sent. In other words, the irq_call_count is incremented by 1 but irq_tlb_count is incremented by 2. Over time, irq_tlb_count will be bigger than irq_call_count and the substract will produce a very large irq_call_count value due to overflow.
Considering that:
1 it's not worth to send more IPIs for the sake of accurate counting of
irq_call_count in generic_exec_single;
2 it's not easy to tell if the call function interrupt is for TLB
shootdown in __smp_call_function_single_interrupt.
Not to exclude TLB shootdown from call function count seems to be the simplest fix and this patch just did that.
This is found by LKP's cyclic performance regression tracking recently with the vm-scalability test suite. I have bisected to commit 0a7ce4b5a632 ("mm/rmap: share the i_mmap_rwsem"). This commit didn't do anything wrong but revealed the irq_call_count problem. IIUC, the commit makes rwc->remap_one in rmap_walk_file concurrent with multiple threads. When remap_one is try_to_unmap_one, then multiple threads could queue flush tlb to the same CPU but only one IPI will be sent.
Since the commit enter Linux v3.19, the counting problem only shows up from v3.19. Considering this is a behaviour change, I'm not sure if I should add the stable tag here.

Quoted 5 lines

Quoted 1 line
Signed-off-by Aaron Lu <[email protected]>
Thanks for fix. You forget to add :)
Reported-by "Huang, Ying" <[email protected]>

Oh right, sorry about that.

Regards,
Aaron