x86 TLB flush cleanups, moving toward PCID support

flush_tlb_page() was very similar to flush_tlb_mm_range() except that it had a couple of issues:

- It was missing an smp_mb() in the case where current->active_mm != mm. (This is a longstanding bug reported by Nadav Amit.)

- It was missing tracepoints and vm counter updates.

The only reason that I can see for keeping it at as a separate function is that it could avoid a few branches that flush_tlb_mm_range() needs to decide to flush just one page. This hardly seems worthwhile. If we decide we want to get rid of those branches again, a better way would be to introduce an __flush_tlb_mm_range() helper and make both flush_tlb_page() and flush_tlb_mm_range() use it.

Cc Rik van Riel <[email protected]>

Cc Dave Hansen <[email protected]>

Cc Nadav Amit <[email protected]>

Cc Michal Hocko <[email protected]>

Cc Andrew Morton <[email protected]>

Signed-off-by Andy Lutomirski <[email protected]>

---
 arch/x86/include/asm/tlbflush.h |  6 +++++-
 arch/x86/mm/tlb.c               | 27 ---------------------------
 2 files changed, 5 insertions(+), 28 deletions(-)

arch/x86/include/asm/tlbflush.h +0 -0

index 6ed9ea469b48..5ed64cdaf536 100644
@@ -307,11 +307,15 @@ static inline void flush_tlb_kernel_range(unsigned long start,
 		flush_tlb_mm_range(vma->vm_mm, start, end, vma->vm_flags)

arch/x86/include/asm/tlbflush.h static inline void flush_tlb_kernel_range(unsigned long start, · line 316

Quoted 2 lines

arch/x86/include/asm/tlbflush.h +2 -0

+{
+     flush_tlb_mm_range(vma->vm_mm, a, a + PAGE_SIZE, 0);

                                                         VM_NONE);

mm/rmap.c +0 -13

index f6838015810f..2e568c82f477 100644
@@ -579,25 +579,12 @@ void page_unlock_anon_vma_read(struct anon_vma *anon_vma)
 void try_to_unmap_flush(void)
 {
 	struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
-	int cpu;
 
 	if (!tlb_ubc->flush_required)
 		return;
 
-	cpu = get_cpu();
-
-	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;
-	put_cpu();
 }
 
 /* Flush iff there are potentially writable TLB entries that can race with IO */

Quoted 38 lines · Andy Lutomirski

mm/rmap.c +1 -14

index f6838015810f..2e568c82f477 100644
@@ -579,25 +579,12 @@ void page_unlock_anon_vma_read(struct anon_vma *anon_vma)
 void try_to_unmap_flush(void)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
-     int cpu;
 
      if (!tlb_ubc->flush_required)
              return;
 
-     cpu = get_cpu();
-
-     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;
-     put_cpu();
 }
 
 /* Flush iff there are potentially writable TLB entries that can race with IO */

@@ -613,7 +600,7 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
 
-     cpumask_or(&tlb_ubc->cpumask, &tlb_ubc->cpumask, mm_cpumask(mm));
+     arch_tlbbatch_add_mm(&tlb_ubc->arch, mm);
      tlb_ubc->flush_required = true;
 

mm/rmap.c static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable) · line 606

Quoted 1 line

mm/rmap.c +0 -0

      /*

Looking at this patch in isolation, how can this be safe? It removes TLB flushes from the generic code. Do other patches in the series fix this up?

Quoted 39 lines · Dave Hansen

Quoted 38 lines · Andy Lutomirski

mm/rmap.c +1 -14

index f6838015810f..2e568c82f477 100644
@@ -579,25 +579,12 @@ void page_unlock_anon_vma_read(struct anon_vma *anon_vma)
 void try_to_unmap_flush(void)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
-     int cpu;
 
      if (!tlb_ubc->flush_required)
              return;
 
-     cpu = get_cpu();
-
-     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;
-     put_cpu();
 }
 
 /* Flush iff there are potentially writable TLB entries that can race with IO */

@@ -613,7 +600,7 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
 
-     cpumask_or(&tlb_ubc->cpumask, &tlb_ubc->cpumask, mm_cpumask(mm));
+     arch_tlbbatch_add_mm(&tlb_ubc->arch, mm);
      tlb_ubc->flush_required = true;
 

