linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
@ 2013-05-29 12:56 Vineet Gupta
  2013-05-29 14:03 ` Catalin Marinas
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Vineet Gupta @ 2013-05-29 12:56 UTC (permalink / raw)
  To: linux-mm, linux-kernel
  Cc: Vineet Gupta, Andrew Morton, Mel Gorman, Hugh Dickins,
	Rik van Riel, David Rientjes, Peter Zijlstra,
	linux-arch@vger.kernel.org, Catalin Marinas, Max Filippov

zap_pte_range loops from @addr to @end. In the middle, if it runs out of
batching slots, TLB entries needs to be flushed for @start to @interim,
NOT @interim to @end.

Since ARC port doesn't use page free batching I can't test it myself but
this seems like the right thing to do.
Observed this when working on a fix for the issue at thread:
	http://www.spinics.net/lists/linux-arch/msg21736.html

Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Mel Gorman <mgorman@suse.de>
Cc: Hugh Dickins <hughd@google.com>
Cc: Rik van Riel <riel@redhat.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: linux-mm@kvack.org
Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Max Filippov <jcmvbkbc@gmail.com>
---
 mm/memory.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 6dc1882..d9d5fd9 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
 	spinlock_t *ptl;
 	pte_t *start_pte;
 	pte_t *pte;
+	unsigned long range_start = addr;
 
 again:
 	init_rss_vec(rss);
@@ -1215,12 +1216,14 @@ again:
 		force_flush = 0;
 
 #ifdef HAVE_GENERIC_MMU_GATHER
-		tlb->start = addr;
-		tlb->end = end;
+		tlb->start = range_start;
+		tlb->end = addr;
 #endif
 		tlb_flush_mmu(tlb);
-		if (addr != end)
+		if (addr != end) {
+			range_start = addr;
 			goto again;
+		}
 	}
 
 	return addr;
-- 
1.7.10.4

--
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>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 12:56 [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots Vineet Gupta
@ 2013-05-29 14:03 ` Catalin Marinas
  2013-05-29 14:08   ` Vineet Gupta
  2013-05-30  5:02 ` Vineet Gupta
  2013-07-29 23:41 ` David Miller
  2 siblings, 1 reply; 10+ messages in thread
From: Catalin Marinas @ 2013-05-29 14:03 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton,
	Mel Gorman, Hugh Dickins, Rik van Riel, David Rientjes,
	Peter Zijlstra, linux-arch@vger.kernel.org, Max Filippov

On Wed, May 29, 2013 at 01:56:13PM +0100, Vineet Gupta wrote:
> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> batching slots, TLB entries needs to be flushed for @start to @interim,
> NOT @interim to @end.
> 
> Since ARC port doesn't use page free batching I can't test it myself but
> this seems like the right thing to do.
> Observed this when working on a fix for the issue at thread:
> 	http://www.spinics.net/lists/linux-arch/msg21736.html
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-mm@kvack.org
> Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  mm/memory.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 6dc1882..d9d5fd9 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>  	spinlock_t *ptl;
>  	pte_t *start_pte;
>  	pte_t *pte;
> +	unsigned long range_start = addr;
>  
>  again:
>  	init_rss_vec(rss);
> @@ -1215,12 +1216,14 @@ again:
>  		force_flush = 0;
>  
>  #ifdef HAVE_GENERIC_MMU_GATHER
> -		tlb->start = addr;
> -		tlb->end = end;
> +		tlb->start = range_start;
> +		tlb->end = addr;
>  #endif
>  		tlb_flush_mmu(tlb);
> -		if (addr != end)
> +		if (addr != end) {
> +			range_start = addr;
>  			goto again;
> +		}
>  	}

Isn't this code only run if force_flush != 0? force_flush is set to
!__tlb_remove_page() and this function always returns 1 on (generic TLB)
UP since tlb_fast_mode() is 1. There is no batching on UP with the
generic TLB code.

-- 
Catalin

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 14:03 ` Catalin Marinas
@ 2013-05-29 14:08   ` Vineet Gupta
  2013-05-29 14:29     ` Catalin Marinas
  0 siblings, 1 reply; 10+ messages in thread
From: Vineet Gupta @ 2013-05-29 14:08 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: Vineet Gupta, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	Andrew Morton, Mel Gorman, Hugh Dickins, Rik van Riel,
	David Rientjes, Peter Zijlstra, linux-arch@vger.kernel.org,
	Max Filippov

