The recent introduction of batched TLB flushes causes accounting problems in
vmstat and misses some tracepoints. In addition, I am afraid it might cause
problems in some platforms (Xen, UV) since it uses non-standard APIs.
v1->v2
* Added second patch to use standard api for invalidations
* cc'ing linux-mm
Nadav Amit (2):
x86/mm: TLB_REMOTE_SEND_IPI should count pages
mm/rmap: batched invalidations should use existing api
arch/x86/include/asm/tlbflush.h | 6 ------
arch/x86/mm/tlb.c | 14 ++++++++++----
mm/rmap.c | 28 +++++++---------------------
3 files changed, 17 insertions(+), 31 deletions(-)
--
2.5.0
The recently introduced batched invalidations mechanism uses its own
mechanism for shootdown. However, it does wrong accounting of interrupts
(e.g., inc_irq_stat is called for local invalidations), trace-points
(e.g., TLB_REMOTE_SHOOTDOWN for local invalidations) and may break some
platforms as it bypasses the invalidation mechanisms of Xen and SGI UV.
This patch reuses the existing TLB flushing mechnaisms instead. We use
NULL as mm to indicate a global invalidation is required.
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/include/asm/tlbflush.h | 6 ------
arch/x86/mm/tlb.c | 2 +-
mm/rmap.c | 28 +++++++---------------------
3 files changed, 8 insertions(+), 28 deletions(-)
diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 6df2029..cd79194 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -261,12 +261,6 @@ static inline void reset_lazy_tlbstate(void)
#endif /* SMP */
-/* Not inlined due to inc_irq_stat not being defined yet */
-#define flush_tlb_local() { \
- inc_irq_stat(irq_tlb_count); \
- local_flush_tlb(); \
-}
-
#ifndef CONFIG_PARAVIRT
#define flush_tlb_others(mask, mm, start, end) \
native_flush_tlb_others(mask, mm, start, end)
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 5fb6ada..fe9b9f7 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -104,7 +104,7 @@ static void flush_tlb_func(void *info)
inc_irq_stat(irq_tlb_count);
- if (f->flush_mm != this_cpu_read(cpu_tlbstate.active_mm))
+ if (f->flush_mm && f->flush_mm != this_cpu_read(cpu_tlbstate.active_mm))
return;
count_vm_tlb_event(NR_TLB_REMOTE_FLUSH_RECEIVED);
diff --git a/mm/rmap.c b/mm/rmap.c
index 79f3bf0..37fb08f 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -569,19 +569,6 @@ void page_unlock_anon_vma_read(struct anon_vma *anon_vma)
}
#ifdef CONFIG_ARCH_WANT_BATCHED_UNMAP_TLB_FLUSH
-static void percpu_flush_tlb_batch_pages(void *data)
-{
- /*
- * All TLB entries are flushed on the assumption that it is
- * cheaper to flush all TLBs and let them be refilled than
- * flushing individual PFNs. Note that we do not track mm's
- * to flush as that might simply be multiple full TLB flushes
- * for no gain.
- */
- count_vm_tlb_event(NR_TLB_REMOTE_FLUSH_RECEIVED);
- flush_tlb_local();
-}
-
/*
* Flush TLB entries for recently unmapped pages from remote CPUs. It is
* important if a PTE was dirty when it was unmapped that it's flushed
@@ -598,15 +585,14 @@ void try_to_unmap_flush(void)
cpu = get_cpu();
- trace_tlb_flush(TLB_REMOTE_SHOOTDOWN, -1UL);
-
- if (cpumask_test_cpu(cpu, &tlb_ubc->cpumask))
- percpu_flush_tlb_batch_pages(&tlb_ubc->cpumask);
-
- if (cpumask_any_but(&tlb_ubc->cpumask, cpu) < nr_cpu_ids) {
- smp_call_function_many(&tlb_ubc->cpumask,
- percpu_flush_tlb_batch_pages, (void *)tlb_ubc, true);
+ if (cpumask_test_cpu(cpu, &tlb_ubc->cpumask)) {
+ count_vm_tlb_event(NR_TLB_LOCAL_FLUSH_ALL);
+ local_flush_tlb();
+ trace_tlb_flush(TLB_LOCAL_SHOOTDOWN, TLB_FLUSH_ALL);
}
+
+ if (cpumask_any_but(&tlb_ubc->cpumask, cpu) < nr_cpu_ids)
+ flush_tlb_others(&tlb_ubc->cpumask, NULL, 0, TLB_FLUSH_ALL);
cpumask_clear(&tlb_ubc->cpumask);
tlb_ubc->flush_required = false;
tlb_ubc->writable = false;
--
2.5.0
TLB_REMOTE_SEND_IPI was recently introduced, but it counts bytes instead
of pages. In addition, it does not report correctly the case in which
flush_tlb_page flushes a page. Fix it to be consistent with other TLB
counters.
Fixes: 4595f9620cda8a1e973588e743cf5f8436dd20c6
Signed-off-by: Nadav Amit <[email protected]>
---
arch/x86/mm/tlb.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
index 8f4cc3d..5fb6ada 100644
--- a/arch/x86/mm/tlb.c
+++ b/arch/x86/mm/tlb.c
@@ -106,8 +106,6 @@ static void flush_tlb_func(void *info)
if (f->flush_mm != this_cpu_read(cpu_tlbstate.active_mm))
return;
- if (!f->flush_end)
- f->flush_end = f->flush_start + PAGE_SIZE;
count_vm_tlb_event(NR_TLB_REMOTE_FLUSH_RECEIVED);
if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK) {
@@ -135,12 +133,20 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
unsigned long end)
{
struct flush_tlb_info info;
+
+ if (end == 0)
+ end = start + PAGE_SIZE;
info.flush_mm = mm;
info.flush_start = start;
info.flush_end = end;
count_vm_tlb_event(NR_TLB_REMOTE_FLUSH);
- trace_tlb_flush(TLB_REMOTE_SEND_IPI, end - start);
+ if (end == TLB_FLUSH_ALL)
+ trace_tlb_flush(TLB_REMOTE_SEND_IPI, TLB_FLUSH_ALL);
+ else
+ trace_tlb_flush(TLB_REMOTE_SEND_IPI,
+ (end - start) >> PAGE_SHIFT);
+
if (is_uv_system()) {
unsigned int cpu;
--
2.5.0
The commit message should have said:
Fixes: 5b74283ab251b9db55cbbe31d19ca72482103290
(and not what it currently says).
Let me know whether to submit v3.
Nadav
On 3/26/16, 1:25 AM, "Nadav Amit" <[email protected]> wrote:
>TLB_REMOTE_SEND_IPI was recently introduced, but it counts bytes instead
>of pages. In addition, it does not report correctly the case in which
>flush_tlb_page flushes a page. Fix it to be consistent with other TLB
>counters.
>
>Fixes: 4595f9620cda8a1e973588e743cf5f8436dd20c6
>
>Signed-off-by: Nadav Amit <[email protected]>
>---
> arch/x86/mm/tlb.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
>diff --git a/arch/x86/mm/tlb.c b/arch/x86/mm/tlb.c
>index 8f4cc3d..5fb6ada 100644
>--- a/arch/x86/mm/tlb.c
>+++ b/arch/x86/mm/tlb.c
>@@ -106,8 +106,6 @@ static void flush_tlb_func(void *info)
>
> if (f->flush_mm != this_cpu_read(cpu_tlbstate.active_mm))
> return;
>- if (!f->flush_end)
>- f->flush_end = f->flush_start + PAGE_SIZE;
>
> count_vm_tlb_event(NR_TLB_REMOTE_FLUSH_RECEIVED);
> if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK) {
>@@ -135,12 +133,20 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
> unsigned long end)
> {
> struct flush_tlb_info info;
>+
>+ if (end == 0)
>+ end = start + PAGE_SIZE;
> info.flush_mm = mm;
> info.flush_start = start;
> info.flush_end = end;
>
> count_vm_tlb_event(NR_TLB_REMOTE_FLUSH);
>- trace_tlb_flush(TLB_REMOTE_SEND_IPI, end - start);
>+ if (end == TLB_FLUSH_ALL)
>+ trace_tlb_flush(TLB_REMOTE_SEND_IPI, TLB_FLUSH_ALL);
>+ else
>+ trace_tlb_flush(TLB_REMOTE_SEND_IPI,
>+ (end - start) >> PAGE_SHIFT);
>+
> if (is_uv_system()) {
> unsigned int cpu;
>
>--
>2.5.0
>
On Sat, 26 Mar 2016 01:25:04 -0700 Nadav Amit <[email protected]> wrote:
> TLB_REMOTE_SEND_IPI was recently introduced, but it counts bytes instead
> of pages. In addition, it does not report correctly the case in which
> flush_tlb_page flushes a page. Fix it to be consistent with other TLB
> counters.
>
> Fixes: 4595f9620cda8a1e973588e743cf5f8436dd20c6
I think you mean 5b74283ab251b9 ("x86, mm: trace when an IPI is about
to be sent")?
> --- a/arch/x86/mm/tlb.c
> +++ b/arch/x86/mm/tlb.c
> @@ -106,8 +106,6 @@ static void flush_tlb_func(void *info)
>
> if (f->flush_mm != this_cpu_read(cpu_tlbstate.active_mm))
> return;
> - if (!f->flush_end)
> - f->flush_end = f->flush_start + PAGE_SIZE;
>
> count_vm_tlb_event(NR_TLB_REMOTE_FLUSH_RECEIVED);
> if (this_cpu_read(cpu_tlbstate.state) == TLBSTATE_OK) {
> @@ -135,12 +133,20 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
> unsigned long end)
> {
> struct flush_tlb_info info;
> +
> + if (end == 0)
> + end = start + PAGE_SIZE;
> info.flush_mm = mm;
> info.flush_start = start;
> info.flush_end = end;
>
> count_vm_tlb_event(NR_TLB_REMOTE_FLUSH);
> - trace_tlb_flush(TLB_REMOTE_SEND_IPI, end - start);
> + if (end == TLB_FLUSH_ALL)
> + trace_tlb_flush(TLB_REMOTE_SEND_IPI, TLB_FLUSH_ALL);
> + else
> + trace_tlb_flush(TLB_REMOTE_SEND_IPI,
> + (end - start) >> PAGE_SHIFT);
> +
> if (is_uv_system()) {
> unsigned int cpu;
On 3/28/16, 1:38 PM, "Andrew Morton" <[email protected]> wrote:
>On Sat, 26 Mar 2016 01:25:04 -0700 Nadav Amit <[email protected]> wrote:
>
>> TLB_REMOTE_SEND_IPI was recently introduced, but it counts bytes instead
>> of pages. In addition, it does not report correctly the case in which
>> flush_tlb_page flushes a page. Fix it to be consistent with other TLB
>> counters.
>>
>> Fixes: 4595f9620cda8a1e973588e743cf5f8436dd20c6
>
>I think you mean 5b74283ab251b9 ("x86, mm: trace when an IPI is about
>to be sent")?
Indeed.