Quoted 5 lines

mm/rmap.c static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable) · line 606

Quoted 1 line

mm/rmap.c +0 -0

      /*

Looking at this patch in isolation, how can this be safe? It removes TLB flushes from the generic code. Do other patches in the series fix this up?

Hmm? Unless I totally screwed this up, this patch just moves the flushes around -- it shouldn't remove any flushes.

Quoted 39 lines · Dave Hansen

Quoted 38 lines · Andy Lutomirski

mm/rmap.c +1 -14

index f6838015810f..2e568c82f477 100644
@@ -579,25 +579,12 @@ void page_unlock_anon_vma_read(struct anon_vma *anon_vma)
 void try_to_unmap_flush(void)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
-     int cpu;
 
      if (!tlb_ubc->flush_required)
              return;
 
-     cpu = get_cpu();
-
-     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;
-     put_cpu();
 }
 
 /* Flush iff there are potentially writable TLB entries that can race with IO */

@@ -613,7 +600,7 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
 
-     cpumask_or(&tlb_ubc->cpumask, &tlb_ubc->cpumask, mm_cpumask(mm));
+     arch_tlbbatch_add_mm(&tlb_ubc->arch, mm);
      tlb_ubc->flush_required = true;
 

Quoted 5 lines

mm/rmap.c static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable) · line 606

Quoted 1 line

mm/rmap.c +0 -0

      /*

Looking at this patch in isolation, how can this be safe? It removes TLB flushes from the generic code. Do other patches in the series fix this up?

Hmm? Unless I totally screwed this up, this patch just moves the flushes around -- it shouldn't remove any flushes.

Quoted 40 lines · Andy Lutomirski

Quoted 39 lines · Dave Hansen

Quoted 38 lines · Andy Lutomirski

mm/rmap.c +1 -14

index f6838015810f..2e568c82f477 100644
@@ -579,25 +579,12 @@ void page_unlock_anon_vma_read(struct anon_vma *anon_vma)
 void try_to_unmap_flush(void)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
-     int cpu;
 
      if (!tlb_ubc->flush_required)
              return;
 
-     cpu = get_cpu();
-
-     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;
-     put_cpu();
 }
 
 /* Flush iff there are potentially writable TLB entries that can race with IO */

@@ -613,7 +600,7 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
 {
      struct tlbflush_unmap_batch *tlb_ubc = &current->tlb_ubc;
 
-     cpumask_or(&tlb_ubc->cpumask, &tlb_ubc->cpumask, mm_cpumask(mm));
+     arch_tlbbatch_add_mm(&tlb_ubc->arch, mm);
      tlb_ubc->flush_required = true;
 

Quoted 8 lines

Quoted 5 lines

mm/rmap.c static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable) · line 606

Quoted 1 line

