[PATCH] ARM:mm: fix kmap_atomic_to_page

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Chanho Min]

Since kmap_atomic returns the pkmap address without a new mapping to
fixmap for the page that is already mapped by kmap, It should be
considered for the pkmap address in kmap_atomic_to_page.

Signed-off-by: Chanho Min <chanho.min@lge.com>
---
 arch/arm/mm/highmem.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm/mm/highmem.c b/arch/arm/mm/highmem.c
index 45aeaac..3e973b7 100644
--- a/arch/arm/mm/highmem.c
+++ b/arch/arm/mm/highmem.c
@@ -145,8 +145,13 @@ struct page *kmap_atomic_to_page(const void *ptr)
 {
 	unsigned long vaddr = (unsigned long)ptr;
 
-	if (vaddr < FIXADDR_START)
-		return virt_to_page(ptr);
+	if (vaddr >= PKMAP_ADDR(0) && vaddr < PKMAP_ADDR(LAST_PKMAP)) {
+		int i = PKMAP_NR(vaddr);
+		return pte_page(pkmap_page_table[i]);
+	}
+
+	if (vaddr >= FIXADDR_START)
+		return pte_page(get_fixmap_pte(vaddr));
 
-	return pte_page(get_fixmap_pte(vaddr));
+	return virt_to_page(ptr);
 }
-- 
1.7.9.5

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Russell King - ARM Linux]

On Tue, Oct 06, 2015 at 08:09:33PM +0900, Chanho Min wrote:
> Since kmap_atomic returns the pkmap address without a new mapping to
> fixmap for the page that is already mapped by kmap, It should be
> considered for the pkmap address in kmap_atomic_to_page.

What's the reasoning behind this change, given that I can find lots of
definitions of kmap_atomic_to_page() in the kernel, but not a single
user of this.

If there's no users, should we be deleting this code?

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Nicolas Pitre]

On Tue, 6 Oct 2015, Russell King - ARM Linux wrote:

> On Tue, Oct 06, 2015 at 08:09:33PM +0900, Chanho Min wrote:
> > Since kmap_atomic returns the pkmap address without a new mapping to
> > fixmap for the page that is already mapped by kmap, It should be
> > considered for the pkmap address in kmap_atomic_to_page.
> 
> What's the reasoning behind this change, given that I can find lots of
> definitions of kmap_atomic_to_page() in the kernel, but not a single
> user of this.
> 
> If there's no users, should we be deleting this code?

I think commit 5bbeed12bdc3 provides the answer to that question.


Nicolas

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Jongsung Kim]

Recently, we made a driver utilizing kmap_atomic_to_page. Of course, it's not mainlined. People may be using it outside mainline just like us.

vmalloc has vmalloc_to_page, pkmap has kmap_to page, and fixmap has kmap_atomic_to_page. Then.. how about letting virt_to_page do them all?


On 10/07/2015 10:37 AM, Nicolas Pitre wrote:
> On Tue, 6 Oct 2015, Russell King - ARM Linux wrote:
>
>> On Tue, Oct 06, 2015 at 08:09:33PM +0900, Chanho Min wrote:
>>> Since kmap_atomic returns the pkmap address without a new mapping to
>>> fixmap for the page that is already mapped by kmap, It should be
>>> considered for the pkmap address in kmap_atomic_to_page.
>> What's the reasoning behind this change, given that I can find lots of
>> definitions of kmap_atomic_to_page() in the kernel, but not a single
>> user of this.
>>
>> If there's no users, should we be deleting this code?
> I think commit 5bbeed12bdc3 provides the answer to that question.
>
>
> Nicolas
>

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Russell King - ARM Linux]

On Wed, Oct 07, 2015 at 12:55:08PM +0900, Jongsung Kim wrote:
> Recently, we made a driver utilizing kmap_atomic_to_page. Of course,
> it's not mainlined. People may be using it outside mainline just like us.

Since kmap_atomic() mappings are supposed to be short-lived, why do you
need it in your driver?  Don't you already have the struct page pointer
when setting up the kmap_atomic() mapping?

It is invalid to setup a mapping, and leave it setup across any context
switching or similar.

Also, kmap_atomic_to_page() is not exported to modules, so you can only
use it when built-in.

> vmalloc has vmalloc_to_page, pkmap has kmap_to page, and fixmap has
> kmap_atomic_to_page. Then.. how about letting virt_to_page do them all?

No.  virt_to_page() is defined to only work on the lowmem mapping, and
that's not going to change.

Please show the outline of your code making use of this function so we
can better understand your use case.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Jongsung Kim]

We tried to utilize a HW compressor as a zram backend. Current zram uses kmap_atomic to map a page, and the HW DMAes. So we needed to use kmap_atomic_to_page to get the page to be dma-mapped.


