Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1671353 > unrolled thread
| Started by | Andy Lutomirski <luto@kernel.org> |
|---|---|
| First post | 2017-06-21 07:30 +0200 |
| Last post | 2017-06-22 07:30 +0200 |
| Articles | 20 on this page of 56 — 8 participants |
Back to article view | Back to linux.kernel
[PATCH v3 00/11] PCID and improved laziness Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
[PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 15:40 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 15:50 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Andy Lutomirski <luto@kernel.org> - 2017-06-22 05:00 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Thomas Gleixner <tglx@linutronix.de> - 2017-06-22 14:30 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Andy Lutomirski <luto@kernel.org> - 2017-06-22 20:20 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Thomas Gleixner <tglx@linutronix.de> - 2017-06-22 23:30 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Andy Lutomirski <luto@kernel.org> - 2017-06-23 05:10 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Thomas Gleixner <tglx@linutronix.de> - 2017-06-23 09:30 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Nadav Amit <nadav.amit@gmail.com> - 2017-06-22 18:20 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Andy Lutomirski <luto@kernel.org> - 2017-06-22 20:20 +0200
Re: [PATCH v3 11/11] x86/mm: Try to preserve old TLB entries using PCID Borislav Petkov <bp@alien8.de> - 2017-06-26 18:10 +0200
[PATCH v3 08/11] x86/mm: Disable PCID on 32-bit kernels Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 08/11] x86/mm: Disable PCID on 32-bit kernels Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 11:30 +0200
Re: [PATCH v3 08/11] x86/mm: Disable PCID on 32-bit kernels Borislav Petkov <bp@alien8.de> - 2017-06-23 11:30 +0200
[PATCH v3 02/11] x86/ldt: Simplify LDT switching logic Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 02/11] x86/ldt: Simplify LDT switching logic Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 10:10 +0200
Re: [PATCH v3 02/11] x86/ldt: Simplify LDT switching logic Borislav Petkov <bp@alien8.de> - 2017-06-21 11:50 +0200
[tip:x86/mm] x86/ldt: Simplify the LDT switching logic tip-bot for Andy Lutomirski <tipbot@zytor.com> - 2017-06-22 13:20 +0200
[PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 10:10 +0200
Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Borislav Petkov <bp@alien8.de> - 2017-06-21 10:50 +0200
Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Andy Lutomirski <luto@kernel.org> - 2017-06-21 17:20 +0200
Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Nadav Amit <nadav.amit@gmail.com> - 2017-06-22 01:30 +0200
Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Andy Lutomirski <luto@kernel.org> - 2017-06-22 04:30 +0200
Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() Ingo Molnar <mingo@kernel.org> - 2017-06-22 09:40 +0200
[PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 11:10 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Andy Lutomirski <luto@kernel.org> - 2017-06-21 18:30 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Borislav Petkov <bp@alien8.de> - 2017-06-21 19:30 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Borislav Petkov <bp@alien8.de> - 2017-06-22 17:00 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Andy Lutomirski <luto@kernel.org> - 2017-06-22 19:50 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Borislav Petkov <bp@alien8.de> - 2017-06-22 21:10 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-23 15:40 +0200
Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking Andy Lutomirski <luto@kernel.org> - 2017-06-23 17:30 +0200
[PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 10:10 +0200
Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Borislav Petkov <bp@alien8.de> - 2017-06-21 12:40 +0200
Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Andy Lutomirski <luto@kernel.org> - 2017-06-21 18:40 +0200
Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Borislav Petkov <bp@alien8.de> - 2017-06-21 19:10 +0200
Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Borislav Petkov <bp@alien8.de> - 2017-06-21 19:50 +0200
Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID Andy Lutomirski <luto@kernel.org> - 2017-06-22 04:40 +0200
[PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 11:40 +0200
Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 15:50 +0200
Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Andy Lutomirski <luto@kernel.org> - 2017-06-21 22:40 +0200
Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Borislav Petkov <bp@alien8.de> - 2017-06-23 14:00 +0200
Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Andy Lutomirski <luto@kernel.org> - 2017-06-23 17:30 +0200
Re: [PATCH v3 10/11] x86/mm: Enable CR4.PCIDE on supported systems Boris Ostrovsky <boris.ostrovsky@oracle.com> - 2017-06-23 15:40 +0200
[PATCH v3 07/11] x86/mm: Stop calling leave_mm() in idle code Andy Lutomirski <luto@kernel.org> - 2017-06-21 07:30 +0200
Re: [PATCH v3 07/11] x86/mm: Stop calling leave_mm() in idle code Thomas Gleixner <tglx@linutronix.de> - 2017-06-21 11:30 +0200
Re: [PATCH v3 07/11] x86/mm: Stop calling leave_mm() in idle code Andy Lutomirski <luto@kernel.org> - 2017-06-21 18:30 +0200
Re: [PATCH v3 07/11] x86/mm: Stop calling leave_mm() in idle code Borislav Petkov <bp@alien8.de> - 2017-06-23 11:10 +0200
Re: [PATCH v3 00/11] PCID and improved laziness Linus Torvalds <torvalds@linux-foundation.org> - 2017-06-21 20:30 +0200
Re: [PATCH v3 00/11] PCID and improved laziness Andy Lutomirski <luto@kernel.org> - 2017-06-22 07:30 +0200
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-21 07:30 +0200 |
| Subject | [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tUGng-1Tn-29@gated-at.bofh.it> |
| In reply to | #1671353 |
It was historically possible to have two concurrent TLB flushes
targetting the same CPU: one initiated locally and one initiated
remotely. This can now cause an OOPS in leave_mm() at
arch/x86/mm/tlb.c:47:
if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
BUG();
with this call trace:
flush_tlb_func_local arch/x86/mm/tlb.c:239 [inline]
flush_tlb_mm_range+0x26d/0x370 arch/x86/mm/tlb.c:317
Without reentrancy, this OOPS is impossible: leave_mm() is only
called if we're not in TLBSTATE_OK, but then we're unexpectedly
in TLBSTATE_OK in leave_mm().
This can be caused by flush_tlb_func_remote() happening between
the two checks and calling leave_mm(), resulting in two consecutive
leave_mm() calls on the same CPU with no intervening switch_mm()
calls.
We never saw this OOPS before because the old leave_mm()
implementation didn't put us back in TLBSTATE_OK, so the assertion
didn't fire.
Nadav noticed the reentrancy issue in a different context, but
neither of us realized that it caused a problem yet.
Cc: Nadav Amit <nadav.amit@gmail.com>
Cc: Dave Hansen <dave.hansen@intel.com>
Reported-by: "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com>
Fixes: 3d28ebceaffa ("x86/mm: Rework lazy TLB to track the actual loaded mm")
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/mm/tlb.c | 15 +++++++++++++--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 2a5e851f2035..f06239c6919f 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -208,6 +208,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
static void flush_tlb_func_common(const struct flush_tlb_info *f,
bool local, enum tlb_flush_reason reason)
{
+ /* This code cannot presently handle being reentered. */
+ VM_WARN_ON(!irqs_disabled());
+
if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
leave_mm(smp_processor_id());
return;
@@ -313,8 +316,12 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
info.end = TLB_FLUSH_ALL;
}
- if (mm == this_cpu_read(cpu_tlbstate.loaded_mm))
+ if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
+ local_irq_disable();
flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN);
+ local_irq_enable();
+ }
+
if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
flush_tlb_others(mm_cpumask(mm), &info);
put_cpu();
@@ -370,8 +377,12 @@ void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
int cpu = get_cpu();
- if (cpumask_test_cpu(cpu, &batch->cpumask))
+ if (cpumask_test_cpu(cpu, &batch->cpumask)) {
+ local_irq_disable();
flush_tlb_func_local(&info, TLB_LOCAL_SHOOTDOWN);
+ local_irq_enable();
+ }
+
if (cpumask_any_but(&batch->cpumask, cpu) < nr_cpu_ids)
flush_tlb_others(&batch->cpumask, &info);
cpumask_clear(&batch->cpumask);
--
2.9.4
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-06-21 10:10 +0200 |
| Subject | Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tUIS7-3AK-37@gated-at.bofh.it> |
| In reply to | #1671360 |
On Tue, 20 Jun 2017, Andy Lutomirski wrote:
> Nadav noticed the reentrancy issue in a different context, but
> neither of us realized that it caused a problem yet.
>
> Cc: Nadav Amit <nadav.amit@gmail.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Reported-by: "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com>
> Fixes: 3d28ebceaffa ("x86/mm: Rework lazy TLB to track the actual loaded mm")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-06-21 10:50 +0200 |
| Subject | Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tUJuO-3Nw-15@gated-at.bofh.it> |
| In reply to | #1671360 |
On Tue, Jun 20, 2017 at 10:22:07PM -0700, Andy Lutomirski wrote:
> It was historically possible to have two concurrent TLB flushes
> targetting the same CPU: one initiated locally and one initiated
> remotely. This can now cause an OOPS in leave_mm() at
> arch/x86/mm/tlb.c:47:
>
> if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
> BUG();
>
> with this call trace:
> flush_tlb_func_local arch/x86/mm/tlb.c:239 [inline]
> flush_tlb_mm_range+0x26d/0x370 arch/x86/mm/tlb.c:317
These line numbers would most likely mean nothing soon. I think you
should rather explain why the bug can happen so that future lookers at
that code can find the spot...
>
> Without reentrancy, this OOPS is impossible: leave_mm() is only
> called if we're not in TLBSTATE_OK, but then we're unexpectedly
> in TLBSTATE_OK in leave_mm().
>
> This can be caused by flush_tlb_func_remote() happening between
> the two checks and calling leave_mm(), resulting in two consecutive
> leave_mm() calls on the same CPU with no intervening switch_mm()
> calls.
...like this, for example. That should be more future-code-changes-proof.
> We never saw this OOPS before because the old leave_mm()
> implementation didn't put us back in TLBSTATE_OK, so the assertion
> didn't fire.
>
> Nadav noticed the reentrancy issue in a different context, but
> neither of us realized that it caused a problem yet.
>
> Cc: Nadav Amit <nadav.amit@gmail.com>
> Cc: Dave Hansen <dave.hansen@intel.com>
> Reported-by: "Levin, Alexander (Sasha Levin)" <alexander.levin@verizon.com>
> Fixes: 3d28ebceaffa ("x86/mm: Rework lazy TLB to track the actual loaded mm")
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> arch/x86/mm/tlb.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 2a5e851f2035..f06239c6919f 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -208,6 +208,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> static void flush_tlb_func_common(const struct flush_tlb_info *f,
> bool local, enum tlb_flush_reason reason)
> {
> + /* This code cannot presently handle being reentered. */
> + VM_WARN_ON(!irqs_disabled());
> +
> if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
> leave_mm(smp_processor_id());
> return;
> @@ -313,8 +316,12 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
> info.end = TLB_FLUSH_ALL;
> }
>
> - if (mm == this_cpu_read(cpu_tlbstate.loaded_mm))
> + if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
> + local_irq_disable();
> flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN);
> + local_irq_enable();
> + }
I'm assuming this is going away in a future patch, as disabling IRQs
around a TLB flush is kinda expensive. I guess I'll see if I continue
reading...
:)
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-21 17:20 +0200 |
| Subject | Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tUPAe-7N4-17@gated-at.bofh.it> |
| In reply to | #1671503 |
On Wed, Jun 21, 2017 at 1:49 AM, Borislav Petkov <bp@alien8.de> wrote: > On Tue, Jun 20, 2017 at 10:22:07PM -0700, Andy Lutomirski wrote: >> It was historically possible to have two concurrent TLB flushes >> targetting the same CPU: one initiated locally and one initiated >> remotely. This can now cause an OOPS in leave_mm() at >> arch/x86/mm/tlb.c:47: >> >> if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK) >> BUG(); >> >> with this call trace: >> flush_tlb_func_local arch/x86/mm/tlb.c:239 [inline] >> flush_tlb_mm_range+0x26d/0x370 arch/x86/mm/tlb.c:317 > > These line numbers would most likely mean nothing soon. I think you > should rather explain why the bug can happen so that future lookers at > that code can find the spot... > That's why I gave function names and the actual code :) > I'm assuming this is going away in a future patch, as disabling IRQs > around a TLB flush is kinda expensive. I guess I'll see if I continue > reading... No, it's still there. It's possible that it could be removed with lots of care, but I'm not convinced it's worth it. local_irq_disable() and local_irq_enable() are fast, though (3 cycles each last time I benchmarked them?) -- it's local_irq_save() that really hurts. --Andy
[toc] | [prev] | [next] | [standalone]
| From | Nadav Amit <nadav.amit@gmail.com> |
|---|---|
| Date | 2017-06-22 01:30 +0200 |
| Subject | Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tUXep-4FF-15@gated-at.bofh.it> |
| In reply to | #1671360 |
Andy Lutomirski <luto@kernel.org> wrote:
> index 2a5e851f2035..f06239c6919f 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -208,6 +208,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> static void flush_tlb_func_common(const struct flush_tlb_info *f,
> bool local, enum tlb_flush_reason reason)
> {
> + /* This code cannot presently handle being reentered. */
> + VM_WARN_ON(!irqs_disabled());
> +
> if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
> leave_mm(smp_processor_id());
> return;
> @@ -313,8 +316,12 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
> info.end = TLB_FLUSH_ALL;
> }
>
> - if (mm == this_cpu_read(cpu_tlbstate.loaded_mm))
> + if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
Perhaps you want to add:
VM_WARN_ON(irqs_disabled());
here
> + local_irq_disable();
> flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN);
> + local_irq_enable();
> + }
> +
> if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
> flush_tlb_others(mm_cpumask(mm), &info);
> put_cpu();
> @@ -370,8 +377,12 @@ void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
>
> int cpu = get_cpu();
>
> - if (cpumask_test_cpu(cpu, &batch->cpumask))
> + if (cpumask_test_cpu(cpu, &batch->cpumask)) {
and here?
> + local_irq_disable();
> flush_tlb_func_local(&info, TLB_LOCAL_SHOOTDOWN);
> + local_irq_enable();
> + }
> +
> if (cpumask_any_but(&batch->cpumask, cpu) < nr_cpu_ids)
> flush_tlb_others(&batch->cpumask, &info);
> cpumask_clear(&batch->cpumask);
> --
> 2.9.4
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-22 04:30 +0200 |
| Subject | Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tV02C-6I7-9@gated-at.bofh.it> |
| In reply to | #1672146 |
On Wed, Jun 21, 2017 at 4:26 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> Andy Lutomirski <luto@kernel.org> wrote:
>
>> index 2a5e851f2035..f06239c6919f 100644
>> --- a/arch/x86/mm/tlb.c
>> +++ b/arch/x86/mm/tlb.c
>> @@ -208,6 +208,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>> static void flush_tlb_func_common(const struct flush_tlb_info *f,
>> bool local, enum tlb_flush_reason reason)
>> {
>> + /* This code cannot presently handle being reentered. */
>> + VM_WARN_ON(!irqs_disabled());
>> +
>> if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
>> leave_mm(smp_processor_id());
>> return;
>> @@ -313,8 +316,12 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
>> info.end = TLB_FLUSH_ALL;
>> }
>>
>> - if (mm == this_cpu_read(cpu_tlbstate.loaded_mm))
>> + if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
>
> Perhaps you want to add:
>
> VM_WARN_ON(irqs_disabled());
>
> here
>
>> + local_irq_disable();
>> flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN);
>> + local_irq_enable();
>> + }
>> +
>> if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
>> flush_tlb_others(mm_cpumask(mm), &info);
>> put_cpu();
>> @@ -370,8 +377,12 @@ void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
>>
>> int cpu = get_cpu();
>>
>> - if (cpumask_test_cpu(cpu, &batch->cpumask))
>> + if (cpumask_test_cpu(cpu, &batch->cpumask)) {
>
> and here?
>
Will do.
What I really want is lockdep_assert_irqs_disabled() or, even better,
for this to be implicit when calling local_irq_disable(). Ingo?
--Andy
[toc] | [prev] | [next] | [standalone]
| From | Ingo Molnar <mingo@kernel.org> |
|---|---|
| Date | 2017-06-22 09:40 +0200 |
| Subject | Re: [PATCH v3 01/11] x86/mm: Don't reenter flush_tlb_func_common() |
| Message-ID | <tV4SC-1wt-19@gated-at.bofh.it> |
| In reply to | #1672260 |
* Andy Lutomirski <luto@kernel.org> wrote:
> On Wed, Jun 21, 2017 at 4:26 PM, Nadav Amit <nadav.amit@gmail.com> wrote:
> > Andy Lutomirski <luto@kernel.org> wrote:
> >
> >> index 2a5e851f2035..f06239c6919f 100644
> >> --- a/arch/x86/mm/tlb.c
> >> +++ b/arch/x86/mm/tlb.c
> >> @@ -208,6 +208,9 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> >> static void flush_tlb_func_common(const struct flush_tlb_info *f,
> >> bool local, enum tlb_flush_reason reason)
> >> {
> >> + /* This code cannot presently handle being reentered. */
> >> + VM_WARN_ON(!irqs_disabled());
> >> +
> >> if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
> >> leave_mm(smp_processor_id());
> >> return;
> >> @@ -313,8 +316,12 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
> >> info.end = TLB_FLUSH_ALL;
> >> }
> >>
> >> - if (mm == this_cpu_read(cpu_tlbstate.loaded_mm))
> >> + if (mm == this_cpu_read(cpu_tlbstate.loaded_mm)) {
> >
> > Perhaps you want to add:
> >
> > VM_WARN_ON(irqs_disabled());
> >
> > here
> >
> >> + local_irq_disable();
> >> flush_tlb_func_local(&info, TLB_LOCAL_MM_SHOOTDOWN);
> >> + local_irq_enable();
> >> + }
> >> +
> >> if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
> >> flush_tlb_others(mm_cpumask(mm), &info);
> >> put_cpu();
> >> @@ -370,8 +377,12 @@ void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
> >>
> >> int cpu = get_cpu();
> >>
> >> - if (cpumask_test_cpu(cpu, &batch->cpumask))
> >> + if (cpumask_test_cpu(cpu, &batch->cpumask)) {
> >
> > and here?
> >
>
> Will do.
>
> What I really want is lockdep_assert_irqs_disabled() or, even better,
> for this to be implicit when calling local_irq_disable(). Ingo?
I tried that once many years ago and IIRC there were problems - but maybe we could
try it again and enforce it, as I agree that the following pattern:
local_irq_disable();
...
local_irq_disable();
...
local_irq_enable();
...
local_irq_enable();
.. is actively dangerous.
Thanks,
Ingo
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-21 07:30 +0200 |
| Subject | [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tUGng-1Tn-35@gated-at.bofh.it> |
| In reply to | #1671353 |
x86's lazy TLB mode used to be fairly weak -- it would switch to
init_mm the first time it tried to flush a lazy TLB. This meant an
unnecessary CR3 write and, if the flush was remote, an unnecessary
IPI.
Rewrite it entirely. When we enter lazy mode, we simply remove the
cpu from mm_cpumask. This means that we need a way to figure out
whether we've missed a flush when we switch back out of lazy mode.
I use the tlb_gen machinery to track whether a context is up to
date.
Note to reviewers: this patch, my itself, looks a bit odd. I'm
using an array of length 1 containing (ctx_id, tlb_gen) rather than
just storing tlb_gen, and making it at array isn't necessary yet.
I'm doing this because the next few patches add PCID support, and,
with PCID, we need ctx_id, and the array will end up with a length
greater than 1. Making it an array now means that there will be
less churn and therefore less stress on your eyeballs.
NB: This is dubious but, AFAICT, still correct on Xen and UV.
xen_exit_mmap() uses mm_cpumask() for nefarious purposes and this
patch changes the way that mm_cpumask() works. This should be okay,
since Xen *also* iterates all online CPUs to find all the CPUs it
needs to twiddle.
The UV tlbflush code is rather dated and should be changed.
Cc: Andrew Banman <abanman@sgi.com>
Cc: Mike Travis <travis@sgi.com>
Cc: Dimitri Sivanich <sivanich@sgi.com>
Cc: Juergen Gross <jgross@suse.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/mmu_context.h | 6 +-
arch/x86/include/asm/tlbflush.h | 4 -
arch/x86/mm/init.c | 1 -
arch/x86/mm/tlb.c | 227 +++++++++++++++++++------------------
arch/x86/xen/mmu_pv.c | 3 +-
5 files changed, 119 insertions(+), 122 deletions(-)
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index e5295d485899..69a4f1ee86ac 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -125,8 +125,10 @@ static inline void switch_ldt(struct mm_struct *prev, struct mm_struct *next)
static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
{
- if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
- this_cpu_write(cpu_tlbstate.state, TLBSTATE_LAZY);
+ int cpu = smp_processor_id();
+
+ if (cpumask_test_cpu(cpu, mm_cpumask(mm)))
+ cpumask_clear_cpu(cpu, mm_cpumask(mm));
}
extern atomic64_t last_mm_ctx_id;
diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 4f6c30d6ec39..87b13e51e867 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -95,7 +95,6 @@ struct tlb_state {
* mode even if we've already switched back to swapper_pg_dir.
*/
struct mm_struct *loaded_mm;
- int state;
/*
* Access to this CR4 shadow and to H/W CR4 is protected by
@@ -310,9 +309,6 @@ static inline void flush_tlb_page(struct vm_area_struct *vma, unsigned long a)
void native_flush_tlb_others(const struct cpumask *cpumask,
const struct flush_tlb_info *info);
-#define TLBSTATE_OK 1
-#define TLBSTATE_LAZY 2
-
static inline void arch_tlbbatch_add_mm(struct arch_tlbflush_unmap_batch *batch,
struct mm_struct *mm)
{
diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
index 88ee942cb47d..7d6fa4676af9 100644
--- a/arch/x86/mm/init.c
+++ b/arch/x86/mm/init.c
@@ -812,7 +812,6 @@ void __init zone_sizes_init(void)
DEFINE_PER_CPU_SHARED_ALIGNED(struct tlb_state, cpu_tlbstate) = {
.loaded_mm = &init_mm,
- .state = 0,
.cr4 = ~0UL, /* fail hard if we screw up cr4 shadow initialization */
};
EXPORT_SYMBOL_GPL(cpu_tlbstate);
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 9f5ef7a5e74a..fea2b07ac7d8 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -45,8 +45,8 @@ void leave_mm(int cpu)
if (loaded_mm == &init_mm)
return;
- if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
- BUG();
+ /* Warn if we're not lazy. */
+ WARN_ON(cpumask_test_cpu(smp_processor_id(), mm_cpumask(loaded_mm)));
switch_mm(NULL, &init_mm, NULL);
}
@@ -67,133 +67,118 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
{
unsigned cpu = smp_processor_id();
struct mm_struct *real_prev = this_cpu_read(cpu_tlbstate.loaded_mm);
+ u64 next_tlb_gen;
/*
- * NB: The scheduler will call us with prev == next when
- * switching from lazy TLB mode to normal mode if active_mm
- * isn't changing. When this happens, there is no guarantee
- * that CR3 (and hence cpu_tlbstate.loaded_mm) matches next.
+ * NB: The scheduler will call us with prev == next when switching
+ * from lazy TLB mode to normal mode if active_mm isn't changing.
+ * When this happens, we don't assume that CR3 (and hence
+ * cpu_tlbstate.loaded_mm) matches next.
*
* NB: leave_mm() calls us with prev == NULL and tsk == NULL.
*/
- this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK);
+ /* We don't want flush_tlb_func_* to run concurrently with us. */
+ if (IS_ENABLED(CONFIG_PROVE_LOCKING))
+ WARN_ON_ONCE(!irqs_disabled());
+
+ VM_BUG_ON(read_cr3_pa() != __pa(real_prev->pgd));
if (real_prev == next) {
- /*
- * There's nothing to do: we always keep the per-mm control
- * regs in sync with cpu_tlbstate.loaded_mm. Just
- * sanity-check mm_cpumask.
- */
- if (WARN_ON_ONCE(!cpumask_test_cpu(cpu, mm_cpumask(next))))
- cpumask_set_cpu(cpu, mm_cpumask(next));
- return;
- }
+ if (cpumask_test_cpu(cpu, mm_cpumask(next))) {
+ /*
+ * There's nothing to do: we weren't lazy, and we
+ * aren't changing our mm. We don't need to flush
+ * anything, nor do we need to update CR3, CR4, or
+ * LDTR.
+ */
+ return;
+ }
+
+ /* Resume remote flushes and then read tlb_gen. */
+ cpumask_set_cpu(cpu, mm_cpumask(next));
+ next_tlb_gen = atomic64_read(&next->context.tlb_gen);
+
+ VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
+ next->context.ctx_id);
+
+ if (this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen) <
+ next_tlb_gen) {
+ /*
+ * Ideally, we'd have a flush_tlb() variant that
+ * takes the known CR3 value as input. This would
+ * be faster on Xen PV and on hypothetical CPUs
+ * on which INVPCID is fast.
+ */
+ this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
+ next_tlb_gen);
+ write_cr3(__pa(next->pgd));
+ /*
+ * This gets called via leave_mm() in the idle path
+ * where RCU functions differently. Tracing normally
+ * uses RCU, so we have to call the tracepoint
+ * specially here.
+ */
+ trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
+ TLB_FLUSH_ALL);
+ }
- if (IS_ENABLED(CONFIG_VMAP_STACK)) {
/*
- * If our current stack is in vmalloc space and isn't
- * mapped in the new pgd, we'll double-fault. Forcibly
- * map it.
+ * We just exited lazy mode, which means that CR4 and/or LDTR
+ * may be stale. (Changes to the required CR4 and LDTR states
+ * are not reflected in tlb_gen.)
*/
- unsigned int stack_pgd_index = pgd_index(current_stack_pointer());
+ } else {
+ if (IS_ENABLED(CONFIG_VMAP_STACK)) {
+ /*
+ * If our current stack is in vmalloc space and isn't
+ * mapped in the new pgd, we'll double-fault. Forcibly
+ * map it.
+ */
+ unsigned int stack_pgd_index =
+ pgd_index(current_stack_pointer());
+
+ pgd_t *pgd = next->pgd + stack_pgd_index;
+
+ if (unlikely(pgd_none(*pgd)))
+ set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
+ }
- pgd_t *pgd = next->pgd + stack_pgd_index;
+ /* Stop remote flushes for the previous mm */
+ if (cpumask_test_cpu(cpu, mm_cpumask(real_prev)))
+ cpumask_clear_cpu(cpu, mm_cpumask(real_prev));
- if (unlikely(pgd_none(*pgd)))
- set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
- }
+ WARN_ON_ONCE(cpumask_test_cpu(cpu, mm_cpumask(next)));
- this_cpu_write(cpu_tlbstate.loaded_mm, next);
- this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id, next->context.ctx_id);
- this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
- atomic64_read(&next->context.tlb_gen));
+ /*
+ * Start remote flushes and then read tlb_gen.
+ */
+ cpumask_set_cpu(cpu, mm_cpumask(next));
+ next_tlb_gen = atomic64_read(&next->context.tlb_gen);
- WARN_ON_ONCE(cpumask_test_cpu(cpu, mm_cpumask(next)));
- cpumask_set_cpu(cpu, mm_cpumask(next));
+ VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) ==
+ next->context.ctx_id);
- /*
- * Re-load page tables.
- *
- * This logic has an ordering constraint:
- *
- * CPU 0: Write to a PTE for 'next'
- * CPU 0: load bit 1 in mm_cpumask. if nonzero, send IPI.
- * CPU 1: set bit 1 in next's mm_cpumask
- * CPU 1: load from the PTE that CPU 0 writes (implicit)
- *
- * We need to prevent an outcome in which CPU 1 observes
- * the new PTE value and CPU 0 observes bit 1 clear in
- * mm_cpumask. (If that occurs, then the IPI will never
- * be sent, and CPU 0's TLB will contain a stale entry.)
- *
- * The bad outcome can occur if either CPU's load is
- * reordered before that CPU's store, so both CPUs must
- * execute full barriers to prevent this from happening.
- *
- * Thus, switch_mm needs a full barrier between the
- * store to mm_cpumask and any operation that could load
- * from next->pgd. TLB fills are special and can happen
- * due to instruction fetches or for no reason at all,
- * and neither LOCK nor MFENCE orders them.
- * Fortunately, load_cr3() is serializing and gives the
- * ordering guarantee we need.
- */
- load_cr3(next->pgd);
-
- /*
- * This gets called via leave_mm() in the idle path where RCU
- * functions differently. Tracing normally uses RCU, so we have to
- * call the tracepoint specially here.
- */
- trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
+ this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id,
+ next->context.ctx_id);
+ this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
+ next_tlb_gen);
+ this_cpu_write(cpu_tlbstate.loaded_mm, next);
+ write_cr3(__pa(next->pgd));
- /* Stop flush ipis for the previous mm */
- WARN_ON_ONCE(!cpumask_test_cpu(cpu, mm_cpumask(real_prev)) &&
- real_prev != &init_mm);
- cpumask_clear_cpu(cpu, mm_cpumask(real_prev));
+ /*
+ * This gets called via leave_mm() in the idle path where RCU
+ * functions differently. Tracing normally uses RCU, so we
+ * have to call the tracepoint specially here.
+ */
+ trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
+ TLB_FLUSH_ALL);
+ }
- /* Load per-mm CR4 and LDTR state */
load_mm_cr4(next);
switch_ldt(real_prev, next);
}
-/*
- * The flush IPI assumes that a thread switch happens in this order:
- * [cpu0: the cpu that switches]
- * 1) switch_mm() either 1a) or 1b)
- * 1a) thread switch to a different mm
- * 1a1) set cpu_tlbstate to TLBSTATE_OK
- * Now the tlb flush NMI handler flush_tlb_func won't call leave_mm
- * if cpu0 was in lazy tlb mode.
- * 1a2) update cpu active_mm
- * Now cpu0 accepts tlb flushes for the new mm.
- * 1a3) cpu_set(cpu, new_mm->cpu_vm_mask);
- * Now the other cpus will send tlb flush ipis.
- * 1a4) change cr3.
- * 1a5) cpu_clear(cpu, old_mm->cpu_vm_mask);
- * Stop ipi delivery for the old mm. This is not synchronized with
- * the other cpus, but flush_tlb_func ignore flush ipis for the wrong
- * mm, and in the worst case we perform a superfluous tlb flush.
- * 1b) thread switch without mm change
- * cpu active_mm is correct, cpu0 already handles flush ipis.
- * 1b1) set cpu_tlbstate to TLBSTATE_OK
- * 1b2) test_and_set the cpu bit in cpu_vm_mask.
- * Atomically set the bit [other cpus will start sending flush ipis],
- * and test the bit.
- * 1b3) if the bit was 0: leave_mm was called, flush the tlb.
- * 2) switch %%esp, ie current
- *
- * The interrupt must handle 2 special cases:
- * - cr3 is changed before %%esp, ie. it cannot use current->{active_,}mm.
- * - the cpu performs speculative tlb reads, i.e. even if the cpu only
- * runs in kernel space, the cpu could load tlb entries for user space
- * pages.
- *
- * The good news is that cpu_tlbstate is local to each cpu, no
- * write/read ordering problems.
- */
-
static void flush_tlb_func_common(const struct flush_tlb_info *f,
bool local, enum tlb_flush_reason reason)
{
@@ -215,12 +200,13 @@ static void flush_tlb_func_common(const struct flush_tlb_info *f,
VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
loaded_mm->context.ctx_id);
- if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
+ if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(loaded_mm))) {
/*
- * leave_mm() is adequate to handle any type of flush, and
- * we would prefer not to receive further IPIs.
+ * We're in lazy mode -- don't flush. We can get here on
+ * remote flushes due to races and on local flushes if a
+ * kernel thread coincidentally flushes the mm it's lazily
+ * still using.
*/
- leave_mm(smp_processor_id());
return;
}
@@ -317,6 +303,21 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
(info->end - info->start) >> PAGE_SHIFT);
if (is_uv_system()) {
+ /*
+ * This whole special case is confused. UV has a "Broadcast
+ * Assist Unit", which seems to be a fancy way to send IPIs.
+ * Back when x86 used an explicit TLB flush IPI, UV was
+ * optimized to use its own mechanism. These days, x86 uses
+ * smp_call_function_many(), but UV still uses a manual IPI,
+ * and that IPI's action is out of date -- it does a manual
+ * flush instead of calling flush_tlb_func_remote(). This
+ * means that the percpu tlb_gen variables won't be updated
+ * and we'll do pointless flushes on future context switches.
+ *
+ * Rather than hooking native_flush_tlb_others() here, I think
+ * that UV should be updated so that smp_call_function_many(),
+ * etc, are optimal on UV.
+ */
unsigned int cpu;
cpu = smp_processor_id();
@@ -375,6 +376,7 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
if (cpumask_any_but(mm_cpumask(mm), cpu) < nr_cpu_ids)
flush_tlb_others(mm_cpumask(mm), &info);
+
put_cpu();
}
@@ -383,8 +385,6 @@ static void do_flush_tlb_all(void *info)
{
count_vm_tlb_event(NR_TLB_REMOTE_FLUSH_RECEIVED);
__flush_tlb_all();
- if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_LAZY)
- leave_mm(smp_processor_id());
}
void flush_tlb_all(void)
@@ -436,6 +436,7 @@ void arch_tlbbatch_flush(struct arch_tlbflush_unmap_batch *batch)
if (cpumask_any_but(&batch->cpumask, cpu) < nr_cpu_ids)
flush_tlb_others(&batch->cpumask, &info);
+
cpumask_clear(&batch->cpumask);
put_cpu();
diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 1d7a7213a310..f5df56fb8b5c 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1005,8 +1005,7 @@ static void xen_drop_mm_ref(struct mm_struct *mm)
/* Get the "official" set of cpus referring to our pagetable. */
if (!alloc_cpumask_var(&mask, GFP_ATOMIC)) {
for_each_online_cpu(cpu) {
- if (!cpumask_test_cpu(cpu, mm_cpumask(mm))
- && per_cpu(xen_current_cr3, cpu) != __pa(mm->pgd))
+ if (per_cpu(xen_current_cr3, cpu) != __pa(mm->pgd))
continue;
smp_call_function_single(cpu, drop_mm_ref_this_cpu, mm, 1);
}
--
2.9.4
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-06-21 11:10 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tUJOa-4bl-13@gated-at.bofh.it> |
| In reply to | #1671363 |
On Tue, 20 Jun 2017, Andy Lutomirski wrote:
> -/*
> - * The flush IPI assumes that a thread switch happens in this order:
> - * [cpu0: the cpu that switches]
> - * 1) switch_mm() either 1a) or 1b)
> - * 1a) thread switch to a different mm
> - * 1a1) set cpu_tlbstate to TLBSTATE_OK
> - * Now the tlb flush NMI handler flush_tlb_func won't call leave_mm
> - * if cpu0 was in lazy tlb mode.
> - * 1a2) update cpu active_mm
> - * Now cpu0 accepts tlb flushes for the new mm.
> - * 1a3) cpu_set(cpu, new_mm->cpu_vm_mask);
> - * Now the other cpus will send tlb flush ipis.
> - * 1a4) change cr3.
> - * 1a5) cpu_clear(cpu, old_mm->cpu_vm_mask);
> - * Stop ipi delivery for the old mm. This is not synchronized with
> - * the other cpus, but flush_tlb_func ignore flush ipis for the wrong
> - * mm, and in the worst case we perform a superfluous tlb flush.
> - * 1b) thread switch without mm change
> - * cpu active_mm is correct, cpu0 already handles flush ipis.
> - * 1b1) set cpu_tlbstate to TLBSTATE_OK
> - * 1b2) test_and_set the cpu bit in cpu_vm_mask.
> - * Atomically set the bit [other cpus will start sending flush ipis],
> - * and test the bit.
> - * 1b3) if the bit was 0: leave_mm was called, flush the tlb.
> - * 2) switch %%esp, ie current
> - *
> - * The interrupt must handle 2 special cases:
> - * - cr3 is changed before %%esp, ie. it cannot use current->{active_,}mm.
> - * - the cpu performs speculative tlb reads, i.e. even if the cpu only
> - * runs in kernel space, the cpu could load tlb entries for user space
> - * pages.
> - *
> - * The good news is that cpu_tlbstate is local to each cpu, no
> - * write/read ordering problems.
While the new code is really well commented, it would be a good thing to
have a single place where all of this including the ordering constraints
are documented.
> @@ -215,12 +200,13 @@ static void flush_tlb_func_common(const struct flush_tlb_info *f,
> VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
> loaded_mm->context.ctx_id);
>
> - if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
> + if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(loaded_mm))) {
> /*
> - * leave_mm() is adequate to handle any type of flush, and
> - * we would prefer not to receive further IPIs.
> + * We're in lazy mode -- don't flush. We can get here on
> + * remote flushes due to races and on local flushes if a
> + * kernel thread coincidentally flushes the mm it's lazily
> + * still using.
Ok. That's more informative.
Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-21 18:30 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tUQFY-8uH-11@gated-at.bofh.it> |
| In reply to | #1671521 |
On Wed, Jun 21, 2017 at 2:01 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Tue, 20 Jun 2017, Andy Lutomirski wrote:
>> -/*
>> - * The flush IPI assumes that a thread switch happens in this order:
>> - * [cpu0: the cpu that switches]
>> - * 1) switch_mm() either 1a) or 1b)
>> - * 1a) thread switch to a different mm
>> - * 1a1) set cpu_tlbstate to TLBSTATE_OK
>> - * Now the tlb flush NMI handler flush_tlb_func won't call leave_mm
>> - * if cpu0 was in lazy tlb mode.
>> - * 1a2) update cpu active_mm
>> - * Now cpu0 accepts tlb flushes for the new mm.
>> - * 1a3) cpu_set(cpu, new_mm->cpu_vm_mask);
>> - * Now the other cpus will send tlb flush ipis.
>> - * 1a4) change cr3.
>> - * 1a5) cpu_clear(cpu, old_mm->cpu_vm_mask);
>> - * Stop ipi delivery for the old mm. This is not synchronized with
>> - * the other cpus, but flush_tlb_func ignore flush ipis for the wrong
>> - * mm, and in the worst case we perform a superfluous tlb flush.
>> - * 1b) thread switch without mm change
>> - * cpu active_mm is correct, cpu0 already handles flush ipis.
>> - * 1b1) set cpu_tlbstate to TLBSTATE_OK
>> - * 1b2) test_and_set the cpu bit in cpu_vm_mask.
>> - * Atomically set the bit [other cpus will start sending flush ipis],
>> - * and test the bit.
>> - * 1b3) if the bit was 0: leave_mm was called, flush the tlb.
>> - * 2) switch %%esp, ie current
>> - *
>> - * The interrupt must handle 2 special cases:
>> - * - cr3 is changed before %%esp, ie. it cannot use current->{active_,}mm.
>> - * - the cpu performs speculative tlb reads, i.e. even if the cpu only
>> - * runs in kernel space, the cpu could load tlb entries for user space
>> - * pages.
>> - *
>> - * The good news is that cpu_tlbstate is local to each cpu, no
>> - * write/read ordering problems.
>
> While the new code is really well commented, it would be a good thing to
> have a single place where all of this including the ordering constraints
> are documented.
I'll look at the end of the whole series and see if I can come up with
something good.
>
>> @@ -215,12 +200,13 @@ static void flush_tlb_func_common(const struct flush_tlb_info *f,
>> VM_WARN_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
>> loaded_mm->context.ctx_id);
>>
>> - if (this_cpu_read(cpu_tlbstate.state) != TLBSTATE_OK) {
>> + if (!cpumask_test_cpu(smp_processor_id(), mm_cpumask(loaded_mm))) {
>> /*
>> - * leave_mm() is adequate to handle any type of flush, and
>> - * we would prefer not to receive further IPIs.
>> + * We're in lazy mode -- don't flush. We can get here on
>> + * remote flushes due to races and on local flushes if a
>> + * kernel thread coincidentally flushes the mm it's lazily
>> + * still using.
>
> Ok. That's more informative.
>
> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-06-21 19:30 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tURC2-HR-17@gated-at.bofh.it> |
| In reply to | #1671759 |
On Wed, Jun 21, 2017 at 09:04:48AM -0700, Andy Lutomirski wrote:
> I'll look at the end of the whole series and see if I can come up with
> something good.
... along with the logic what we flush when, please. I.e., the text in
struct flush_tlb_info.
Thanks.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-06-22 17:00 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tVbKq-67R-21@gated-at.bofh.it> |
| In reply to | #1671363 |
On Tue, Jun 20, 2017 at 10:22:12PM -0700, Andy Lutomirski wrote:
> x86's lazy TLB mode used to be fairly weak -- it would switch to
> init_mm the first time it tried to flush a lazy TLB. This meant an
> unnecessary CR3 write and, if the flush was remote, an unnecessary
> IPI.
>
> Rewrite it entirely. When we enter lazy mode, we simply remove the
> cpu from mm_cpumask. This means that we need a way to figure out
s/cpu/CPU/
> whether we've missed a flush when we switch back out of lazy mode.
> I use the tlb_gen machinery to track whether a context is up to
> date.
>
> Note to reviewers: this patch, my itself, looks a bit odd. I'm
> using an array of length 1 containing (ctx_id, tlb_gen) rather than
> just storing tlb_gen, and making it at array isn't necessary yet.
> I'm doing this because the next few patches add PCID support, and,
> with PCID, we need ctx_id, and the array will end up with a length
> greater than 1. Making it an array now means that there will be
> less churn and therefore less stress on your eyeballs.
>
> NB: This is dubious but, AFAICT, still correct on Xen and UV.
> xen_exit_mmap() uses mm_cpumask() for nefarious purposes and this
> patch changes the way that mm_cpumask() works. This should be okay,
> since Xen *also* iterates all online CPUs to find all the CPUs it
> needs to twiddle.
This whole text should be under the "---" line below if we don't want it
in the commit message.
>
> The UV tlbflush code is rather dated and should be changed.
>
> Cc: Andrew Banman <abanman@sgi.com>
> Cc: Mike Travis <travis@sgi.com>
> Cc: Dimitri Sivanich <sivanich@sgi.com>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> arch/x86/include/asm/mmu_context.h | 6 +-
> arch/x86/include/asm/tlbflush.h | 4 -
> arch/x86/mm/init.c | 1 -
> arch/x86/mm/tlb.c | 227 +++++++++++++++++++------------------
> arch/x86/xen/mmu_pv.c | 3 +-
> 5 files changed, 119 insertions(+), 122 deletions(-)
>
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index e5295d485899..69a4f1ee86ac 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -125,8 +125,10 @@ static inline void switch_ldt(struct mm_struct *prev, struct mm_struct *next)
>
> static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
> {
> - if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
> - this_cpu_write(cpu_tlbstate.state, TLBSTATE_LAZY);
> + int cpu = smp_processor_id();
> +
> + if (cpumask_test_cpu(cpu, mm_cpumask(mm)))
> + cpumask_clear_cpu(cpu, mm_cpumask(mm));
It seems we haz a helper for that: cpumask_test_and_clear_cpu() which
does BTR straightaway.
> extern atomic64_t last_mm_ctx_id;
> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
> index 4f6c30d6ec39..87b13e51e867 100644
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -95,7 +95,6 @@ struct tlb_state {
> * mode even if we've already switched back to swapper_pg_dir.
> */
> struct mm_struct *loaded_mm;
> - int state;
>
> /*
> * Access to this CR4 shadow and to H/W CR4 is protected by
> @@ -310,9 +309,6 @@ static inline void flush_tlb_page(struct vm_area_struct *vma, unsigned long a)
> void native_flush_tlb_others(const struct cpumask *cpumask,
> const struct flush_tlb_info *info);
>
> -#define TLBSTATE_OK 1
> -#define TLBSTATE_LAZY 2
> -
> static inline void arch_tlbbatch_add_mm(struct arch_tlbflush_unmap_batch *batch,
> struct mm_struct *mm)
> {
> diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c
> index 88ee942cb47d..7d6fa4676af9 100644
> --- a/arch/x86/mm/init.c
> +++ b/arch/x86/mm/init.c
> @@ -812,7 +812,6 @@ void __init zone_sizes_init(void)
>
> DEFINE_PER_CPU_SHARED_ALIGNED(struct tlb_state, cpu_tlbstate) = {
> .loaded_mm = &init_mm,
> - .state = 0,
> .cr4 = ~0UL, /* fail hard if we screw up cr4 shadow initialization */
> };
> EXPORT_SYMBOL_GPL(cpu_tlbstate);
> diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
> index 9f5ef7a5e74a..fea2b07ac7d8 100644
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -45,8 +45,8 @@ void leave_mm(int cpu)
> if (loaded_mm == &init_mm)
> return;
>
> - if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
> - BUG();
> + /* Warn if we're not lazy. */
> + WARN_ON(cpumask_test_cpu(smp_processor_id(), mm_cpumask(loaded_mm)));
We don't BUG() anymore?
>
> switch_mm(NULL, &init_mm, NULL);
> }
> @@ -67,133 +67,118 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
> {
> unsigned cpu = smp_processor_id();
> struct mm_struct *real_prev = this_cpu_read(cpu_tlbstate.loaded_mm);
> + u64 next_tlb_gen;
Please sort function local variables declaration in a reverse christmas
tree order:
<type> longest_variable_name;
<type> shorter_var_name;
<type> even_shorter;
<type> i;
>
> /*
> - * NB: The scheduler will call us with prev == next when
> - * switching from lazy TLB mode to normal mode if active_mm
> - * isn't changing. When this happens, there is no guarantee
> - * that CR3 (and hence cpu_tlbstate.loaded_mm) matches next.
> + * NB: The scheduler will call us with prev == next when switching
> + * from lazy TLB mode to normal mode if active_mm isn't changing.
> + * When this happens, we don't assume that CR3 (and hence
> + * cpu_tlbstate.loaded_mm) matches next.
> *
> * NB: leave_mm() calls us with prev == NULL and tsk == NULL.
> */
>
> - this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK);
> + /* We don't want flush_tlb_func_* to run concurrently with us. */
> + if (IS_ENABLED(CONFIG_PROVE_LOCKING))
> + WARN_ON_ONCE(!irqs_disabled());
> +
> + VM_BUG_ON(read_cr3_pa() != __pa(real_prev->pgd));
Why do we need that check? Can that ever happen?
> if (real_prev == next) {
> - /*
> - * There's nothing to do: we always keep the per-mm control
> - * regs in sync with cpu_tlbstate.loaded_mm. Just
> - * sanity-check mm_cpumask.
> - */
> - if (WARN_ON_ONCE(!cpumask_test_cpu(cpu, mm_cpumask(next))))
> - cpumask_set_cpu(cpu, mm_cpumask(next));
> - return;
> - }
> + if (cpumask_test_cpu(cpu, mm_cpumask(next))) {
> + /*
> + * There's nothing to do: we weren't lazy, and we
> + * aren't changing our mm. We don't need to flush
> + * anything, nor do we need to update CR3, CR4, or
> + * LDTR.
> + */
Nice comment.
> + return;
> + }
> +
> + /* Resume remote flushes and then read tlb_gen. */
> + cpumask_set_cpu(cpu, mm_cpumask(next));
> + next_tlb_gen = atomic64_read(&next->context.tlb_gen);
> +
> + VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
> + next->context.ctx_id);
I guess this check should be right under the if (real_prev == next), right?
> +
> + if (this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen) <
> + next_tlb_gen) {
Yeah, let it stick out - that trailing '<' doesn't make it any nicer.
> + /*
> + * Ideally, we'd have a flush_tlb() variant that
> + * takes the known CR3 value as input. This would
> + * be faster on Xen PV and on hypothetical CPUs
> + * on which INVPCID is fast.
> + */
> + this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
> + next_tlb_gen);
> + write_cr3(__pa(next->pgd));
<---- newline here.
> + /*
> + * This gets called via leave_mm() in the idle path
> + * where RCU functions differently. Tracing normally
> + * uses RCU, so we have to call the tracepoint
> + * specially here.
> + */
> + trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
> + TLB_FLUSH_ALL);
> + }
>
> - if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> /*
> - * If our current stack is in vmalloc space and isn't
> - * mapped in the new pgd, we'll double-fault. Forcibly
> - * map it.
> + * We just exited lazy mode, which means that CR4 and/or LDTR
> + * may be stale. (Changes to the required CR4 and LDTR states
> + * are not reflected in tlb_gen.)
We need that comment because... ? I mean, we do update CR4/LDTR at the
end of the function.
> */
> - unsigned int stack_pgd_index = pgd_index(current_stack_pointer());
> + } else {
> + if (IS_ENABLED(CONFIG_VMAP_STACK)) {
> + /*
> + * If our current stack is in vmalloc space and isn't
> + * mapped in the new pgd, we'll double-fault. Forcibly
> + * map it.
> + */
> + unsigned int stack_pgd_index =
Shorten that var name and make it fit into 80ish cols so that the
linebreak is gone.
> + pgd_index(current_stack_pointer());
> +
> + pgd_t *pgd = next->pgd + stack_pgd_index;
> +
> + if (unlikely(pgd_none(*pgd)))
> + set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
> + }
>
> - pgd_t *pgd = next->pgd + stack_pgd_index;
> + /* Stop remote flushes for the previous mm */
> + if (cpumask_test_cpu(cpu, mm_cpumask(real_prev)))
> + cpumask_clear_cpu(cpu, mm_cpumask(real_prev));
cpumask_test_and_clear_cpu()
>
> - if (unlikely(pgd_none(*pgd)))
> - set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
> - }
> + WARN_ON_ONCE(cpumask_test_cpu(cpu, mm_cpumask(next)));
We warn if the next task is not lazy because...?
> - this_cpu_write(cpu_tlbstate.loaded_mm, next);
> - this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id, next->context.ctx_id);
> - this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
> - atomic64_read(&next->context.tlb_gen));
> + /*
> + * Start remote flushes and then read tlb_gen.
> + */
> + cpumask_set_cpu(cpu, mm_cpumask(next));
> + next_tlb_gen = atomic64_read(&next->context.tlb_gen);
>
> - WARN_ON_ONCE(cpumask_test_cpu(cpu, mm_cpumask(next)));
> - cpumask_set_cpu(cpu, mm_cpumask(next));
> + VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) ==
> + next->context.ctx_id);
Also put it as the first statement after the else { ?
> - /*
> - * Re-load page tables.
> - *
> - * This logic has an ordering constraint:
> - *
> - * CPU 0: Write to a PTE for 'next'
> - * CPU 0: load bit 1 in mm_cpumask. if nonzero, send IPI.
> - * CPU 1: set bit 1 in next's mm_cpumask
> - * CPU 1: load from the PTE that CPU 0 writes (implicit)
> - *
> - * We need to prevent an outcome in which CPU 1 observes
> - * the new PTE value and CPU 0 observes bit 1 clear in
> - * mm_cpumask. (If that occurs, then the IPI will never
> - * be sent, and CPU 0's TLB will contain a stale entry.)
> - *
> - * The bad outcome can occur if either CPU's load is
> - * reordered before that CPU's store, so both CPUs must
> - * execute full barriers to prevent this from happening.
> - *
> - * Thus, switch_mm needs a full barrier between the
> - * store to mm_cpumask and any operation that could load
> - * from next->pgd. TLB fills are special and can happen
> - * due to instruction fetches or for no reason at all,
> - * and neither LOCK nor MFENCE orders them.
> - * Fortunately, load_cr3() is serializing and gives the
> - * ordering guarantee we need.
> - */
> - load_cr3(next->pgd);
> -
> - /*
> - * This gets called via leave_mm() in the idle path where RCU
> - * functions differently. Tracing normally uses RCU, so we have to
> - * call the tracepoint specially here.
> - */
> - trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
> + this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id,
> + next->context.ctx_id);
> + this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
> + next_tlb_gen);
Yeah, just let all those three stick out.
Also, no:
if (this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen) <
next_tlb_gen) {
check?
> + this_cpu_write(cpu_tlbstate.loaded_mm, next);
> + write_cr3(__pa(next->pgd));
>
> - /* Stop flush ipis for the previous mm */
> - WARN_ON_ONCE(!cpumask_test_cpu(cpu, mm_cpumask(real_prev)) &&
> - real_prev != &init_mm);
> - cpumask_clear_cpu(cpu, mm_cpumask(real_prev));
> + /*
> + * This gets called via leave_mm() in the idle path where RCU
> + * functions differently. Tracing normally uses RCU, so we
> + * have to call the tracepoint specially here.
> + */
> + trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
> + TLB_FLUSH_ALL);
> + }
That's repeated as in the if-branch above. Move it out I guess.
>
> - /* Load per-mm CR4 and LDTR state */
> load_mm_cr4(next);
> switch_ldt(real_prev, next);
> }
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-22 19:50 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tVeoW-7TL-9@gated-at.bofh.it> |
| In reply to | #1672713 |
On Thu, Jun 22, 2017 at 7:50 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Jun 20, 2017 at 10:22:12PM -0700, Andy Lutomirski wrote:
>> Rewrite it entirely. When we enter lazy mode, we simply remove the
>> cpu from mm_cpumask. This means that we need a way to figure out
>
> s/cpu/CPU/
Done.
>
>> whether we've missed a flush when we switch back out of lazy mode.
>> I use the tlb_gen machinery to track whether a context is up to
>> date.
>>
>> Note to reviewers: this patch, my itself, looks a bit odd. I'm
>> using an array of length 1 containing (ctx_id, tlb_gen) rather than
>> just storing tlb_gen, and making it at array isn't necessary yet.
>> I'm doing this because the next few patches add PCID support, and,
>> with PCID, we need ctx_id, and the array will end up with a length
>> greater than 1. Making it an array now means that there will be
>> less churn and therefore less stress on your eyeballs.
>>
>> NB: This is dubious but, AFAICT, still correct on Xen and UV.
>> xen_exit_mmap() uses mm_cpumask() for nefarious purposes and this
>> patch changes the way that mm_cpumask() works. This should be okay,
>> since Xen *also* iterates all online CPUs to find all the CPUs it
>> needs to twiddle.
>
> This whole text should be under the "---" line below if we don't want it
> in the commit message.
I figured that some future reader of this patch might actually want to
see this text, though.
>
>>
>> The UV tlbflush code is rather dated and should be changed.
And I'd definitely like the UV maintainers to notice this part, now or
in the future :) I don't want to personally touch the UV code with a
ten-foot pole, but it really should be updated by someone who has a
chance of getting it right and being able to test it.
>> +
>> + if (cpumask_test_cpu(cpu, mm_cpumask(mm)))
>> + cpumask_clear_cpu(cpu, mm_cpumask(mm));
>
> It seems we haz a helper for that: cpumask_test_and_clear_cpu() which
> does BTR straightaway.
Yeah, but I'm doing this for performance. I think that all the
various one-line helpers do a LOCKed op right away, and I think it's
faster to see if we can avoid the LOCKed op by trying an ordinary read
first. OTOH, maybe this is misguided -- if the cacheline lives
somewhere else and we do end up needing to update it, we'll end up
first sharing it and then making it exclusive, which increases the
amount of cache coherency traffic, so maybe I'm optimizing for the
wrong thing. What do you think?
>> - if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK)
>> - BUG();
>> + /* Warn if we're not lazy. */
>> + WARN_ON(cpumask_test_cpu(smp_processor_id(), mm_cpumask(loaded_mm)));
>
> We don't BUG() anymore?
We could. But, when the whole patch series is applied, the only
caller left is a somewhat dubious Xen optimization, and if we blindly
continue executing, I think the worst that happens is that we OOPS
later or that we get segfaults when we shouldn't get segfaults.
>
>>
>> switch_mm(NULL, &init_mm, NULL);
>> }
>> @@ -67,133 +67,118 @@ void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>> {
>> unsigned cpu = smp_processor_id();
>> struct mm_struct *real_prev = this_cpu_read(cpu_tlbstate.loaded_mm);
>> + u64 next_tlb_gen;
>
> Please sort function local variables declaration in a reverse christmas
> tree order:
>
> <type> longest_variable_name;
> <type> shorter_var_name;
> <type> even_shorter;
> <type> i;
>
>>
>> /*
>> - * NB: The scheduler will call us with prev == next when
>> - * switching from lazy TLB mode to normal mode if active_mm
>> - * isn't changing. When this happens, there is no guarantee
>> - * that CR3 (and hence cpu_tlbstate.loaded_mm) matches next.
>> + * NB: The scheduler will call us with prev == next when switching
>> + * from lazy TLB mode to normal mode if active_mm isn't changing.
>> + * When this happens, we don't assume that CR3 (and hence
>> + * cpu_tlbstate.loaded_mm) matches next.
>> *
>> * NB: leave_mm() calls us with prev == NULL and tsk == NULL.
>> */
>>
>> - this_cpu_write(cpu_tlbstate.state, TLBSTATE_OK);
>> + /* We don't want flush_tlb_func_* to run concurrently with us. */
>> + if (IS_ENABLED(CONFIG_PROVE_LOCKING))
>> + WARN_ON_ONCE(!irqs_disabled());
>> +
>> + VM_BUG_ON(read_cr3_pa() != __pa(real_prev->pgd));
>
> Why do we need that check? Can that ever happen?
It did in one particular buggy incarnation. It would also trigger if,
say, suspend/resume corrupts CR3. Admittedly this is unlikely, but
I'd rather catch it. Once PCID is on, corruption seems a bit less
farfetched -- this assertion will catch anyone who accidentally does
write_cr3(read_cr3_pa()).
>
>> if (real_prev == next) {
>> - /*
>> - * There's nothing to do: we always keep the per-mm control
>> - * regs in sync with cpu_tlbstate.loaded_mm. Just
>> - * sanity-check mm_cpumask.
>> - */
>> - if (WARN_ON_ONCE(!cpumask_test_cpu(cpu, mm_cpumask(next))))
>> - cpumask_set_cpu(cpu, mm_cpumask(next));
>> - return;
>> - }
>> + if (cpumask_test_cpu(cpu, mm_cpumask(next))) {
>> + /*
>> + * There's nothing to do: we weren't lazy, and we
>> + * aren't changing our mm. We don't need to flush
>> + * anything, nor do we need to update CR3, CR4, or
>> + * LDTR.
>> + */
>
> Nice comment.
>
>> + return;
>> + }
>> +
>> + /* Resume remote flushes and then read tlb_gen. */
>> + cpumask_set_cpu(cpu, mm_cpumask(next));
>> + next_tlb_gen = atomic64_read(&next->context.tlb_gen);
>> +
>> + VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) !=
>> + next->context.ctx_id);
>
> I guess this check should be right under the if (real_prev == next), right?
Moved.
>
>> +
>> + if (this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen) <
>> + next_tlb_gen) {
>
> Yeah, let it stick out - that trailing '<' doesn't make it any nicer.
>
>> + /*
>> + * Ideally, we'd have a flush_tlb() variant that
>> + * takes the known CR3 value as input. This would
>> + * be faster on Xen PV and on hypothetical CPUs
>> + * on which INVPCID is fast.
>> + */
>> + this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
>> + next_tlb_gen);
>> + write_cr3(__pa(next->pgd));
>
> <---- newline here.
>
>> + /*
>> + * This gets called via leave_mm() in the idle path
>> + * where RCU functions differently. Tracing normally
>> + * uses RCU, so we have to call the tracepoint
>> + * specially here.
>> + */
>> + trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
>> + TLB_FLUSH_ALL);
>> + }
>>
>> - if (IS_ENABLED(CONFIG_VMAP_STACK)) {
>> /*
>> - * If our current stack is in vmalloc space and isn't
>> - * mapped in the new pgd, we'll double-fault. Forcibly
>> - * map it.
>> + * We just exited lazy mode, which means that CR4 and/or LDTR
>> + * may be stale. (Changes to the required CR4 and LDTR states
>> + * are not reflected in tlb_gen.)
>
> We need that comment because... ? I mean, we do update CR4/LDTR at the
> end of the function.
I'm trying to explain to the potentially confused reader why we fall
through and update CR4 and LDTR even if we decided not to update the
TLB.
>
>> */
>> - unsigned int stack_pgd_index = pgd_index(current_stack_pointer());
>> + } else {
>> + if (IS_ENABLED(CONFIG_VMAP_STACK)) {
>> + /*
>> + * If our current stack is in vmalloc space and isn't
>> + * mapped in the new pgd, we'll double-fault. Forcibly
>> + * map it.
>> + */
>> + unsigned int stack_pgd_index =
>
> Shorten that var name and make it fit into 80ish cols so that the
> linebreak is gone.
Done.
>
>> + pgd_index(current_stack_pointer());
>> +
>> + pgd_t *pgd = next->pgd + stack_pgd_index;
>> +
>> + if (unlikely(pgd_none(*pgd)))
>> + set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
>> + }
>>
>> - pgd_t *pgd = next->pgd + stack_pgd_index;
>> + /* Stop remote flushes for the previous mm */
>> + if (cpumask_test_cpu(cpu, mm_cpumask(real_prev)))
>> + cpumask_clear_cpu(cpu, mm_cpumask(real_prev));
>
> cpumask_test_and_clear_cpu()
Same as before. I can change this, but it'll have different
performance characteristics. This one here will optimize the case
where we go lazy and then switch away.
>
>>
>> - if (unlikely(pgd_none(*pgd)))
>> - set_pgd(pgd, init_mm.pgd[stack_pgd_index]);
>> - }
>> + WARN_ON_ONCE(cpumask_test_cpu(cpu, mm_cpumask(next)));
>
> We warn if the next task is not lazy because...?
The next task had better not be in mm_cpumask(), but I agree that this
is minor. I'll change it to VM_WARN_ON_ONCE.
>
>> - this_cpu_write(cpu_tlbstate.loaded_mm, next);
>> - this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id, next->context.ctx_id);
>> - this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
>> - atomic64_read(&next->context.tlb_gen));
>> + /*
>> + * Start remote flushes and then read tlb_gen.
>> + */
>> + cpumask_set_cpu(cpu, mm_cpumask(next));
>> + next_tlb_gen = atomic64_read(&next->context.tlb_gen);
>>
>> - WARN_ON_ONCE(cpumask_test_cpu(cpu, mm_cpumask(next)));
>> - cpumask_set_cpu(cpu, mm_cpumask(next));
>> + VM_BUG_ON(this_cpu_read(cpu_tlbstate.ctxs[0].ctx_id) ==
>> + next->context.ctx_id);
>
> Also put it as the first statement after the else { ?
Done.
>
>> - /*
>> - * Re-load page tables.
>> - *
>> - * This logic has an ordering constraint:
>> - *
>> - * CPU 0: Write to a PTE for 'next'
>> - * CPU 0: load bit 1 in mm_cpumask. if nonzero, send IPI.
>> - * CPU 1: set bit 1 in next's mm_cpumask
>> - * CPU 1: load from the PTE that CPU 0 writes (implicit)
>> - *
>> - * We need to prevent an outcome in which CPU 1 observes
>> - * the new PTE value and CPU 0 observes bit 1 clear in
>> - * mm_cpumask. (If that occurs, then the IPI will never
>> - * be sent, and CPU 0's TLB will contain a stale entry.)
>> - *
>> - * The bad outcome can occur if either CPU's load is
>> - * reordered before that CPU's store, so both CPUs must
>> - * execute full barriers to prevent this from happening.
>> - *
>> - * Thus, switch_mm needs a full barrier between the
>> - * store to mm_cpumask and any operation that could load
>> - * from next->pgd. TLB fills are special and can happen
>> - * due to instruction fetches or for no reason at all,
>> - * and neither LOCK nor MFENCE orders them.
>> - * Fortunately, load_cr3() is serializing and gives the
>> - * ordering guarantee we need.
>> - */
>> - load_cr3(next->pgd);
>> -
>> - /*
>> - * This gets called via leave_mm() in the idle path where RCU
>> - * functions differently. Tracing normally uses RCU, so we have to
>> - * call the tracepoint specially here.
>> - */
>> - trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH, TLB_FLUSH_ALL);
>> + this_cpu_write(cpu_tlbstate.ctxs[0].ctx_id,
>> + next->context.ctx_id);
>> + this_cpu_write(cpu_tlbstate.ctxs[0].tlb_gen,
>> + next_tlb_gen);
>
> Yeah, just let all those three stick out.
>
> Also, no:
>
> if (this_cpu_read(cpu_tlbstate.ctxs[0].tlb_gen) <
> next_tlb_gen) {
>
> check?
What would the check do? Without PCID, we don't have a choice as to
whether we flush.
>
>> + this_cpu_write(cpu_tlbstate.loaded_mm, next);
>> + write_cr3(__pa(next->pgd));
>>
>> - /* Stop flush ipis for the previous mm */
>> - WARN_ON_ONCE(!cpumask_test_cpu(cpu, mm_cpumask(real_prev)) &&
>> - real_prev != &init_mm);
>> - cpumask_clear_cpu(cpu, mm_cpumask(real_prev));
>> + /*
>> + * This gets called via leave_mm() in the idle path where RCU
>> + * functions differently. Tracing normally uses RCU, so we
>> + * have to call the tracepoint specially here.
>> + */
>> + trace_tlb_flush_rcuidle(TLB_FLUSH_ON_TASK_SWITCH,
>> + TLB_FLUSH_ALL);
>> + }
>
> That's repeated as in the if-branch above. Move it out I guess.
I would have, but it changes later in the series and this reduces churn.
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-06-22 21:10 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tVfEm-nS-35@gated-at.bofh.it> |
| In reply to | #1672915 |
On Thu, Jun 22, 2017 at 10:47:29AM -0700, Andy Lutomirski wrote:
> I figured that some future reader of this patch might actually want to
> see this text, though.
Oh, don't get me wrong: with commit messages more is more, in the
general case. That's why I said "if".
> >> The UV tlbflush code is rather dated and should be changed.
>
> And I'd definitely like the UV maintainers to notice this part, now or
> in the future :) I don't want to personally touch the UV code with a
> ten-foot pole, but it really should be updated by someone who has a
> chance of getting it right and being able to test it.
Ah, could be because they moved recently and have hpe addresses now.
Lemme add them.
> >> +
> >> + if (cpumask_test_cpu(cpu, mm_cpumask(mm)))
> >> + cpumask_clear_cpu(cpu, mm_cpumask(mm));
> >
> > It seems we haz a helper for that: cpumask_test_and_clear_cpu() which
> > does BTR straightaway.
>
> Yeah, but I'm doing this for performance. I think that all the
> various one-line helpers do a LOCKed op right away, and I think it's
> faster to see if we can avoid the LOCKed op by trying an ordinary read
> first.
Right, the test part of the operation is unlocked so if that is the
likely case, it is a win.
> OTOH, maybe this is misguided -- if the cacheline lives somewhere else
> and we do end up needing to update it, we'll end up first sharing it
> and then making it exclusive, which increases the amount of cache
> coherency traffic, so maybe I'm optimizing for the wrong thing. What
> do you think?
Yeah, but we'll have to do that anyway for the locked operation. Ok,
let's leave it split like it is.
> It did in one particular buggy incarnation. It would also trigger if,
> say, suspend/resume corrupts CR3. Admittedly this is unlikely, but
> I'd rather catch it. Once PCID is on, corruption seems a bit less
> farfetched -- this assertion will catch anyone who accidentally does
> write_cr3(read_cr3_pa()).
Ok, but let's put a comment over it pls as it is not obvious when
something like that can happen.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Boris Ostrovsky <boris.ostrovsky@oracle.com> |
|---|---|
| Date | 2017-06-23 15:40 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tVwYy-2KP-29@gated-at.bofh.it> |
| In reply to | #1671363 |
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 1d7a7213a310..f5df56fb8b5c 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -1005,8 +1005,7 @@ static void xen_drop_mm_ref(struct mm_struct *mm)
> /* Get the "official" set of cpus referring to our pagetable. */
> if (!alloc_cpumask_var(&mask, GFP_ATOMIC)) {
> for_each_online_cpu(cpu) {
> - if (!cpumask_test_cpu(cpu, mm_cpumask(mm))
> - && per_cpu(xen_current_cr3, cpu) != __pa(mm->pgd))
> + if (per_cpu(xen_current_cr3, cpu) != __pa(mm->pgd))
> continue;
> smp_call_function_single(cpu, drop_mm_ref_this_cpu, mm, 1);
> }
>
I wonder then whether
cpumask_copy(mask, mm_cpumask(mm));
immediately below is needed.
-boris
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-23 17:30 +0200 |
| Subject | Re: [PATCH v3 06/11] x86/mm: Rework lazy TLB mode and TLB freshness tracking |
| Message-ID | <tVyGZ-3S8-11@gated-at.bofh.it> |
| In reply to | #1673561 |
On Fri, Jun 23, 2017 at 6:34 AM, Boris Ostrovsky
<boris.ostrovsky@oracle.com> wrote:
>
>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>> index 1d7a7213a310..f5df56fb8b5c 100644
>> --- a/arch/x86/xen/mmu_pv.c
>> +++ b/arch/x86/xen/mmu_pv.c
>> @@ -1005,8 +1005,7 @@ static void xen_drop_mm_ref(struct mm_struct *mm)
>> /* Get the "official" set of cpus referring to our pagetable. */
>> if (!alloc_cpumask_var(&mask, GFP_ATOMIC)) {
>> for_each_online_cpu(cpu) {
>> - if (!cpumask_test_cpu(cpu, mm_cpumask(mm))
>> - && per_cpu(xen_current_cr3, cpu) !=
>> __pa(mm->pgd))
>> + if (per_cpu(xen_current_cr3, cpu) !=
>> __pa(mm->pgd))
>> continue;
>> smp_call_function_single(cpu,
>> drop_mm_ref_this_cpu, mm, 1);
>> }
>>
>
>
> I wonder then whether
> cpumask_copy(mask, mm_cpumask(mm));
> immediately below is needed.
Probably not. I'll change it to cpumask_clear(). Then the two cases
in that function match better.
>
> -boris
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-21 07:30 +0200 |
| Subject | [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID |
| Message-ID | <tUGnh-1Tn-39@gated-at.bofh.it> |
| In reply to | #1671353 |
This adds two new variables to mmu_context_t: ctx_id and tlb_gen.
ctx_id uniquely identifies the mm_struct and will never be reused.
For a given mm_struct (and hence ctx_id), tlb_gen is a monotonic
count of the number of times that a TLB flush has been requested.
The pair (ctx_id, tlb_gen) can be used as an identifier for TLB
flush actions and will be used in subsequent patches to reliably
determine whether all needed TLB flushes have occurred on a given
CPU.
This patch is split out for ease of review. By itself, it has no
real effect other than creating and updating the new variables.
Signed-off-by: Andy Lutomirski <luto@kernel.org>
---
arch/x86/include/asm/mmu.h | 25 +++++++++++++++++++++++--
arch/x86/include/asm/mmu_context.h | 5 +++++
arch/x86/include/asm/tlbflush.h | 18 ++++++++++++++++++
arch/x86/mm/tlb.c | 6 ++++--
4 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/arch/x86/include/asm/mmu.h b/arch/x86/include/asm/mmu.h
index 79b647a7ebd0..bb8c597c2248 100644
--- a/arch/x86/include/asm/mmu.h
+++ b/arch/x86/include/asm/mmu.h
@@ -3,12 +3,28 @@
#include <linux/spinlock.h>
#include <linux/mutex.h>
+#include <linux/atomic.h>
/*
- * The x86 doesn't have a mmu context, but
- * we put the segment information here.
+ * x86 has arch-specific MMU state beyond what lives in mm_struct.
*/
typedef struct {
+ /*
+ * ctx_id uniquely identifies this mm_struct. A ctx_id will never
+ * be reused, and zero is not a valid ctx_id.
+ */
+ u64 ctx_id;
+
+ /*
+ * Any code that needs to do any sort of TLB flushing for this
+ * mm will first make its changes to the page tables, then
+ * increment tlb_gen, then flush. This lets the low-level
+ * flushing code keep track of what needs flushing.
+ *
+ * This is not used on Xen PV.
+ */
+ atomic64_t tlb_gen;
+
#ifdef CONFIG_MODIFY_LDT_SYSCALL
struct ldt_struct *ldt;
#endif
@@ -37,6 +53,11 @@ typedef struct {
#endif
} mm_context_t;
+#define INIT_MM_CONTEXT(mm) \
+ .context = { \
+ .ctx_id = 1, \
+ }
+
void leave_mm(int cpu);
#endif /* _ASM_X86_MMU_H */
diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
index ecfcb6643c9b..e5295d485899 100644
--- a/arch/x86/include/asm/mmu_context.h
+++ b/arch/x86/include/asm/mmu_context.h
@@ -129,9 +129,14 @@ static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
this_cpu_write(cpu_tlbstate.state, TLBSTATE_LAZY);
}
+extern atomic64_t last_mm_ctx_id;
+
static inline int init_new_context(struct task_struct *tsk,
struct mm_struct *mm)
{
+ mm->context.ctx_id = atomic64_inc_return(&last_mm_ctx_id);
+ atomic64_set(&mm->context.tlb_gen, 0);
+
#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
if (cpu_feature_enabled(X86_FEATURE_OSPKE)) {
/* pkey 0 is the default and always allocated */
diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 50ea3482e1d1..1eb946c0507e 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -57,6 +57,23 @@ static inline void invpcid_flush_all_nonglobals(void)
__invpcid(0, 0, INVPCID_TYPE_ALL_NON_GLOBAL);
}
+static inline u64 bump_mm_tlb_gen(struct mm_struct *mm)
+{
+ u64 new_tlb_gen;
+
+ /*
+ * Bump the generation count. This also serves as a full barrier
+ * that synchronizes with switch_mm: callers are required to order
+ * their read of mm_cpumask after their writes to the paging
+ * structures.
+ */
+ smp_mb__before_atomic();
+ new_tlb_gen = atomic64_inc_return(&mm->context.tlb_gen);
+ smp_mb__after_atomic();
+
+ return new_tlb_gen;
+}
+
#ifdef CONFIG_PARAVIRT
#include <asm/paravirt.h>
#else
@@ -262,6 +279,7 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
static inline void arch_tlbbatch_add_mm(struct arch_tlbflush_unmap_batch *batch,
struct mm_struct *mm)
{
+ bump_mm_tlb_gen(mm);
cpumask_or(&batch->cpumask, &batch->cpumask, mm_cpumask(mm));
}
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index fd593833a854..6d9d37323a43 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -28,6 +28,8 @@
* Implement flush IPI by CALL_FUNCTION_VECTOR, Alex Shi
*/
+atomic64_t last_mm_ctx_id = ATOMIC64_INIT(1);
+
void leave_mm(int cpu)
{
struct mm_struct *loaded_mm = this_cpu_read(cpu_tlbstate.loaded_mm);
@@ -286,8 +288,8 @@ void flush_tlb_mm_range(struct mm_struct *mm, unsigned long start,
cpu = get_cpu();
- /* Synchronize with switch_mm. */
- smp_mb();
+ /* This is also a barrier that synchronizes with switch_mm(). */
+ bump_mm_tlb_gen(mm);
/* Should we flush just the requested range? */
if ((end != TLB_FLUSH_ALL) &&
--
2.9.4
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2017-06-21 10:10 +0200 |
| Subject | Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID |
| Message-ID | <tUIS7-3AK-41@gated-at.bofh.it> |
| In reply to | #1671364 |
On Tue, 20 Jun 2017, Andy Lutomirski wrote: > This adds two new variables to mmu_context_t: ctx_id and tlb_gen. > ctx_id uniquely identifies the mm_struct and will never be reused. > For a given mm_struct (and hence ctx_id), tlb_gen is a monotonic > count of the number of times that a TLB flush has been requested. > The pair (ctx_id, tlb_gen) can be used as an identifier for TLB > flush actions and will be used in subsequent patches to reliably > determine whether all needed TLB flushes have occurred on a given > CPU. > > This patch is split out for ease of review. By itself, it has no > real effect other than creating and updating the new variables. Thanks for splitting this apart! > > Signed-off-by: Andy Lutomirski <luto@kernel.org> Reviewed-by: Thomas Gleixner <tglx@linutronix.de>
[toc] | [prev] | [next] | [standalone]
| From | Borislav Petkov <bp@alien8.de> |
|---|---|
| Date | 2017-06-21 12:40 +0200 |
| Subject | Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID |
| Message-ID | <tULdg-4VV-15@gated-at.bofh.it> |
| In reply to | #1671364 |
On Tue, Jun 20, 2017 at 10:22:10PM -0700, Andy Lutomirski wrote:
> - * The x86 doesn't have a mmu context, but
> - * we put the segment information here.
> + * x86 has arch-specific MMU state beyond what lives in mm_struct.
> */
> typedef struct {
> + /*
> + * ctx_id uniquely identifies this mm_struct. A ctx_id will never
> + * be reused, and zero is not a valid ctx_id.
> + */
> + u64 ctx_id;
> +
> + /*
> + * Any code that needs to do any sort of TLB flushing for this
> + * mm will first make its changes to the page tables, then
> + * increment tlb_gen, then flush. This lets the low-level
> + * flushing code keep track of what needs flushing.
> + *
> + * This is not used on Xen PV.
> + */
> + atomic64_t tlb_gen;
> +
> #ifdef CONFIG_MODIFY_LDT_SYSCALL
> struct ldt_struct *ldt;
> #endif
> @@ -37,6 +53,11 @@ typedef struct {
> #endif
> } mm_context_t;
>
> +#define INIT_MM_CONTEXT(mm) \
> + .context = { \
> + .ctx_id = 1, \
So ctx_id of 0 is invalid?
Let's state that explicitly. We could even use it to sanity-check mms or
whatever.
> + }
> +
> void leave_mm(int cpu);
>
> #endif /* _ASM_X86_MMU_H */
> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
> index ecfcb6643c9b..e5295d485899 100644
> --- a/arch/x86/include/asm/mmu_context.h
> +++ b/arch/x86/include/asm/mmu_context.h
> @@ -129,9 +129,14 @@ static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
> this_cpu_write(cpu_tlbstate.state, TLBSTATE_LAZY);
> }
>
> +extern atomic64_t last_mm_ctx_id;
I think we prefer externs/variable defines at the beginning of the file,
not intermixed with functions.
> +
> static inline int init_new_context(struct task_struct *tsk,
> struct mm_struct *mm)
> {
> + mm->context.ctx_id = atomic64_inc_return(&last_mm_ctx_id);
> + atomic64_set(&mm->context.tlb_gen, 0);
> +
> #ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS
> if (cpu_feature_enabled(X86_FEATURE_OSPKE)) {
> /* pkey 0 is the default and always allocated */
> diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
> index 50ea3482e1d1..1eb946c0507e 100644
> --- a/arch/x86/include/asm/tlbflush.h
> +++ b/arch/x86/include/asm/tlbflush.h
> @@ -57,6 +57,23 @@ static inline void invpcid_flush_all_nonglobals(void)
> __invpcid(0, 0, INVPCID_TYPE_ALL_NON_GLOBAL);
> }
>
> +static inline u64 bump_mm_tlb_gen(struct mm_struct *mm)
inc_mm_tlb_gen() I guess. git grep says like "inc" more :-)
> +{
> + u64 new_tlb_gen;
> +
> + /*
> + * Bump the generation count. This also serves as a full barrier
> + * that synchronizes with switch_mm: callers are required to order
Please end function names with parentheses.
> + * their read of mm_cpumask after their writes to the paging
> + * structures.
> + */
> + smp_mb__before_atomic();
> + new_tlb_gen = atomic64_inc_return(&mm->context.tlb_gen);
> + smp_mb__after_atomic();
> +
> + return new_tlb_gen;
> +}
> +
> #ifdef CONFIG_PARAVIRT
> #include <asm/paravirt.h>
> #else
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
[toc] | [prev] | [next] | [standalone]
| From | Andy Lutomirski <luto@kernel.org> |
|---|---|
| Date | 2017-06-21 18:40 +0200 |
| Subject | Re: [PATCH v3 04/11] x86/mm: Give each mm TLB flush generation a unique ID |
| Message-ID | <tUQPE-6J-23@gated-at.bofh.it> |
| In reply to | #1671600 |
On Wed, Jun 21, 2017 at 3:33 AM, Borislav Petkov <bp@alien8.de> wrote:
> On Tue, Jun 20, 2017 at 10:22:10PM -0700, Andy Lutomirski wrote:
>> +#define INIT_MM_CONTEXT(mm) \
>> + .context = { \
>> + .ctx_id = 1, \
>
> So ctx_id of 0 is invalid?
>
> Let's state that explicitly. We could even use it to sanity-check mms or
> whatever.
It's stated explicitly in the comment where it's declared in the same file.
>
>> + }
>> +
>> void leave_mm(int cpu);
>>
>> #endif /* _ASM_X86_MMU_H */
>> diff --git a/arch/x86/include/asm/mmu_context.h b/arch/x86/include/asm/mmu_context.h
>> index ecfcb6643c9b..e5295d485899 100644
>> --- a/arch/x86/include/asm/mmu_context.h
>> +++ b/arch/x86/include/asm/mmu_context.h
>> @@ -129,9 +129,14 @@ static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
>> this_cpu_write(cpu_tlbstate.state, TLBSTATE_LAZY);
>> }
>>
>> +extern atomic64_t last_mm_ctx_id;
>
> I think we prefer externs/variable defines at the beginning of the file,
> not intermixed with functions.
Done
>
>> +static inline u64 bump_mm_tlb_gen(struct mm_struct *mm)
>
> inc_mm_tlb_gen() I guess. git grep says like "inc" more :-)
Done
>> + * that synchronizes with switch_mm: callers are required to order
>
> Please end function names with parentheses.
Done.
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web