mm/rmap.c +0 -0

      /*

Looking at this patch in isolation, how can this be safe? It removes TLB flushes from the generic code. Do other patches in the series fix this up?

Hmm? Unless I totally screwed this up, this patch just moves the flushes around -- it shouldn't remove any flushes.

This takes a flush out of try_to_unmap_flush(). It adds code for arch_tlbbatch_flush(), but not *calls* to arch_tlbbatch_flush() that I can see.

I actually don't see _any_ in the whole series in a quick grepping. Am I just missing them?

arch/x86/mm/tlb.c +5 -10

@@ -243,15 +237,15 @@ static void flush_tlb_func(void *info)
 		return;
 	}
 
-	if (f->flush_end == TLB_FLUSH_ALL) {
+	if (f->end == TLB_FLUSH_ALL) {
 		local_flush_tlb();
 		trace_tlb_flush(TLB_REMOTE_SHOOTDOWN, TLB_FLUSH_ALL);
 	} else {
 		unsigned long addr;
 		unsigned long nr_pages =
-			(f->flush_end - f->flush_start) / PAGE_SIZE;
-		addr = f->flush_start;
-		while (addr < f->flush_end) {
+			(f->end - f->start) / PAGE_SIZE;
+		addr = f->start;
+		while (addr < f->end) {
 			__flush_tlb_single(addr);
 			addr += PAGE_SIZE;
 		}

@@ -260,33 +254,27 @@ static void flush_tlb_func(void *info)
 }
 
 void native_flush_tlb_others(const struct cpumask *cpumask,
-				 struct mm_struct *mm, unsigned long start,
-				 unsigned long end)
+			     const struct flush_tlb_info *info)
 {
-	struct flush_tlb_info info;
-
-	info.flush_mm = mm;
-	info.flush_start = start;

Quoted 41 lines · Andy Lutomirski

arch/x86/mm/tlb.c +6 -13

@@ -243,15 +237,15 @@ static void flush_tlb_func(void *info)
              return;
      }
 
-     if (f->flush_end == TLB_FLUSH_ALL) {
+     if (f->end == TLB_FLUSH_ALL) {
              local_flush_tlb();
              trace_tlb_flush(TLB_REMOTE_SHOOTDOWN, TLB_FLUSH_ALL);
      } else {
              unsigned long addr;
              unsigned long nr_pages =
-                     (f->flush_end - f->flush_start) / PAGE_SIZE;
-             addr = f->flush_start;
-             while (addr < f->flush_end) {
+                     (f->end - f->start) / PAGE_SIZE;
+             addr = f->start;
+             while (addr < f->end) {
                      __flush_tlb_single(addr);
                      addr += PAGE_SIZE;
              }

@@ -260,33 +254,27 @@ static void flush_tlb_func(void *info)
 }
 
 void native_flush_tlb_others(const struct cpumask *cpumask,
-                              struct mm_struct *mm, unsigned long start,
-                              unsigned long end)
+                          const struct flush_tlb_info *info)
 {
-     struct flush_tlb_info info;
-
-     info.flush_mm = mm;
-     info.flush_start = start;
-     info.flush_end = end;
-
      count_vm_tlb_event(NR_TLB_REMOTE_FLUSH);
-     if (end == TLB_FLUSH_ALL)
+     if (info->end == TLB_FLUSH_ALL)
              trace_tlb_flush(TLB_REMOTE_SEND_IPI, TLB_FLUSH_ALL);

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

Quoted 4 lines

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

      else
              trace_tlb_flush(TLB_REMOTE_SEND_IPI,
-                             (end - start) >> PAGE_SHIFT);
+                             (info->end - info->start) >> PAGE_SHIFT);

I know it is stupid, but since you already change the code, can you make flush_tlb_func() and native_flush_tlb_others() consistent in the way they compute the number of pages? (either ‘>> PAGE_SHIFT’ or ‘/ PAGE_SIZE’)?

On a different topic: I do not like or actually understand why TLBSTATE_OK is defined as 1 and not 0. The very least it would generate a better code.

Thanks,
Nadav

Quoted 6 lines · Andy Lutomirski

/* context.lock is held for us, so we don't need any locking. */ static void flush_ldt(void *current_mm)
{

+     struct mm_struct *mm = current_mm;
      mm_context_t *pc;

arch/x86/kernel/ldt.c line 30

Quoted 2 lines

arch/x86/kernel/ldt.c +1 -1

-     if (current->active_mm != current_mm)
+     if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)

While functional correct, this really should compare against 'mm'.

arch/x86/kernel/ldt.c line 30

Quoted 8 lines · Andy Lutomirski

/* context.lock is held for us, so we don't need any locking. */ static void flush_ldt(void *current_mm)
{

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

+	struct mm_struct *mm = current_mm;
 	mm_context_t *pc;
 
-	if (current->active_mm != current_mm)
+	if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)

While functional correct, this really should compare against 'mm'.

arch/x86/kernel/ldt.c line 33

Quoted 4 lines

arch/x86/kernel/ldt.c +1 -1

 		return;
 
-	pc = &current->active_mm->context;
+	pc = &mm->context;

Quoted 12 lines · Thomas Gleixner

arch/x86/kernel/ldt.c line 30

Quoted 8 lines · Andy Lutomirski

/* context.lock is held for us, so we don't need any locking. */ static void flush_ldt(void *current_mm)
{

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

+	struct mm_struct *mm = current_mm;
 	mm_context_t *pc;
 
-	if (current->active_mm != current_mm)
+	if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)

While functional correct, this really should compare against 'mm'.

Quoted 4 lines

arch/x86/kernel/ldt.c line 33

Quoted 4 lines

