Currently the tlb_remove_table_smp_sync IPI is sent to all CPUs
indiscriminately, this causes unnecessary work and delays notable in
real-time use-cases and isolated cpus, this patch will limit this IPI to
only be sent to cpus referencing the effected mm and are currently in
kernel space.
Signed-off-by: Yair Podemsky <[email protected]>
Suggested-by: David Hildenbrand <[email protected]>
---
include/asm-generic/tlb.h | 4 ++--
mm/khugepaged.c | 4 ++--
mm/mmu_gather.c | 20 +++++++++++++++-----
3 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/include/asm-generic/tlb.h b/include/asm-generic/tlb.h
index b46617207c93..0b6ba17cc8d3 100644
--- a/include/asm-generic/tlb.h
+++ b/include/asm-generic/tlb.h
@@ -222,7 +222,7 @@ extern void tlb_remove_table(struct mmu_gather *tlb, void *table);
#define tlb_needs_table_invalidate() (true)
#endif
-void tlb_remove_table_sync_one(void);
+void tlb_remove_table_sync_one(struct mm_struct *mm);
#else
@@ -230,7 +230,7 @@ void tlb_remove_table_sync_one(void);
#error tlb_needs_table_invalidate() requires MMU_GATHER_RCU_TABLE_FREE
#endif
-static inline void tlb_remove_table_sync_one(void) { }
+static inline void tlb_remove_table_sync_one(struct mm_struct *mm) { }
#endif /* CONFIG_MMU_GATHER_RCU_TABLE_FREE */
diff --git a/mm/khugepaged.c b/mm/khugepaged.c
index 5cb401aa2b9d..86a82c0ac41f 100644
--- a/mm/khugepaged.c
+++ b/mm/khugepaged.c
@@ -1051,7 +1051,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address,
_pmd = pmdp_collapse_flush(vma, address, pmd);
spin_unlock(pmd_ptl);
mmu_notifier_invalidate_range_end(&range);
- tlb_remove_table_sync_one();
+ tlb_remove_table_sync_one(mm);
spin_lock(pte_ptl);
result = __collapse_huge_page_isolate(vma, address, pte, cc,
@@ -1408,7 +1408,7 @@ static void collapse_and_free_pmd(struct mm_struct *mm, struct vm_area_struct *v
addr + HPAGE_PMD_SIZE);
mmu_notifier_invalidate_range_start(&range);
pmd = pmdp_collapse_flush(vma, addr, pmdp);
- tlb_remove_table_sync_one();
+ tlb_remove_table_sync_one(mm);
mmu_notifier_invalidate_range_end(&range);
mm_dec_nr_ptes(mm);
page_table_check_pte_clear_range(mm, addr, pmd);
diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
index 2b93cf6ac9ae..3b267600a5e9 100644
--- a/mm/mmu_gather.c
+++ b/mm/mmu_gather.c
@@ -9,6 +9,7 @@
#include <linux/smp.h>
#include <linux/swap.h>
#include <linux/rmap.h>
+#include <linux/context_tracking_state.h>
#include <asm/pgalloc.h>
#include <asm/tlb.h>
@@ -191,7 +192,15 @@ static void tlb_remove_table_smp_sync(void *arg)
/* Simply deliver the interrupt */
}
-void tlb_remove_table_sync_one(void)
+static bool cpu_in_kernel(int cpu, void *info)
+{
+ struct context_tracking *ct = per_cpu_ptr(&context_tracking, cpu);
+ int statue = atomic_read(&ct->state);
+ //will return true only for cpu's in kernel space
+ return !(statue & CT_STATE_MASK);
+}
+
+void tlb_remove_table_sync_one(struct mm_struct *mm)
{
/*
* This isn't an RCU grace period and hence the page-tables cannot be
@@ -200,7 +209,8 @@ void tlb_remove_table_sync_one(void)
* It is however sufficient for software page-table walkers that rely on
* IRQ disabling.
*/
- smp_call_function(tlb_remove_table_smp_sync, NULL, 1);
+ on_each_cpu_cond_mask(cpu_in_kernel, tlb_remove_table_smp_sync,
+ NULL, true, mm_cpumask(mm));
}
static void tlb_remove_table_rcu(struct rcu_head *head)
@@ -237,9 +247,9 @@ static inline void tlb_table_invalidate(struct mmu_gather *tlb)
}
}
-static void tlb_remove_table_one(void *table)
+static void tlb_remove_table_one(struct mm_struct *mm, void *table)
{
- tlb_remove_table_sync_one();
+ tlb_remove_table_sync_one(mm);
__tlb_remove_table(table);
}
@@ -262,7 +272,7 @@ void tlb_remove_table(struct mmu_gather *tlb, void *table)
*batch = (struct mmu_table_batch *)__get_free_page(GFP_NOWAIT | __GFP_NOWARN);
if (*batch == NULL) {
tlb_table_invalidate(tlb);
- tlb_remove_table_one(table);
+ tlb_remove_table_one(tlb->mm, table);
return;
}
(*batch)->nr = 0;
--
2.31.1
On Sun, 12 Mar 2023 10:09:45 +0200 Yair Podemsky <[email protected]> wrote:
> Currently the tlb_remove_table_smp_sync IPI is sent to all CPUs
> indiscriminately, this causes unnecessary work and delays notable in
> real-time use-cases and isolated cpus, this patch will limit this IPI to
> only be sent to cpus referencing the effected mm and are currently in
> kernel space.
>
> ...
>
> --- a/mm/mmu_gather.c
> +++ b/mm/mmu_gather.c
> @@ -191,7 +192,15 @@ static void tlb_remove_table_smp_sync(void *arg)
> /* Simply deliver the interrupt */
> }
>
> -void tlb_remove_table_sync_one(void)
> +static bool cpu_in_kernel(int cpu, void *info)
> +{
> + struct context_tracking *ct = per_cpu_ptr(&context_tracking, cpu);
> + int statue = atomic_read(&ct->state);
Strange identifier. Should be "state"?
> + //will return true only for cpu's in kernel space
Please use /* */ style comments
And use "cpus" rather than "cpu's" - plural, not possessive.
> + return !(statue & CT_STATE_MASK);
Using
return state & CT_STATE_MASK == CONTEXT_KERNEL;
would more clearly express the intent.
> +}
And... surely this function is racy. ct->state can change value one
nanosecond after cpu_in_kernel() reads it, so cpu_in_kernel()'s return
value is now wrong?
On Sun, Mar 12, 2023 at 10:09:45AM +0200, Yair Podemsky wrote:
> Currently the tlb_remove_table_smp_sync IPI is sent to all CPUs
> indiscriminately, this causes unnecessary work and delays notable in
> real-time use-cases and isolated cpus, this patch will limit this IPI to
> only be sent to cpus referencing the effected mm and are currently in
> kernel space.
Did you validate that all architectures for which this is relevant
actually set bits in mm_cpumask() ?
On Mon, 2023-03-20 at 09:49 +0100, Peter Zijlstra wrote:
> On Sun, Mar 12, 2023 at 10:09:45AM +0200, Yair Podemsky wrote:
> > Currently the tlb_remove_table_smp_sync IPI is sent to all CPUs
> > indiscriminately, this causes unnecessary work and delays notable
> > in
> > real-time use-cases and isolated cpus, this patch will limit this
> > IPI to
> > only be sent to cpus referencing the effected mm and are currently
> > in
> > kernel space.
>
> Did you validate that all architectures for which this is relevant
> actually set bits in mm_cpumask() ?
>
Hi Peter,
Thank you for bringing this to my attention.
I reviewed the architectures using the MMU_GATHER_RCU_TABLE_FREE:
arm, powerpc, s390, sparc and x86 set the bit when switching process
in.
for arm64 removed set/clear bit in 38d96287504a ("arm64: mm: kill
mm_cpumask usage")
The reason given was that mm_cpumask was not used.
Given that we now have a use for it, I will add a patch to revert.
Thanks
Yair
On Wed, Mar 22, 2023 at 04:11:44PM +0200, [email protected] wrote:
> On Mon, 2023-03-20 at 09:49 +0100, Peter Zijlstra wrote:
> > On Sun, Mar 12, 2023 at 10:09:45AM +0200, Yair Podemsky wrote:
> > > Currently the tlb_remove_table_smp_sync IPI is sent to all CPUs
> > > indiscriminately, this causes unnecessary work and delays notable
> > > in
> > > real-time use-cases and isolated cpus, this patch will limit this
> > > IPI to
> > > only be sent to cpus referencing the effected mm and are currently
> > > in
> > > kernel space.
> >
> > Did you validate that all architectures for which this is relevant
> > actually set bits in mm_cpumask() ?
> >
> Hi Peter,
> Thank you for bringing this to my attention.
> I reviewed the architectures using the MMU_GATHER_RCU_TABLE_FREE:
> arm, powerpc, s390, sparc and x86 set the bit when switching process
> in.
> for arm64 removed set/clear bit in 38d96287504a ("arm64: mm: kill
> mm_cpumask usage")
> The reason given was that mm_cpumask was not used.
> Given that we now have a use for it, I will add a patch to revert.
Maintaining the mask is also not free, so I'm not keen on adding it back
unless there's a net win.
Will
On Fri, 2023-03-24 at 15:13 +0000, Will Deacon wrote:
> On Wed, Mar 22, 2023 at 04:11:44PM +0200, [email protected] wrote:
> > On Mon, 2023-03-20 at 09:49 +0100, Peter Zijlstra wrote:
> > > On Sun, Mar 12, 2023 at 10:09:45AM +0200, Yair Podemsky wrote:
> > > > Currently the tlb_remove_table_smp_sync IPI is sent to all CPUs
> > > > indiscriminately, this causes unnecessary work and delays
> > > > notable
> > > > in
> > > > real-time use-cases and isolated cpus, this patch will limit
> > > > this
> > > > IPI to
> > > > only be sent to cpus referencing the effected mm and are
> > > > currently
> > > > in
> > > > kernel space.
> > >
> > > Did you validate that all architectures for which this is
> > > relevant
> > > actually set bits in mm_cpumask() ?
> > >
> > Hi Peter,
> > Thank you for bringing this to my attention.
> > I reviewed the architectures using the MMU_GATHER_RCU_TABLE_FREE:
> > arm, powerpc, s390, sparc and x86 set the bit when switching
> > process
> > in.
> > for arm64 removed set/clear bit in 38d96287504a ("arm64: mm: kill
> > mm_cpumask usage")
> > The reason given was that mm_cpumask was not used.
> > Given that we now have a use for it, I will add a patch to revert.
>
> Maintaining the mask is also not free, so I'm not keen on adding it
> back
> unless there's a net win.
>
> Will
>
How about adding a Kconfig to mark which architectures set/use the
mm_cpumask?
This will allow us to use the mm_cpumask on architectures that support
it, and use acpu_online_mask on those that don't.
Also make it clear which architectures set the bit for the future.
Yair