On 05/29/2013 07:33 PM, Catalin Marinas wrote:
> On Wed, May 29, 2013 at 01:56:13PM +0100, Vineet Gupta wrote:
>> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
>> batching slots, TLB entries needs to be flushed for @start to @interim,
>> NOT @interim to @end.
>>
>> Since ARC port doesn't use page free batching I can't test it myself but
>> this seems like the right thing to do.
>> Observed this when working on a fix for the issue at thread:
>> 	http://www.spinics.net/lists/linux-arch/msg21736.html
>>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: Rik van Riel <riel@redhat.com>
>> Cc: David Rientjes <rientjes@google.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: linux-mm@kvack.org
>> Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Max Filippov <jcmvbkbc@gmail.com>
>> ---
>>  mm/memory.c |    9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index 6dc1882..d9d5fd9 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>>  	spinlock_t *ptl;
>>  	pte_t *start_pte;
>>  	pte_t *pte;
>> +	unsigned long range_start = addr;
>>  
>>  again:
>>  	init_rss_vec(rss);
>> @@ -1215,12 +1216,14 @@ again:
>>  		force_flush = 0;
>>  
>>  #ifdef HAVE_GENERIC_MMU_GATHER
>> -		tlb->start = addr;
>> -		tlb->end = end;
>> +		tlb->start = range_start;
>> +		tlb->end = addr;
>>  #endif
>>  		tlb_flush_mmu(tlb);
>> -		if (addr != end)
>> +		if (addr != end) {
>> +			range_start = addr;
>>  			goto again;
>> +		}
>>  	}
> Isn't this code only run if force_flush != 0? force_flush is set to
> !__tlb_remove_page() and this function always returns 1 on (generic TLB)
> UP since tlb_fast_mode() is 1. There is no batching on UP with the
> generic TLB code.

Correct ! That's why the changelog says I couldn't test it on ARC port itself :-)