arch/x86/kernel/ldt.c +1 -1

 		return;
 
-	pc = &current->active_mm->context;
+	pc = &mm->context;

So this appears to be the function:

static void flush_ldt(void *current_mm)

 {
        struct mm_struct *mm = current_mm;
        mm_context_t *pc;

        if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
                return;

        pc = &mm->context;
        set_ldt(pc->ldt->entries, pc->ldt->size);

}

why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?

Quoted 30 lines · Ingo Molnar

Quoted 12 lines · Thomas Gleixner

arch/x86/kernel/ldt.c line 30

Quoted 8 lines · Andy Lutomirski

/* context.lock is held for us, so we don't need any locking. */ static void flush_ldt(void *current_mm)
{

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

+	struct mm_struct *mm = current_mm;
 	mm_context_t *pc;
 
-	if (current->active_mm != current_mm)
+	if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)

While functional correct, this really should compare against 'mm'.

Quoted 4 lines

arch/x86/kernel/ldt.c line 33

Quoted 4 lines

arch/x86/kernel/ldt.c +1 -1

 		return;
 
-	pc = &current->active_mm->context;
+	pc = &mm->context;

So this appears to be the function:

static void flush_ldt(void *current_mm)

 {
        struct mm_struct *mm = current_mm;
        mm_context_t *pc;

        if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
                return;

Quoted 5 lines

        pc = &mm->context;
        set_ldt(pc->ldt->entries, pc->ldt->size);

}

why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?

Because you cannot dereference a void pointer, i.e. &mm->context ....

Quoted 31 lines · Thomas Gleixner

Quoted 30 lines · Ingo Molnar

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Andy Lutomirski

/* context.lock is held for us, so we don't need any locking. */ static void flush_ldt(void *current_mm)
{

+       struct mm_struct *mm = current_mm;
        mm_context_t *pc;

Quoted 9 lines

arch/x86/kernel/ldt.c line 30

Quoted 2 lines

arch/x86/kernel/ldt.c +1 -1

-       if (current->active_mm != current_mm)
+       if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)

While functional correct, this really should compare against 'mm'.

Quoted 4 lines

                return;

arch/x86/kernel/ldt.c +1 -1

-       pc = &current->active_mm->context;
+       pc = &mm->context;

So this appears to be the function:

static void flush_ldt(void *current_mm)

 {
        struct mm_struct *mm = current_mm;
        mm_context_t *pc;

        if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
                return;

Quoted 7 lines

Quoted 5 lines

        pc = &mm->context;
        set_ldt(pc->ldt->entries, pc->ldt->size);

}

why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?

Because you cannot dereference a void pointer, i.e. &mm->context ....