On 10/07/2015 06:01 PM, Russell King - ARM Linux wrote:
> On Wed, Oct 07, 2015 at 12:55:08PM +0900, Jongsung Kim wrote:
>> Recently, we made a driver utilizing kmap_atomic_to_page. Of course,
>> it's not mainlined. People may be using it outside mainline just like us.
> Since kmap_atomic() mappings are supposed to be short-lived, why do you
> need it in your driver?  Don't you already have the struct page pointer
> when setting up the kmap_atomic() mapping?
>
> It is invalid to setup a mapping, and leave it setup across any context
> switching or similar.
>
> Also, kmap_atomic_to_page() is not exported to modules, so you can only
> use it when built-in.
>
>> vmalloc has vmalloc_to_page, pkmap has kmap_to page, and fixmap has
>> kmap_atomic_to_page. Then.. how about letting virt_to_page do them all?
> No.  virt_to_page() is defined to only work on the lowmem mapping, and
> that's not going to change.
>
> Please show the outline of your code making use of this function so we
> can better understand your use case.
>

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Arnd Bergmann]

On Monday 12 October 2015 14:30:39 Jongsung Kim wrote:
> We tried to utilize a HW compressor as a zram backend. Current zram uses
> kmap_atomic to map a page, and the HW DMAes. So we needed to use
> kmap_atomic_to_page to get the page to be dma-mapped.

How about changing the zcomp code to pass the page pointer instead of
the kernel space pointer? That would avoid having to do the kmap_atomic,
which can itself be expensive on 32-bit machines and should not
be needed here if you have a HW DMA engine doing the compression.

	Arnd

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Jongsung Kim]

On 10/12/2015 06:27 PM, Arnd Bergmann wrote:
> How about changing the zcomp code to pass the page pointer instead of the kernel space pointer? That would avoid having to do the kmap_atomic, which can itself be expensive on 32-bit machines and should not be needed here if you have a HW DMA engine doing the compression. Arnd 

Mainline zram uses lzo / lz4 library functions as backend. Using kmap_atomic and passing address look reasonable.

[PATCH] ARM:mm: fix kmap_atomic_to_page

[Arnd Bergmann]

On Tuesday 13 October 2015 11:06:44 Jongsung Kim wrote:
> On 10/12/2015 06:27 PM, Arnd Bergmann wrote:
> > How about changing the zcomp code to pass the page pointer instead of the kernel space pointer? That would avoid having to do the kmap_atomic, which can itself be expensive on 32-bit machines and should not be needed here if you have a HW DMA engine doing the compression. Arnd 
> 
> Mainline zram uses lzo / lz4 library functions as backend. Using kmap_atomic and passing address look reasonable.

Yes, I know, but changing this to do the kmap_atomic in the backend
seems better here if you are adding another backend that doesn't
want or need the kmap_atomic.

	Arnd

[PATCH] ARM:mm: fix kmap_atomic_to_page

[yalin wang]

> On Oct 6, 2015, at 19:09, Chanho Min <chanho.min@lge.com> wrote:
> 
> Since kmap_atomic returns the pkmap address without a new mapping to
> fixmap for the page that is already mapped by kmap, It should be
> considered for the pkmap address in kmap_atomic_to_page.
> 
> Signed-off-by: Chanho Min <chanho.min@lge.com>
> ---
> arch/arm/mm/highmem.c |   11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/mm/highmem.c b/arch/arm/mm/highmem.c
> index 45aeaac..3e973b7 100644-
> --- a/arch/arm/mm/highmem.c
> +++ b/arch/arm/mm/highmem.c
> @@ -145,8 +145,13 @@ struct page *kmap_atomic_to_page(const void *ptr)
> {
> 	unsigned long vaddr = (unsigned long)ptr;
> 
> -	if (vaddr < FIXADDR_START)
> -		return virt_to_page(ptr);
> +	if (vaddr >= PKMAP_ADDR(0) && vaddr < PKMAP_ADDR(LAST_PKMAP)) {
> +		int i = PKMAP_NR(vaddr);
> +		return pte_page(pkmap_page_table[i]);
> +	}
> +
> +	if (vaddr >= FIXADDR_START)
> +		return pte_page(get_fixmap_pte(vaddr));
> 
> -	return pte_page(get_fixmap_pte(vaddr));
> +	return virt_to_page(ptr);
> }
> -- 
> 1.7.9.5
> 
why not change like this,
more simple :

diff --git a/arch/arm/mm/highmem.c b/arch/arm/mm/highmem.c
index 9df5f09..4eeed3d 100644
--- a/arch/arm/mm/highmem.c
+++ b/arch/arm/mm/highmem.c
@@ -152,8 +152,7 @@ struct page *kmap_atomic_to_page(const void *ptr)
 {
        unsigned long vaddr = (unsigned long)ptr;
 
-       if (vaddr < FIXADDR_START)
-               return virt_to_page(ptr);
-
-       return pte_page(get_fixmap_pte(vaddr));
+       if (vaddr >= FIXADDR_START && vaddr <= FIXADDR_TOP)
+               return pte_page(get_fixmap_pte(vaddr));
+       return kmap_to_page(ptr);
 }



Thanks