[RFC] mm/tlbbatch: Introduce arch_tlbbatch_should_defer()

[RFC] mm/tlbbatch: Introduce arch_tlbbatch_should_defer()

[Anshuman Khandual]

The entire scheme of deferred TLB flush in reclaim path rests on the
fact that the cost to refill TLB entries is less than flushing out
individual entries by sending IPI to remote CPUs. But architecture
can have different ways to evaluate that. Hence apart from checking
TTU_BATCH_FLUSH in the TTU flags, rest of the decision should be
architecture specific.

Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
- Thought about this just by code inspection.

 arch/x86/include/asm/tlbflush.h | 12 ++++++++++++
 mm/rmap.c                       |  9 +--------
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/arch/x86/include/asm/tlbflush.h b/arch/x86/include/asm/tlbflush.h
index 50ea348..a636a69 100644
--- a/arch/x86/include/asm/tlbflush.h
+++ b/arch/x86/include/asm/tlbflush.h
@@ -259,6 +259,18 @@ void native_flush_tlb_others(const struct cpumask *cpumask,
 #define TLBSTATE_OK	1
 #define TLBSTATE_LAZY	2
 
+static inline void arch_tlbbatch_should_defer(struct mm_struct *mm)
+{
+	bool should_defer = false;
+
+	/* If remote CPUs need to be flushed then defer batch the flush */
+	if (cpumask_any_but(mm_cpumask(mm), get_cpu()) < nr_cpu_ids)
+		should_defer = true;
+	put_cpu();
+
+	return should_defer;
+}
+
 static inline void arch_tlbbatch_add_mm(struct arch_tlbflush_unmap_batch *batch,
 					struct mm_struct *mm)
 {
diff --git a/mm/rmap.c b/mm/rmap.c
index c570f82..e350380 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -626,17 +626,10 @@ static void set_tlb_ubc_flush_pending(struct mm_struct *mm, bool writable)
  */
 static bool should_defer_flush(struct mm_struct *mm, enum ttu_flags flags)
 {
-	bool should_defer = false;
-
 	if (!(flags & TTU_BATCH_FLUSH))
 		return false;
 
-	/* If remote CPUs need to be flushed then defer batch the flush */
-	if (cpumask_any_but(mm_cpumask(mm), get_cpu()) < nr_cpu_ids)
-		should_defer = true;
-	put_cpu();
-
-	return should_defer;
+	return arch_tlbbatch_should_defer(mm);
 }
 
 /*
-- 
1.8.5.2

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/tlbbatch: Introduce arch_tlbbatch_should_defer()

[Mel Gorman]

On Tue, Sep 05, 2017 at 08:15:40PM +0530, Anshuman Khandual wrote:
> The entire scheme of deferred TLB flush in reclaim path rests on the
> fact that the cost to refill TLB entries is less than flushing out
> individual entries by sending IPI to remote CPUs. But architecture
> can have different ways to evaluate that. Hence apart from checking
> TTU_BATCH_FLUSH in the TTU flags, rest of the decision should be
> architecture specific.
> 
> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

There is only one arch implementation given and if an arch knows that
the flush should not be deferred then why would it implement support in
the first place? I'm struggling to see the point of the patch.

-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/tlbbatch: Introduce arch_tlbbatch_should_defer()

[Anshuman Khandual]

On 09/05/2017 09:20 PM, Mel Gorman wrote:
> On Tue, Sep 05, 2017 at 08:15:40PM +0530, Anshuman Khandual wrote:
>> The entire scheme of deferred TLB flush in reclaim path rests on the
>> fact that the cost to refill TLB entries is less than flushing out
>> individual entries by sending IPI to remote CPUs. But architecture
>> can have different ways to evaluate that. Hence apart from checking
>> TTU_BATCH_FLUSH in the TTU flags, rest of the decision should be
>> architecture specific.
>>
>> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> 
> There is only one arch implementation given and if an arch knows that
> the flush should not be deferred then why would it implement support in
> the first place? I'm struggling to see the point of the patch.

Even if the arch supports deferring of TLB flush like in the existing
case, it still checks if mm_cpumask(mm) contains anything other than
the current CPU (which indicates need for an IPI for a TLB flush) to
decide whether the TLB batch flush should be deferred or not. The
point is some architectures might do something different for a given
struct mm other than checking for presence of remote CPU in the mask
mm_cpumask(mm). It might be specific to the situation, struct mm etc.
Hence arch callback should be used instead.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [RFC] mm/tlbbatch: Introduce arch_tlbbatch_should_defer()

[Mel Gorman]

On Wed, Sep 06, 2017 at 09:23:49AM +0530, Anshuman Khandual wrote:
> On 09/05/2017 09:20 PM, Mel Gorman wrote:
> > On Tue, Sep 05, 2017 at 08:15:40PM +0530, Anshuman Khandual wrote:
> >> The entire scheme of deferred TLB flush in reclaim path rests on the
> >> fact that the cost to refill TLB entries is less than flushing out
> >> individual entries by sending IPI to remote CPUs. But architecture
> >> can have different ways to evaluate that. Hence apart from checking
> >> TTU_BATCH_FLUSH in the TTU flags, rest of the decision should be
> >> architecture specific.
> >>
> >> Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> > 
> > There is only one arch implementation given and if an arch knows that
> > the flush should not be deferred then why would it implement support in
> > the first place? I'm struggling to see the point of the patch.
> 
> Even if the arch supports deferring of TLB flush like in the existing
> case, it still checks if mm_cpumask(mm) contains anything other than
> the current CPU (which indicates need for an IPI for a TLB flush) to
> decide whether the TLB batch flush should be deferred or not. The
> point is some architectures might do something different for a given
> struct mm other than checking for presence of remote CPU in the mask
> mm_cpumask(mm). It might be specific to the situation, struct mm etc.
> Hence arch callback should be used instead.
> 

If that turns out to be the case then the arch can create the hook at the
same time. RIght now, this is churn.


-- 
Mel Gorman
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>