Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1240378 > unrolled thread

[PATCH] ARM:mm: fix kmap_atomic_to_page

Back to article view | Back to linux.kernel

Contents

  [PATCH] ARM:mm: fix kmap_atomic_to_page Chanho Min <chanho.min@lge.com> - 2015-10-06 13:30 +0200
    Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-06 21:30 +0200
      Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Nicolas Pitre <nicolas.pitre@linaro.org> - 2015-10-07 03:40 +0200
        Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Jongsung Kim <neidhard.kim@lge.com> - 2015-10-07 06:20 +0200
          Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-10-07 11:10 +0200
            Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Jongsung Kim <neidhard.kim@lge.com> - 2015-10-12 07:40 +0200
              Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Arnd Bergmann <arnd@arndb.de> - 2015-10-12 11:30 +0200
                Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Jongsung Kim <neidhard.kim@lge.com> - 2015-10-13 04:10 +0200
                  Re: [PATCH] ARM:mm: fix kmap_atomic_to_page Arnd Bergmann <arnd@arndb.de> - 2015-10-13 13:50 +0200
    Re: [PATCH] ARM:mm: fix kmap_atomic_to_page yalin wang <yalin.wang2010@gmail.com> - 2015-10-12 08:10 +0200

#1240378 — [PATCH] ARM:mm: fix kmap_atomic_to_page

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1240871

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241121

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241147

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
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241249

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244387

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244526

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245281

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1245624

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1244400

> 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


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web