linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] Cache flush fixes
@ 2013-12-13 12:06 Steve Capper
  2013-12-13 12:06 ` [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages Steve Capper
  2013-12-13 12:06 ` [PATCH 2/2] arm: cacheflush: Fix user split-caching logic Steve Capper
  0 siblings, 2 replies; 7+ messages in thread
From: Steve Capper @ 2013-12-13 12:06 UTC (permalink / raw)
  To: linux-arm-kernel

This patch set fixes a couple of cache flushing problems that I ran
into recently, whilst running libhugetlbfs tests (mainly the
icache-hygiene test).

One problem was due to not flushing enough, the other was due to
flushing too much. Hopefully, this patch set makes things jussst
right :-).

I am not sure whether or not these should be marked as for stable?

Cheers,
-- 
Steve

Steve Capper (2):
  arm: mm: fix dcache flush logic for compound high pages
  arm: cacheflush: Fix user split-caching logic

 arch/arm/kernel/traps.c |  2 +-
 arch/arm/mm/flush.c     | 11 ++++++-----
 2 files changed, 7 insertions(+), 6 deletions(-)

-- 
1.8.1.4

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

* [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages
  2013-12-13 12:06 [PATCH 0/2] Cache flush fixes Steve Capper
@ 2013-12-13 12:06 ` Steve Capper
  2013-12-13 16:38   ` Will Deacon
  2013-12-13 12:06 ` [PATCH 2/2] arm: cacheflush: Fix user split-caching logic Steve Capper
  1 sibling, 1 reply; 7+ messages in thread
From: Steve Capper @ 2013-12-13 12:06 UTC (permalink / raw)
  To: linux-arm-kernel

When given a compound high page, __flush_dcache_page will only flush
the first page of the compound page repeatedly rather than the entire
set of constituent pages.

This patch corrects the logic such that all constituent pages are now
flushed.

Signed-off-by: Steve Capper <steve.capper@linaro.org>
---
 arch/arm/mm/flush.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
index 6d5ba9a..4962302 100644
--- a/arch/arm/mm/flush.c
+++ b/arch/arm/mm/flush.c
@@ -173,18 +173,19 @@ void __flush_dcache_page(struct address_space *mapping, struct page *page)
 		__cpuc_flush_dcache_area(page_address(page), page_size);
 	} else {
 		unsigned long i;
+		struct page *cpage = page;
 		if (cache_is_vipt_nonaliasing()) {
-			for (i = 0; i < (1 << compound_order(page)); i++) {
-				void *addr = kmap_atomic(page);
+			for (i = 0; i < (1 << compound_order(page)); cpage++, i++) {
+				void *addr = kmap_atomic(cpage);
 				__cpuc_flush_dcache_area(addr, PAGE_SIZE);
 				kunmap_atomic(addr);
 			}
 		} else {
-			for (i = 0; i < (1 << compound_order(page)); i++) {
-				void *addr = kmap_high_get(page);
+			for (i = 0; i < (1 << compound_order(page)); cpage++, i++) {
+				void *addr = kmap_high_get(cpage);
 				if (addr) {
 					__cpuc_flush_dcache_area(addr, PAGE_SIZE);
-					kunmap_high(page);
+					kunmap_high(cpage);
 				}
 			}
 		}
-- 
1.8.1.4

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

* [PATCH 2/2] arm: cacheflush: Fix user split-caching logic
  2013-12-13 12:06 [PATCH 0/2] Cache flush fixes Steve Capper
  2013-12-13 12:06 ` [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages Steve Capper
@ 2013-12-13 12:06 ` Steve Capper
  2013-12-13 12:41   ` Steve Capper
  1 sibling, 1 reply; 7+ messages in thread
From: Steve Capper @ 2013-12-13 12:06 UTC (permalink / raw)
  To: linux-arm-kernel

The user split-caching logic in __do_cache_op divides the area to be
flushed into interruptable chunks of size PAGE_SIZE. Unfortunately,
there is no check to see whether or not the range to be flushed is
smaller than the chunk size.

This can result cache flushes for larger ranges than intended,
which can result in the flush failing with a -EFAULT. I've observed
slowdown and failure with the icache-hygiene test from libhugetlbfs.

This patch fixes the problem by replacing chunk with the minimum of
PAGE_SIZE or (end - start), thus we do not overflush.

Signed-off-by: Steve Capper <steve.capper@linaro.org>
---
 arch/arm/kernel/traps.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
index 8fcda14..5d3c455 100644
--- a/arch/arm/kernel/traps.c
+++ b/arch/arm/kernel/traps.c
@@ -503,7 +503,7 @@ static inline int
 __do_cache_op(unsigned long start, unsigned long end)
 {
 	int ret;
-	unsigned long chunk = PAGE_SIZE;
+	unsigned long chunk = min(end - start, PAGE_SIZE);
 
 	do {
 		if (signal_pending(current)) {
-- 
1.8.1.4

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

* [PATCH 2/2] arm: cacheflush: Fix user split-caching logic
  2013-12-13 12:06 ` [PATCH 2/2] arm: cacheflush: Fix user split-caching logic Steve Capper
@ 2013-12-13 12:41   ` Steve Capper
  2013-12-13 12:51     ` Steve Capper
  0 siblings, 1 reply; 7+ messages in thread
From: Steve Capper @ 2013-12-13 12:41 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 13, 2013 at 12:06:31PM +0000, Steve Capper wrote:
> The user split-caching logic in __do_cache_op divides the area to be
> flushed into interruptable chunks of size PAGE_SIZE. Unfortunately,
> there is no check to see whether or not the range to be flushed is
> smaller than the chunk size.
> 
> This can result cache flushes for larger ranges than intended,
> which can result in the flush failing with a -EFAULT. I've observed
> slowdown and failure with the icache-hygiene test from libhugetlbfs.
> 
> This patch fixes the problem by replacing chunk with the minimum of
> PAGE_SIZE or (end - start), thus we do not overflush.
> 
> Signed-off-by: Steve Capper <steve.capper@linaro.org>

Gah, apologies. The min should be placed within the loop not before
as one may not have an exact multiple of chunk size to flush.

Please ignore this patch, I'll resend an improved version.

Cheers,
--
Steve

> ---
>  arch/arm/kernel/traps.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> index 8fcda14..5d3c455 100644
> --- a/arch/arm/kernel/traps.c
> +++ b/arch/arm/kernel/traps.c
> @@ -503,7 +503,7 @@ static inline int
>  __do_cache_op(unsigned long start, unsigned long end)
>  {
>  	int ret;
> -	unsigned long chunk = PAGE_SIZE;
> +	unsigned long chunk = min(end - start, PAGE_SIZE);
>  
>  	do {
>  		if (signal_pending(current)) {
> -- 
> 1.8.1.4
> 

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

* [PATCH 2/2] arm: cacheflush: Fix user split-caching logic
  2013-12-13 12:41   ` Steve Capper
@ 2013-12-13 12:51     ` Steve Capper
  0 siblings, 0 replies; 7+ messages in thread
From: Steve Capper @ 2013-12-13 12:51 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 13, 2013 at 12:41:31PM +0000, Steve Capper wrote:
> On Fri, Dec 13, 2013 at 12:06:31PM +0000, Steve Capper wrote:
> > The user split-caching logic in __do_cache_op divides the area to be
> > flushed into interruptable chunks of size PAGE_SIZE. Unfortunately,
> > there is no check to see whether or not the range to be flushed is
> > smaller than the chunk size.
> > 
> > This can result cache flushes for larger ranges than intended,
> > which can result in the flush failing with a -EFAULT. I've observed
> > slowdown and failure with the icache-hygiene test from libhugetlbfs.
> > 
> > This patch fixes the problem by replacing chunk with the minimum of
> > PAGE_SIZE or (end - start), thus we do not overflush.
> > 
> > Signed-off-by: Steve Capper <steve.capper@linaro.org>
> 
> Gah, apologies. The min should be placed within the loop not before
> as one may not have an exact multiple of chunk size to flush.
> 
> Please ignore this patch, I'll resend an improved version.
> 
> Cheers,
> --
> Steve

... or refer to this one:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/216597.html

Apologies for the noise.

The first patch in the series (fix for compound page flushing),
however, is still pertinent.

Cheers,
-- 
Steve

> 
> > ---
> >  arch/arm/kernel/traps.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> > index 8fcda14..5d3c455 100644
> > --- a/arch/arm/kernel/traps.c
> > +++ b/arch/arm/kernel/traps.c
> > @@ -503,7 +503,7 @@ static inline int
> >  __do_cache_op(unsigned long start, unsigned long end)
> >  {
> >  	int ret;
> > -	unsigned long chunk = PAGE_SIZE;
> > +	unsigned long chunk = min(end - start, PAGE_SIZE);
> >  
> >  	do {
> >  		if (signal_pending(current)) {
> > -- 
> > 1.8.1.4
> > 

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

* [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages
  2013-12-13 12:06 ` [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages Steve Capper
@ 2013-12-13 16:38   ` Will Deacon
  2013-12-16 15:07     ` Steve Capper
  0 siblings, 1 reply; 7+ messages in thread
From: Will Deacon @ 2013-12-13 16:38 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 13, 2013 at 12:06:30PM +0000, Steve Capper wrote:
> When given a compound high page, __flush_dcache_page will only flush
> the first page of the compound page repeatedly rather than the entire
> set of constituent pages.
> 
> This patch corrects the logic such that all constituent pages are now
> flushed.

Ha -- well spotted!

> Signed-off-by: Steve Capper <steve.capper@linaro.org>
> ---
>  arch/arm/mm/flush.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> index 6d5ba9a..4962302 100644
> --- a/arch/arm/mm/flush.c
> +++ b/arch/arm/mm/flush.c
> @@ -173,18 +173,19 @@ void __flush_dcache_page(struct address_space *mapping, struct page *page)
>  		__cpuc_flush_dcache_area(page_address(page), page_size);
>  	} else {
>  		unsigned long i;
> +		struct page *cpage = page;
>  		if (cache_is_vipt_nonaliasing()) {
> -			for (i = 0; i < (1 << compound_order(page)); i++) {
> -				void *addr = kmap_atomic(page);
> +			for (i = 0; i < (1 << compound_order(page)); cpage++, i++) {
> +				void *addr = kmap_atomic(cpage);

Any reason not to use page + i?

Will

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

* [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages
  2013-12-13 16:38   ` Will Deacon
@ 2013-12-16 15:07     ` Steve Capper
  0 siblings, 0 replies; 7+ messages in thread
From: Steve Capper @ 2013-12-16 15:07 UTC (permalink / raw)
  To: linux-arm-kernel

On Fri, Dec 13, 2013 at 04:38:21PM +0000, Will Deacon wrote:
> On Fri, Dec 13, 2013 at 12:06:30PM +0000, Steve Capper wrote:
> > When given a compound high page, __flush_dcache_page will only flush
> > the first page of the compound page repeatedly rather than the entire
> > set of constituent pages.
> > 
> > This patch corrects the logic such that all constituent pages are now
> > flushed.
> 
> Ha -- well spotted!
> 
/me looks around innocently
> > Signed-off-by: Steve Capper <steve.capper@linaro.org>
> > ---
> >  arch/arm/mm/flush.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/mm/flush.c b/arch/arm/mm/flush.c
> > index 6d5ba9a..4962302 100644
> > --- a/arch/arm/mm/flush.c
> > +++ b/arch/arm/mm/flush.c
> > @@ -173,18 +173,19 @@ void __flush_dcache_page(struct address_space *mapping, struct page *page)
> >  		__cpuc_flush_dcache_area(page_address(page), page_size);
> >  	} else {
> >  		unsigned long i;
> > +		struct page *cpage = page;
> >  		if (cache_is_vipt_nonaliasing()) {
> > -			for (i = 0; i < (1 << compound_order(page)); i++) {
> > -				void *addr = kmap_atomic(page);
> > +			for (i = 0; i < (1 << compound_order(page)); cpage++, i++) {
> > +				void *addr = kmap_atomic(cpage);
> 
> Any reason not to use page + i?
> 

Thanks, that does look better with a page + i. I have sent a V2 patch
that includes this change:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-December/219571.html

If this looks okay should it go in the patch system? And I can add a
stable tag too?

Cheers,
-- 
Steve

> Will

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

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

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-13 12:06 [PATCH 0/2] Cache flush fixes Steve Capper
2013-12-13 12:06 ` [PATCH 1/2] arm: mm: fix dcache flush logic for compound high pages Steve Capper
2013-12-13 16:38   ` Will Deacon
2013-12-16 15:07     ` Steve Capper
2013-12-13 12:06 ` [PATCH 2/2] arm: cacheflush: Fix user split-caching logic Steve Capper
2013-12-13 12:41   ` Steve Capper
2013-12-13 12:51     ` Steve Capper

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