However based on the other discussion (Max's TLB/PTE inconsistency), as I started
writing code to reuse this block to flush the TLB even for non forced case, I
realized that what this is doing is incorrect and won't work for the general flushing.

Ignoring all other threads, do we agree that the exiting code - if used in any
situations is incorrect semantically ?

-Vineet

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 14:08   ` Vineet Gupta
@ 2013-05-29 14:29     ` Catalin Marinas
  2013-05-29 14:36       ` Vineet Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Catalin Marinas @ 2013-05-29 14:29 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton,
	Mel Gorman, Hugh Dickins, Rik van Riel, David Rientjes,
	Peter Zijlstra, linux-arch@vger.kernel.org, Max Filippov

On Wed, May 29, 2013 at 03:08:37PM +0100, Vineet Gupta wrote:
> On 05/29/2013 07:33 PM, Catalin Marinas wrote:
> > On Wed, May 29, 2013 at 01:56:13PM +0100, Vineet Gupta wrote:
> >> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> >> batching slots, TLB entries needs to be flushed for @start to @interim,
> >> NOT @interim to @end.
> >>
> >> Since ARC port doesn't use page free batching I can't test it myself but
> >> this seems like the right thing to do.
> >> Observed this when working on a fix for the issue at thread:
> >> 	http://www.spinics.net/lists/linux-arch/msg21736.html
> >>
> >> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> >> Cc: Andrew Morton <akpm@linux-foundation.org>
> >> Cc: Mel Gorman <mgorman@suse.de>
> >> Cc: Hugh Dickins <hughd@google.com>
> >> Cc: Rik van Riel <riel@redhat.com>
> >> Cc: David Rientjes <rientjes@google.com>
> >> Cc: Peter Zijlstra <peterz@infradead.org>
> >> Cc: linux-mm@kvack.org
> >> Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Max Filippov <jcmvbkbc@gmail.com>
> >> ---
> >>  mm/memory.c |    9 ++++++---
> >>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/mm/memory.c b/mm/memory.c
> >> index 6dc1882..d9d5fd9 100644
> >> --- a/mm/memory.c
> >> +++ b/mm/memory.c
> >> @@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
> >>  	spinlock_t *ptl;
> >>  	pte_t *start_pte;
> >>  	pte_t *pte;
> >> +	unsigned long range_start = addr;
> >>  
> >>  again:
> >>  	init_rss_vec(rss);
> >> @@ -1215,12 +1216,14 @@ again:
> >>  		force_flush = 0;
> >>  
> >>  #ifdef HAVE_GENERIC_MMU_GATHER
> >> -		tlb->start = addr;
> >> -		tlb->end = end;
> >> +		tlb->start = range_start;
> >> +		tlb->end = addr;
> >>  #endif
> >>  		tlb_flush_mmu(tlb);
> >> -		if (addr != end)
> >> +		if (addr != end) {
> >> +			range_start = addr;
> >>  			goto again;
> >> +		}
> >>  	}
> > Isn't this code only run if force_flush != 0? force_flush is set to
> > !__tlb_remove_page() and this function always returns 1 on (generic TLB)
> > UP since tlb_fast_mode() is 1. There is no batching on UP with the
> > generic TLB code.
> 
> Correct ! That's why the changelog says I couldn't test it on ARC port itself :-)
> 
> However based on the other discussion (Max's TLB/PTE inconsistency), as I started
> writing code to reuse this block to flush the TLB even for non forced case, I
> realized that what this is doing is incorrect and won't work for the general flushing.

An alternative would be to make sure the above block is always called
when tlb_fast_mode():

diff --git a/mm/memory.c b/mm/memory.c
index 6dc1882..f8b1f30 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1211,7 +1211,7 @@ again:
 	 * the PTE lock to avoid doing the potential expensive TLB invalidate
 	 * and page-free while holding it.
 	 */
-	if (force_flush) {
+	if (force_flush || tlb_fast_mode(tlb)) {
 		force_flush = 0;
 
 #ifdef HAVE_GENERIC_MMU_GATHER

> Ignoring all other threads, do we agree that the exiting code - if used in any
> situations is incorrect semantically ?

It is incorrect unless there are requirements for
arch_leave_lazy_mmu_mode() to handle the TLB invalidation (it doesn't
look like it's widely implemented though).

-- 
Catalin

--
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>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 14:29     ` Catalin Marinas
@ 2013-05-29 14:36       ` Vineet Gupta
  2013-05-29 14:51         ` Catalin Marinas
  0 siblings, 1 reply; 10+ messages in thread
From: Vineet Gupta @ 2013-05-29 14:36 UTC (permalink / raw)
  To: Catalin Marinas
  Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton,
	Mel Gorman, Hugh Dickins, Rik van Riel, David Rientjes,
	Peter Zijlstra, linux-arch@vger.kernel.org, Max Filippov

On 05/29/2013 07:59 PM, Catalin Marinas wrote:
> On Wed, May 29, 2013 at 03:08:37PM +0100, Vineet Gupta wrote:
>> On 05/29/2013 07:33 PM, Catalin Marinas wrote:
>>> On Wed, May 29, 2013 at 01:56:13PM +0100, Vineet Gupta wrote:
>>>> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
>>>> batching slots, TLB entries needs to be flushed for @start to @interim,
>>>> NOT @interim to @end.
>>>>
>>>> Since ARC port doesn't use page free batching I can't test it myself but
>>>> this seems like the right thing to do.
>>>> Observed this when working on a fix for the issue at thread:
>>>> 	http://www.spinics.net/lists/linux-arch/msg21736.html
>>>>
>>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>>>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>>> Cc: Mel Gorman <mgorman@suse.de>
>>>> Cc: Hugh Dickins <hughd@google.com>
>>>> Cc: Rik van Riel <riel@redhat.com>
>>>> Cc: David Rientjes <rientjes@google.com>
>>>> Cc: Peter Zijlstra <peterz@infradead.org>
>>>> Cc: linux-mm@kvack.org
>>>> Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Max Filippov <jcmvbkbc@gmail.com>
>>>> ---
>>>>  mm/memory.c |    9 ++++++---
>>>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index 6dc1882..d9d5fd9 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>>>>  	spinlock_t *ptl;
>>>>  	pte_t *start_pte;
>>>>  	pte_t *pte;
>>>> +	unsigned long range_start = addr;
>>>>  
>>>>  again:
>>>>  	init_rss_vec(rss);
>>>> @@ -1215,12 +1216,14 @@ again:
>>>>  		force_flush = 0;
>>>>  
>>>>  #ifdef HAVE_GENERIC_MMU_GATHER
>>>> -		tlb->start = addr;
>>>> -		tlb->end = end;
>>>> +		tlb->start = range_start;
>>>> +		tlb->end = addr;
>>>>  #endif
>>>>  		tlb_flush_mmu(tlb);
>>>> -		if (addr != end)
>>>> +		if (addr != end) {
>>>> +			range_start = addr;
>>>>  			goto again;
>>>> +		}
>>>>  	}
>>> Isn't this code only run if force_flush != 0? force_flush is set to
>>> !__tlb_remove_page() and this function always returns 1 on (generic TLB)
>>> UP since tlb_fast_mode() is 1. There is no batching on UP with the
>>> generic TLB code.
>> Correct ! That's why the changelog says I couldn't test it on ARC port itself :-)
>>
>> However based on the other discussion (Max's TLB/PTE inconsistency), as I started
>> writing code to reuse this block to flush the TLB even for non forced case, I
>> realized that what this is doing is incorrect and won't work for the general flushing.
> An alternative would be to make sure the above block is always called
> when tlb_fast_mode():
>
> diff --git a/mm/memory.c b/mm/memory.c
> index 6dc1882..f8b1f30 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1211,7 +1211,7 @@ again:
>  	 * the PTE lock to avoid doing the potential expensive TLB invalidate
>  	 * and page-free while holding it.
>  	 */
> -	if (force_flush) {
> +	if (force_flush || tlb_fast_mode(tlb)) {
>  		force_flush = 0;

I agree with tlb_fast_mode() addition (to solve Max's issue). The problem however
is that when we hit this at the end of loop - @addr is already pointing to @end so
range flush gets start = end - not what we really intended.

>> Ignoring all other threads, do we agree that the exiting code - if used in any
>> situations is incorrect semantically ?
> It is incorrect unless there are requirements for
> arch_leave_lazy_mmu_mode() to handle the TLB invalidation (it doesn't
> look like it's widely implemented though).

This patch is preparatory - independent of Max's issue. It is fixing just the
forced flush case - whoever uses it right now (ofcourse UP + generic TLB doesn't).

Thx,
-Vineet

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 14:36       ` Vineet Gupta
@ 2013-05-29 14:51         ` Catalin Marinas
  0 siblings, 0 replies; 10+ messages in thread
From: Catalin Marinas @ 2013-05-29 14:51 UTC (permalink / raw)
  To: Vineet Gupta
  Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Andrew Morton,
	Mel Gorman, Hugh Dickins, Rik van Riel, David Rientjes,
	Peter Zijlstra, linux-arch@vger.kernel.org, Max Filippov

On Wed, May 29, 2013 at 03:36:02PM +0100, Vineet Gupta wrote:
> On 05/29/2013 07:59 PM, Catalin Marinas wrote:
> > On Wed, May 29, 2013 at 03:08:37PM +0100, Vineet Gupta wrote:
> >> On 05/29/2013 07:33 PM, Catalin Marinas wrote:
> >>> On Wed, May 29, 2013 at 01:56:13PM +0100, Vineet Gupta wrote:
> >>>> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> >>>> batching slots, TLB entries needs to be flushed for @start to @interim,
> >>>> NOT @interim to @end.
> >>>>
> >>>> Since ARC port doesn't use page free batching I can't test it myself but
> >>>> this seems like the right thing to do.
> >>>> Observed this when working on a fix for the issue at thread:
> >>>> 	http://www.spinics.net/lists/linux-arch/msg21736.html
> >>>>
> >>>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> >>>> Cc: Andrew Morton <akpm@linux-foundation.org>
> >>>> Cc: Mel Gorman <mgorman@suse.de>
> >>>> Cc: Hugh Dickins <hughd@google.com>
> >>>> Cc: Rik van Riel <riel@redhat.com>
> >>>> Cc: David Rientjes <rientjes@google.com>
> >>>> Cc: Peter Zijlstra <peterz@infradead.org>
> >>>> Cc: linux-mm@kvack.org
> >>>> Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: Max Filippov <jcmvbkbc@gmail.com>
> >>>> ---
> >>>>  mm/memory.c |    9 ++++++---
> >>>>  1 file changed, 6 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/mm/memory.c b/mm/memory.c
> >>>> index 6dc1882..d9d5fd9 100644
> >>>> --- a/mm/memory.c
> >>>> +++ b/mm/memory.c
> >>>> @@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
> >>>>  	spinlock_t *ptl;
> >>>>  	pte_t *start_pte;
> >>>>  	pte_t *pte;
> >>>> +	unsigned long range_start = addr;
> >>>>  
> >>>>  again:
> >>>>  	init_rss_vec(rss);
> >>>> @@ -1215,12 +1216,14 @@ again:
> >>>>  		force_flush = 0;
> >>>>  
> >>>>  #ifdef HAVE_GENERIC_MMU_GATHER
> >>>> -		tlb->start = addr;
> >>>> -		tlb->end = end;
> >>>> +		tlb->start = range_start;
> >>>> +		tlb->end = addr;
> >>>>  #endif
> >>>>  		tlb_flush_mmu(tlb);
> >>>> -		if (addr != end)
> >>>> +		if (addr != end) {
> >>>> +			range_start = addr;
> >>>>  			goto again;
> >>>> +		}
> >>>>  	}
> >>> Isn't this code only run if force_flush != 0? force_flush is set to
> >>> !__tlb_remove_page() and this function always returns 1 on (generic TLB)
> >>> UP since tlb_fast_mode() is 1. There is no batching on UP with the
> >>> generic TLB code.
> >> Correct ! That's why the changelog says I couldn't test it on ARC port itself :-)
> >>
> >> However based on the other discussion (Max's TLB/PTE inconsistency), as I started
> >> writing code to reuse this block to flush the TLB even for non forced case, I
> >> realized that what this is doing is incorrect and won't work for the general flushing.
> > An alternative would be to make sure the above block is always called
> > when tlb_fast_mode():
> >
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 6dc1882..f8b1f30 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -1211,7 +1211,7 @@ again:
> >  	 * the PTE lock to avoid doing the potential expensive TLB invalidate
> >  	 * and page-free while holding it.
> >  	 */
> > -	if (force_flush) {
> > +	if (force_flush || tlb_fast_mode(tlb)) {
> >  		force_flush = 0;
> 
> I agree with tlb_fast_mode() addition (to solve Max's issue). The problem however
> is that when we hit this at the end of loop - @addr is already pointing to @end so
> range flush gets start = end - not what we really intended.

OK. So for this part your patch looks fine.

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 12:56 [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots Vineet Gupta
  2013-05-29 14:03 ` Catalin Marinas
@ 2013-05-30  5:02 ` Vineet Gupta
  2013-07-29 23:41 ` David Miller
  2 siblings, 0 replies; 10+ messages in thread
From: Vineet Gupta @ 2013-05-30  5:02 UTC (permalink / raw)
  To: Alex Shi; +Cc: linux-mm@kvack.org, lkml, linux-arch@vger.kernel.org

[+alex.shi@intel.com]

On 05/29/2013 06:26 PM, Vineet Gupta wrote:
> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> batching slots, TLB entries needs to be flushed for @start to @interim,
> NOT @interim to @end.
> 
> Since ARC port doesn't use page free batching I can't test it myself but
> this seems like the right thing to do.
> Observed this when working on a fix for the issue at thread:
> 	http://www.spinics.net/lists/linux-arch/msg21736.html
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: Rik van Riel <riel@redhat.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: linux-mm@kvack.org
> Cc: linux-arch@vger.kernel.org <linux-arch@vger.kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  mm/memory.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 6dc1882..d9d5fd9 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1110,6 +1110,7 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb,
>  	spinlock_t *ptl;
>  	pte_t *start_pte;
>  	pte_t *pte;
> +	unsigned long range_start = addr;
>  
>  again:
>  	init_rss_vec(rss);
> @@ -1215,12 +1216,14 @@ again:
>  		force_flush = 0;
>  
>  #ifdef HAVE_GENERIC_MMU_GATHER
> -		tlb->start = addr;
> -		tlb->end = end;
> +		tlb->start = range_start;
> +		tlb->end = addr;
>  #endif
>  		tlb_flush_mmu(tlb);
> -		if (addr != end)
> +		if (addr != end) {
> +			range_start = addr;
>  			goto again;
> +		}
>  	}
>  
>  	return addr;
> 

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-05-29 12:56 [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots Vineet Gupta
  2013-05-29 14:03 ` Catalin Marinas
  2013-05-30  5:02 ` Vineet Gupta
@ 2013-07-29 23:41 ` David Miller
  2013-07-29 23:46   ` Andrew Morton
  2 siblings, 1 reply; 10+ messages in thread
From: David Miller @ 2013-07-29 23:41 UTC (permalink / raw)
  To: Vineet.Gupta1
  Cc: linux-mm, linux-kernel, akpm, mgorman, hughd, riel, rientjes,
	peterz, linux-arch, catalin.marinas, jcmvbkbc

From: Vineet Gupta <Vineet.Gupta1@synopsys.com>
Date: Wed, 29 May 2013 18:26:13 +0530

> zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> batching slots, TLB entries needs to be flushed for @start to @interim,
> NOT @interim to @end.
> 
> Since ARC port doesn't use page free batching I can't test it myself but
> this seems like the right thing to do.
> Observed this when working on a fix for the issue at thread:
> 	http://www.spinics.net/lists/linux-arch/msg21736.html
> 
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>

As this bug can cause pretty serious memory corruption, I'd like to
see this submitted to -stable.

Thanks!

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-07-29 23:41 ` David Miller
@ 2013-07-29 23:46   ` Andrew Morton
  2013-07-30  0:16     ` Greg KH
  0 siblings, 1 reply; 10+ messages in thread
From: Andrew Morton @ 2013-07-29 23:46 UTC (permalink / raw)
  To: David Miller
  Cc: Vineet.Gupta1, linux-mm, linux-kernel, mgorman, hughd, riel,
	rientjes, peterz, linux-arch, catalin.marinas, jcmvbkbc, Greg KH,
	stable

On Mon, 29 Jul 2013 16:41:06 -0700 (PDT) David Miller <davem@davemloft.net> wrote:

> From: Vineet Gupta <Vineet.Gupta1@synopsys.com>
> Date: Wed, 29 May 2013 18:26:13 +0530
> 
> > zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> > batching slots, TLB entries needs to be flushed for @start to @interim,
> > NOT @interim to @end.
> > 
> > Since ARC port doesn't use page free batching I can't test it myself but
> > this seems like the right thing to do.
> > Observed this when working on a fix for the issue at thread:
> > 	http://www.spinics.net/lists/linux-arch/msg21736.html
> > 
> > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> 
> As this bug can cause pretty serious memory corruption, I'd like to
> see this submitted to -stable.

Greg, e6c495a96ce02574e765d5140039a64c8d4e8c9e from mainline, please.

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots
  2013-07-29 23:46   ` Andrew Morton
@ 2013-07-30  0:16     ` Greg KH
  0 siblings, 0 replies; 10+ messages in thread
From: Greg KH @ 2013-07-30  0:16 UTC (permalink / raw)
  To: Andrew Morton
  Cc: David Miller, Vineet.Gupta1, linux-mm, linux-kernel, mgorman,
	hughd, riel, rientjes, peterz, linux-arch, catalin.marinas,
	jcmvbkbc, stable

On Mon, Jul 29, 2013 at 04:46:58PM -0700, Andrew Morton wrote:
> On Mon, 29 Jul 2013 16:41:06 -0700 (PDT) David Miller <davem@davemloft.net> wrote:
> 
> > From: Vineet Gupta <Vineet.Gupta1@synopsys.com>
> > Date: Wed, 29 May 2013 18:26:13 +0530
> > 
> > > zap_pte_range loops from @addr to @end. In the middle, if it runs out of
> > > batching slots, TLB entries needs to be flushed for @start to @interim,
> > > NOT @interim to @end.
> > > 
> > > Since ARC port doesn't use page free batching I can't test it myself but
> > > this seems like the right thing to do.
> > > Observed this when working on a fix for the issue at thread:
> > > 	http://www.spinics.net/lists/linux-arch/msg21736.html
> > > 
> > > Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> > 
> > As this bug can cause pretty serious memory corruption, I'd like to
> > see this submitted to -stable.
> 
> Greg, e6c495a96ce02574e765d5140039a64c8d4e8c9e from mainline, please.

Now applied to 3.10-stable, thanks.

greg k-h

--
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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2013-07-30  0:16 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-29 12:56 [PATCH] mm: Fix the TLB range flushed when __tlb_remove_page() runs out of slots Vineet Gupta
2013-05-29 14:03 ` Catalin Marinas
2013-05-29 14:08   ` Vineet Gupta
2013-05-29 14:29     ` Catalin Marinas
2013-05-29 14:36       ` Vineet Gupta
2013-05-29 14:51         ` Catalin Marinas
2013-05-30  5:02 ` Vineet Gupta
2013-07-29 23:41 ` David Miller
2013-07-29 23:46   ` Andrew Morton
2013-07-30  0:16     ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).