Indeed, doh! The naming totally confused me. The way I'd write it is the canonical form for such callbacks:

        static void flush_ldt(void *data)
        {
                struct mm_struct *mm = data;

... which beyond unconfusing me would probably also have prevented any accidental use of the 'current_mm' callback argument.

Quoted 49 lines · Ingo Molnar

Quoted 31 lines · Thomas Gleixner

Quoted 30 lines · Ingo Molnar

Quoted 7 lines · Thomas Gleixner

Quoted 6 lines · Andy Lutomirski

/* context.lock is held for us, so we don't need any locking. */ static void flush_ldt(void *current_mm)
{

+       struct mm_struct *mm = current_mm;
        mm_context_t *pc;

Quoted 9 lines

arch/x86/kernel/ldt.c line 30

Quoted 2 lines

arch/x86/kernel/ldt.c +1 -1

-       if (current->active_mm != current_mm)
+       if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)

While functional correct, this really should compare against 'mm'.

Quoted 4 lines

                return;

arch/x86/kernel/ldt.c +1 -1

-       pc = &current->active_mm->context;
+       pc = &mm->context;

So this appears to be the function:

static void flush_ldt(void *current_mm)

 {
        struct mm_struct *mm = current_mm;
        mm_context_t *pc;

        if (this_cpu_read(cpu_tlbstate.loaded_mm) != current_mm)
                return;

Quoted 7 lines

Quoted 5 lines

        pc = &mm->context;
        set_ldt(pc->ldt->entries, pc->ldt->size);

}

why not rename 'current_mm' to 'mm' and remove the 'mm' local variable?

Because you cannot dereference a void pointer, i.e. &mm->context ....

Indeed, doh! The naming totally confused me. The way I'd write it is the canonical form for such callbacks:

        static void flush_ldt(void *data)
        {
                struct mm_struct *mm = data;

Quoted 4 lines

... which beyond unconfusing me would probably also have prevented any accidental use of the 'current_mm' callback argument.

void *data and void *info both seem fairly common in the kernel.

How about my personal favorite for non-kernel work, though: void *mm_void? It documents what the parameter means and avoids the confusion.

My personal favorite is double underscores prefix, i.e. 'void *__mm', which would clearly signal that this is something special. But this does not appear to have been picked up overly widely:

As I've been working on polishing my PCID code, a major problem I've encountered is that there are too many x86 TLB flushing code paths and that they have too many inconsequential differences. The result was that earlier versions of the PCID code were a colossal mess and very difficult to understand.

This series goes a long way toward cleaning up the mess. With all the patches applied, there is a single function that contains the meat of the code to flush the TLB on a given CPU, and all the tlb flushing APIs call it for both local and remote CPUs.

This series should only adversely affect the kernel in a couple of minor ways:

- It makes smp_mb() unconditional when flushing TLBs. We used to use the TLB flush itself to mostly avoid smp_mb() on the initiating CPU.

- On UP kernels, we lose the dubious optimization of inlining nerfed variants of all the TLB flush APIs. This bloats the kernel a tiny bit, although it should increase performance, since the SMP versions were better.

Patch 10 in here is a little bit off topic. It's a cleanup that's also needed before PCID can go in, but it's not directly about TLB flushing.

Thoughts?

This series goes a long way toward cleaning up the mess. With all the patches applied, there is a single function that contains the meat of the code to flush the TLB on a given CPU, and all the tlb flushing APIs call it for both local and remote CPUs.

As I've been working on polishing my PCID code, a major problem I've encountered is that there are too many x86 TLB flushing code paths and that they have too many inconsequential differences. The result was that earlier versions of the PCID code were a colossal mess and very difficult to understand.

This series goes a long way toward cleaning up the mess. With all the patches applied, there is a single function that contains the meat of the code to flush the TLB on a given CPU, and all the tlb flushing APIs call it for both local and remote CPUs.

This series should only adversely affect the kernel in a couple of minor ways:

- It makes smp_mb() unconditional when flushing TLBs. We used to use the TLB flush itself to mostly avoid smp_mb() on the initiating CPU.

- On UP kernels, we lose the dubious optimization of inlining nerfed variants of all the TLB flush APIs. This bloats the kernel a tiny bit, although it should increase performance, since the SMP versions were better.

Patch 10 in here is a little bit off topic. It's a cleanup that's also needed before PCID can go in, but it's not directly about TLB flushing.

Thoughts?

Quoted 26 lines · Andy Lutomirski

As I've been working on polishing my PCID code, a major problem I've encountered is that there are too many x86 TLB flushing code paths and that they have too many inconsequential differences. The result was that earlier versions of the PCID code were a colossal mess and very difficult to understand.

This series goes a long way toward cleaning up the mess. With all the patches applied, there is a single function that contains the meat of the code to flush the TLB on a given CPU, and all the tlb flushing APIs call it for both local and remote CPUs.

This series should only adversely affect the kernel in a couple of minor ways:

- It makes smp_mb() unconditional when flushing TLBs. We used to use the TLB flush itself to mostly avoid smp_mb() on the initiating CPU.

- On UP kernels, we lose the dubious optimization of inlining nerfed variants of all the TLB flush APIs. This bloats the kernel a tiny bit, although it should increase performance, since the SMP versions were better.

Quoted 5 lines

Patch 10 in here is a little bit off topic. It's a cleanup that's also needed before PCID can go in, but it's not directly about TLB flushing.

Thoughts?

In general I like the changes. I needed to hack Linux TLB shootdowns for a research project just because I could not handle the code otherwise. I ended up doing some of changes that you have done.

I just have two general comments:

- You may want to consider merging the kernel mappings invalidation with the userspace mappings invalidations as well, since there are still code redundancies.

- Don’t expect too much from concurrent TLB invalidations. In many cases the IPI latency dominates the overhead from my experience.