| Message ID | 1444129773-12632-1-git-send-email-chanho.min@lge.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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?
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
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 >
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.
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. >
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
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.
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
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); }
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(-)