linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c
@ 2022-07-04 15:23 Fabio M. De Francesco
  2022-07-04 15:23 ` [PATCH v5 1/2] highmem: Make __kunmap_{local,atomic}() take "const void *" Fabio M. De Francesco
  2022-07-04 15:23 ` [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
  0 siblings, 2 replies; 6+ messages in thread
From: Fabio M. De Francesco @ 2022-07-04 15:23 UTC (permalink / raw)
  To: David Sterba, Chris Mason, Josef Bacik, Nick Terrell,
	linux-btrfs, linux-mm, linux-kernel, Ira Weiny, Andrew Morton,
	Matthew Wilcox, Kees Cook, Sebastian Andrzej Siewior,
	James E. J. Bottomley, Helge Deller, John David Anglin,
	linux-parisc
  Cc: Fabio M. De Francesco

This is a little series which serves the purpose to replace kmap() with
kmap_local_page() in btrfs/zstd.c. Actually this task is only accomplished
in patch 2/2.

Instead patch 1/2 is a pre-requisite for the above-mentioned replacement,
however, above all else, it has the purpose to conform the prototypes of
__kunmap_{local,atomic}() to their own correct semantics. Since those
functions don't make changes to the memory pointed by their arguments,
change the type of those arguments to become pointers to const void.

v4 -> v5: Use plain page_address() for pages which cannot come from Highmem
(instead of kmapping them); remove unnecessary initialisations to NULL
(thanks to Ira Weiny).

v3 -> v4: Resend and add linux-mm to the list of recipients (thanks to
Andrew Morton).

Fabio M. De Francesco (2):
  highmem: Make __kunmap_{local,atomic}() take "const void *"
  btrfs: Replace kmap() with kmap_local_page() in zstd.c

 arch/parisc/include/asm/cacheflush.h |  6 ++---
 arch/parisc/kernel/cache.c           |  2 +-
 fs/btrfs/zstd.c                      | 34 ++++++++++++----------------
 include/linux/highmem-internal.h     | 10 ++++----
 mm/highmem.c                         |  2 +-
 5 files changed, 24 insertions(+), 30 deletions(-)

-- 
2.36.1



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

* [PATCH v5 1/2] highmem: Make __kunmap_{local,atomic}() take "const void *"
  2022-07-04 15:23 [PATCH v5 0/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
@ 2022-07-04 15:23 ` Fabio M. De Francesco
  2022-07-04 15:23 ` [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
  1 sibling, 0 replies; 6+ messages in thread
From: Fabio M. De Francesco @ 2022-07-04 15:23 UTC (permalink / raw)
  To: David Sterba, Chris Mason, Josef Bacik, Nick Terrell,
	linux-btrfs, linux-mm, linux-kernel, Ira Weiny, Andrew Morton,
	Matthew Wilcox, Kees Cook, Sebastian Andrzej Siewior,
	James E. J. Bottomley, Helge Deller, John David Anglin,
	linux-parisc
  Cc: Fabio M. De Francesco, David Sterba

__kunmap_ {local,atomic}() currently take pointers to void. However, this
is semantically incorrect, since these functions do not change the memory
their arguments point to.

Therefore, make this semantics explicit by modifying the
__kunmap_{local,atomic}() prototypes to take pointers to const void.

As a side effect, compilers will likely produce more efficient code.

Cc: Andrew Morton <akpm@linux-foundation.org>
Suggested-by: David Sterba <dsterba@suse.cz>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 arch/parisc/include/asm/cacheflush.h |  6 +++---
 arch/parisc/kernel/cache.c           |  2 +-
 include/linux/highmem-internal.h     | 10 +++++-----
 mm/highmem.c                         |  2 +-
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/arch/parisc/include/asm/cacheflush.h b/arch/parisc/include/asm/cacheflush.h
index 8d03b3b26229..0bdee6724132 100644
--- a/arch/parisc/include/asm/cacheflush.h
+++ b/arch/parisc/include/asm/cacheflush.h
@@ -22,7 +22,7 @@ void flush_kernel_icache_range_asm(unsigned long, unsigned long);
 void flush_user_dcache_range_asm(unsigned long, unsigned long);
 void flush_kernel_dcache_range_asm(unsigned long, unsigned long);
 void purge_kernel_dcache_range_asm(unsigned long, unsigned long);
-void flush_kernel_dcache_page_asm(void *);
+void flush_kernel_dcache_page_asm(const void *addr);
 void flush_kernel_icache_page(void *);
 
 /* Cache flush operations */
@@ -31,7 +31,7 @@ void flush_cache_all_local(void);
 void flush_cache_all(void);
 void flush_cache_mm(struct mm_struct *mm);
 
-void flush_kernel_dcache_page_addr(void *addr);
+void flush_kernel_dcache_page_addr(const void *addr);
 
 #define flush_kernel_dcache_range(start,size) \
 	flush_kernel_dcache_range_asm((start), (start)+(size));
@@ -75,7 +75,7 @@ void flush_dcache_page_asm(unsigned long phys_addr, unsigned long vaddr);
 void flush_anon_page(struct vm_area_struct *vma, struct page *page, unsigned long vmaddr);
 
 #define ARCH_HAS_FLUSH_ON_KUNMAP
-static inline void kunmap_flush_on_unmap(void *addr)
+static inline void kunmap_flush_on_unmap(const void *addr)
 {
 	flush_kernel_dcache_page_addr(addr);
 }
diff --git a/arch/parisc/kernel/cache.c b/arch/parisc/kernel/cache.c
index a9bc578e4c52..993999a65e54 100644
--- a/arch/parisc/kernel/cache.c
+++ b/arch/parisc/kernel/cache.c
@@ -549,7 +549,7 @@ extern void purge_kernel_dcache_page_asm(unsigned long);
 extern void clear_user_page_asm(void *, unsigned long);
 extern void copy_user_page_asm(void *, void *, unsigned long);
 
-void flush_kernel_dcache_page_addr(void *addr)
+void flush_kernel_dcache_page_addr(const void *addr)
 {
 	unsigned long flags;
 
diff --git a/include/linux/highmem-internal.h b/include/linux/highmem-internal.h
index cddb42ff0473..034b1106d022 100644
--- a/include/linux/highmem-internal.h
+++ b/include/linux/highmem-internal.h
@@ -8,7 +8,7 @@
 #ifdef CONFIG_KMAP_LOCAL
 void *__kmap_local_pfn_prot(unsigned long pfn, pgprot_t prot);
 void *__kmap_local_page_prot(struct page *page, pgprot_t prot);
-void kunmap_local_indexed(void *vaddr);
+void kunmap_local_indexed(const void *vaddr);
 void kmap_local_fork(struct task_struct *tsk);
 void __kmap_local_sched_out(void);
 void __kmap_local_sched_in(void);
@@ -89,7 +89,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
 	return __kmap_local_pfn_prot(pfn, kmap_prot);
 }
 
-static inline void __kunmap_local(void *vaddr)
+static inline void __kunmap_local(const void *vaddr)
 {
 	kunmap_local_indexed(vaddr);
 }
@@ -121,7 +121,7 @@ static inline void *kmap_atomic_pfn(unsigned long pfn)
 	return __kmap_local_pfn_prot(pfn, kmap_prot);
 }
 
-static inline void __kunmap_atomic(void *addr)
+static inline void __kunmap_atomic(const void *addr)
 {
 	kunmap_local_indexed(addr);
 	pagefault_enable();
@@ -197,7 +197,7 @@ static inline void *kmap_local_pfn(unsigned long pfn)
 	return kmap_local_page(pfn_to_page(pfn));
 }
 
-static inline void __kunmap_local(void *addr)
+static inline void __kunmap_local(const void *addr)
 {
 #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
 	kunmap_flush_on_unmap(addr);
@@ -224,7 +224,7 @@ static inline void *kmap_atomic_pfn(unsigned long pfn)
 	return kmap_atomic(pfn_to_page(pfn));
 }
 
-static inline void __kunmap_atomic(void *addr)
+static inline void __kunmap_atomic(const void *addr)
 {
 #ifdef ARCH_HAS_FLUSH_ON_KUNMAP
 	kunmap_flush_on_unmap(addr);
diff --git a/mm/highmem.c b/mm/highmem.c
index 1a692997fac4..e32083e4ce0d 100644
--- a/mm/highmem.c
+++ b/mm/highmem.c
@@ -561,7 +561,7 @@ void *__kmap_local_page_prot(struct page *page, pgprot_t prot)
 }
 EXPORT_SYMBOL(__kmap_local_page_prot);
 
-void kunmap_local_indexed(void *vaddr)
+void kunmap_local_indexed(const void *vaddr)
 {
 	unsigned long addr = (unsigned long) vaddr & PAGE_MASK;
 	pte_t *kmap_pte;
-- 
2.36.1



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

* [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c
  2022-07-04 15:23 [PATCH v5 0/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
  2022-07-04 15:23 ` [PATCH v5 1/2] highmem: Make __kunmap_{local,atomic}() take "const void *" Fabio M. De Francesco
@ 2022-07-04 15:23 ` Fabio M. De Francesco
  2022-07-05 19:04   ` Ira Weiny
  1 sibling, 1 reply; 6+ messages in thread
From: Fabio M. De Francesco @ 2022-07-04 15:23 UTC (permalink / raw)
  To: David Sterba, Chris Mason, Josef Bacik, Nick Terrell,
	linux-btrfs, linux-mm, linux-kernel, Ira Weiny, Andrew Morton,
	Matthew Wilcox, Kees Cook, Sebastian Andrzej Siewior,
	James E. J. Bottomley, Helge Deller, John David Anglin,
	linux-parisc
  Cc: Fabio M. De Francesco, Filipe Manana

The use of kmap() is being deprecated in favor of kmap_local_page(). With
kmap_local_page(), the mapping is per thread, CPU local and not globally
visible.

Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in this
file the mappings are per thread and are not visible in other contexts. In
the meanwhile use plain page_address() on pages allocated with the GFP_NOFS
flag instead of calling kmap*() on them (since they are always allocated
from ZONE_NORMAL).

Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and
booting a kernel with HIGHMEM64G enabled.

Cc: Filipe Manana <fdmanana@kernel.org>
Suggested-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
---
 fs/btrfs/zstd.c | 34 ++++++++++++++--------------------
 1 file changed, 14 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
index 0fe31a6f6e68..78e0272e770e 100644
--- a/fs/btrfs/zstd.c
+++ b/fs/btrfs/zstd.c
@@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 
 	/* map in the first page of input data */
 	in_page = find_get_page(mapping, start >> PAGE_SHIFT);
-	workspace->in_buf.src = kmap(in_page);
+	workspace->in_buf.src = kmap_local_page(in_page);
 	workspace->in_buf.pos = 0;
 	workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
 
@@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 		goto out;
 	}
 	pages[nr_pages++] = out_page;
-	workspace->out_buf.dst = kmap(out_page);
+	workspace->out_buf.dst = page_address(out_page);
 	workspace->out_buf.pos = 0;
 	workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
 
@@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 		if (workspace->out_buf.pos == workspace->out_buf.size) {
 			tot_out += PAGE_SIZE;
 			max_out -= PAGE_SIZE;
-			kunmap(out_page);
 			if (nr_pages == nr_dest_pages) {
-				out_page = NULL;
 				ret = -E2BIG;
 				goto out;
 			}
@@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 				goto out;
 			}
 			pages[nr_pages++] = out_page;
-			workspace->out_buf.dst = kmap(out_page);
+			workspace->out_buf.dst = page_address(out_page);
 			workspace->out_buf.pos = 0;
 			workspace->out_buf.size = min_t(size_t, max_out,
 							PAGE_SIZE);
@@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 		/* Check if we need more input */
 		if (workspace->in_buf.pos == workspace->in_buf.size) {
 			tot_in += PAGE_SIZE;
-			kunmap(in_page);
+			kunmap_local(workspace->in_buf.src);
 			put_page(in_page);
-
 			start += PAGE_SIZE;
 			len -= PAGE_SIZE;
 			in_page = find_get_page(mapping, start >> PAGE_SHIFT);
-			workspace->in_buf.src = kmap(in_page);
+			workspace->in_buf.src = kmap_local_page(in_page);
 			workspace->in_buf.pos = 0;
 			workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
+			workspace->out_buf.dst = page_address(out_page);
 		}
 	}
 	while (1) {
@@ -510,9 +508,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 
 		tot_out += PAGE_SIZE;
 		max_out -= PAGE_SIZE;
-		kunmap(out_page);
 		if (nr_pages == nr_dest_pages) {
-			out_page = NULL;
 			ret = -E2BIG;
 			goto out;
 		}
@@ -522,7 +518,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 			goto out;
 		}
 		pages[nr_pages++] = out_page;
-		workspace->out_buf.dst = kmap(out_page);
+		workspace->out_buf.dst = page_address(out_page);
 		workspace->out_buf.pos = 0;
 		workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
 	}
@@ -537,13 +533,10 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
 	*total_out = tot_out;
 out:
 	*out_pages = nr_pages;
-	/* Cleanup */
-	if (in_page) {
-		kunmap(in_page);
+	if (workspace->in_buf.src) {
+		kunmap_local(workspace->in_buf.src);
 		put_page(in_page);
 	}
-	if (out_page)
-		kunmap(out_page);
 	return ret;
 }
 
@@ -567,7 +560,7 @@ int zstd_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 		goto done;
 	}
 
-	workspace->in_buf.src = kmap(pages_in[page_in_index]);
+	workspace->in_buf.src = kmap_local_page(pages_in[page_in_index]);
 	workspace->in_buf.pos = 0;
 	workspace->in_buf.size = min_t(size_t, srclen, PAGE_SIZE);
 
@@ -603,14 +596,15 @@ int zstd_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 			break;
 
 		if (workspace->in_buf.pos == workspace->in_buf.size) {
-			kunmap(pages_in[page_in_index++]);
+			kunmap_local(workspace->in_buf.src);
+			page_in_index++;
 			if (page_in_index >= total_pages_in) {
 				workspace->in_buf.src = NULL;
 				ret = -EIO;
 				goto done;
 			}
 			srclen -= PAGE_SIZE;
-			workspace->in_buf.src = kmap(pages_in[page_in_index]);
+			workspace->in_buf.src = kmap_local_page(pages_in[page_in_index]);
 			workspace->in_buf.pos = 0;
 			workspace->in_buf.size = min_t(size_t, srclen, PAGE_SIZE);
 		}
@@ -619,7 +613,7 @@ int zstd_decompress_bio(struct list_head *ws, struct compressed_bio *cb)
 	zero_fill_bio(cb->orig_bio);
 done:
 	if (workspace->in_buf.src)
-		kunmap(pages_in[page_in_index]);
+		kunmap_local(workspace->in_buf.src);
 	return ret;
 }
 
-- 
2.36.1



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

* Re: [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c
  2022-07-04 15:23 ` [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
@ 2022-07-05 19:04   ` Ira Weiny
  2022-07-06 10:38     ` Fabio M. De Francesco
  0 siblings, 1 reply; 6+ messages in thread
From: Ira Weiny @ 2022-07-05 19:04 UTC (permalink / raw)
  To: Fabio M. De Francesco
  Cc: David Sterba, Chris Mason, Josef Bacik, Nick Terrell,
	linux-btrfs, linux-mm, linux-kernel, Andrew Morton,
	Matthew Wilcox, Kees Cook, Sebastian Andrzej Siewior,
	James E. J. Bottomley, Helge Deller, John David Anglin,
	linux-parisc, Filipe Manana

On Mon, Jul 04, 2022 at 05:23:22PM +0200, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page(). With
> kmap_local_page(), the mapping is per thread, CPU local and not globally
> visible.
> 
> Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in this
> file the mappings are per thread and are not visible in other contexts. In
> the meanwhile use plain page_address() on pages allocated with the GFP_NOFS
> flag instead of calling kmap*() on them (since they are always allocated
> from ZONE_NORMAL).
> 
> Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and
> booting a kernel with HIGHMEM64G enabled.
> 
> Cc: Filipe Manana <fdmanana@kernel.org>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
>  fs/btrfs/zstd.c | 34 ++++++++++++++--------------------
>  1 file changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> index 0fe31a6f6e68..78e0272e770e 100644
> --- a/fs/btrfs/zstd.c
> +++ b/fs/btrfs/zstd.c
> @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
>  
>  	/* map in the first page of input data */
>  	in_page = find_get_page(mapping, start >> PAGE_SHIFT);
> -	workspace->in_buf.src = kmap(in_page);
> +	workspace->in_buf.src = kmap_local_page(in_page);
>  	workspace->in_buf.pos = 0;
>  	workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
>  
> @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
>  		goto out;
>  	}
>  	pages[nr_pages++] = out_page;
> -	workspace->out_buf.dst = kmap(out_page);
> +	workspace->out_buf.dst = page_address(out_page);
>  	workspace->out_buf.pos = 0;
>  	workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
>  
> @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
>  		if (workspace->out_buf.pos == workspace->out_buf.size) {
>  			tot_out += PAGE_SIZE;
>  			max_out -= PAGE_SIZE;
> -			kunmap(out_page);
>  			if (nr_pages == nr_dest_pages) {
> -				out_page = NULL;
>  				ret = -E2BIG;
>  				goto out;
>  			}
> @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
>  				goto out;
>  			}
>  			pages[nr_pages++] = out_page;
> -			workspace->out_buf.dst = kmap(out_page);
> +			workspace->out_buf.dst = page_address(out_page);
>  			workspace->out_buf.pos = 0;
>  			workspace->out_buf.size = min_t(size_t, max_out,
>  							PAGE_SIZE);
> @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, struct address_space *mapping,
>  		/* Check if we need more input */
>  		if (workspace->in_buf.pos == workspace->in_buf.size) {
>  			tot_in += PAGE_SIZE;
> -			kunmap(in_page);
> +			kunmap_local(workspace->in_buf.src);
>  			put_page(in_page);
> -
>  			start += PAGE_SIZE;
>  			len -= PAGE_SIZE;
>  			in_page = find_get_page(mapping, start >> PAGE_SHIFT);
> -			workspace->in_buf.src = kmap(in_page);
> +			workspace->in_buf.src = kmap_local_page(in_page);
>  			workspace->in_buf.pos = 0;
>  			workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
> +			workspace->out_buf.dst = page_address(out_page);

Why is this needed?

The rest looks good,
Ira

[snip]


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

* Re: [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c
  2022-07-05 19:04   ` Ira Weiny
@ 2022-07-06 10:38     ` Fabio M. De Francesco
  2022-07-06 10:48       ` Fabio M. De Francesco
  0 siblings, 1 reply; 6+ messages in thread
From: Fabio M. De Francesco @ 2022-07-06 10:38 UTC (permalink / raw)
  To: Ira Weiny
  Cc: David Sterba, Chris Mason, Josef Bacik, Nick Terrell,
	linux-btrfs, linux-mm, linux-kernel, Andrew Morton,
	Matthew Wilcox, Kees Cook, Sebastian Andrzej Siewior,
	James E. J. Bottomley, Helge Deller, John David Anglin,
	linux-parisc, Filipe Manana

On martedì 5 luglio 2022 21:04:04 CEST Ira Weiny wrote:
> On Mon, Jul 04, 2022 at 05:23:22PM +0200, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page(). 
With
> > kmap_local_page(), the mapping is per thread, CPU local and not 
globally
> > visible.
> > 
> > Therefore, use kmap_local_page() / kunmap_local() in zstd.c because in 
this
> > file the mappings are per thread and are not visible in other contexts. 
In
> > the meanwhile use plain page_address() on pages allocated with the 
GFP_NOFS
> > flag instead of calling kmap*() on them (since they are always 
allocated
> > from ZONE_NORMAL).
> > 
> > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and
> > booting a kernel with HIGHMEM64G enabled.
> > 
> > Cc: Filipe Manana <fdmanana@kernel.org>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > ---
> >  fs/btrfs/zstd.c | 34 ++++++++++++++--------------------
> >  1 file changed, 14 insertions(+), 20 deletions(-)
> > 
> > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> > index 0fe31a6f6e68..78e0272e770e 100644
> > --- a/fs/btrfs/zstd.c
> > +++ b/fs/btrfs/zstd.c
> > @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, 
struct address_space *mapping,
> >  
> >  	/* map in the first page of input data */
> >  	in_page = find_get_page(mapping, start >> PAGE_SHIFT);
> > -	workspace->in_buf.src = kmap(in_page);
> > +	workspace->in_buf.src = kmap_local_page(in_page);
> >  	workspace->in_buf.pos = 0;
> >  	workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
> >  
> > @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, 
struct address_space *mapping,
> >  		goto out;
> >  	}
> >  	pages[nr_pages++] = out_page;
> > -	workspace->out_buf.dst = kmap(out_page);
> > +	workspace->out_buf.dst = page_address(out_page);
> >  	workspace->out_buf.pos = 0;
> >  	workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
> >  
> > @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, 
struct address_space *mapping,
> >  		if (workspace->out_buf.pos == workspace->out_buf.size) 
{
> >  			tot_out += PAGE_SIZE;
> >  			max_out -= PAGE_SIZE;
> > -			kunmap(out_page);
> >  			if (nr_pages == nr_dest_pages) {
> > -				out_page = NULL;
> >  				ret = -E2BIG;
> >  				goto out;
> >  			}
> > @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, 
struct address_space *mapping,
> >  				goto out;
> >  			}
> >  			pages[nr_pages++] = out_page;
> > -			workspace->out_buf.dst = kmap(out_page);
> > +			workspace->out_buf.dst = 
page_address(out_page);
> >  			workspace->out_buf.pos = 0;
> >  			workspace->out_buf.size = min_t(size_t, 
max_out,
> >  							
PAGE_SIZE);
> > @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, 
struct address_space *mapping,
> >  		/* Check if we need more input */
> >  		if (workspace->in_buf.pos == workspace->in_buf.size) {
> >  			tot_in += PAGE_SIZE;
> > -			kunmap(in_page);
> > +			kunmap_local(workspace->in_buf.src);
> >  			put_page(in_page);
> > -
> >  			start += PAGE_SIZE;
> >  			len -= PAGE_SIZE;
> >  			in_page = find_get_page(mapping, start >> 
PAGE_SHIFT);
> > -			workspace->in_buf.src = kmap(in_page);
> > +			workspace->in_buf.src = 
kmap_local_page(in_page);
> >  			workspace->in_buf.pos = 0;
> >  			workspace->in_buf.size = min_t(size_t, len, 
PAGE_SIZE);
> > +			workspace->out_buf.dst = 
page_address(out_page);
> 
> Why is this needed?

Sorry. This initialization is not needed at all.
Probably made a mistake with copy-pasting snippets of code.

I'm going to send ASAP the fifth version of this series.

> The rest looks good,

Thanks,

Fabio

> Ira
> 
> [snip]
> 





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

* Re: [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c
  2022-07-06 10:38     ` Fabio M. De Francesco
@ 2022-07-06 10:48       ` Fabio M. De Francesco
  0 siblings, 0 replies; 6+ messages in thread
From: Fabio M. De Francesco @ 2022-07-06 10:48 UTC (permalink / raw)
  To: Ira Weiny
  Cc: David Sterba, Chris Mason, Josef Bacik, Nick Terrell,
	linux-btrfs, linux-mm, linux-kernel, Andrew Morton,
	Matthew Wilcox, Kees Cook, Sebastian Andrzej Siewior,
	James E. J. Bottomley, Helge Deller, John David Anglin,
	linux-parisc, Filipe Manana

On mercoledì 6 luglio 2022 12:38:29 CEST Fabio M. De Francesco wrote:
> On martedì 5 luglio 2022 21:04:04 CEST Ira Weiny wrote:
> > On Mon, Jul 04, 2022 at 05:23:22PM +0200, Fabio M. De Francesco wrote:
> > > The use of kmap() is being deprecated in favor of kmap_local_page(). 
> With
> > > kmap_local_page(), the mapping is per thread, CPU local and not 
> globally
> > > visible.
> > > 
> > > Therefore, use kmap_local_page() / kunmap_local() in zstd.c because 
in 
> this
> > > file the mappings are per thread and are not visible in other 
contexts. 
> In
> > > the meanwhile use plain page_address() on pages allocated with the 
> GFP_NOFS
> > > flag instead of calling kmap*() on them (since they are always 
> allocated
> > > from ZONE_NORMAL).
> > > 
> > > Tested with xfstests on QEMU + KVM 32 bits VM with 4GB of RAM and
> > > booting a kernel with HIGHMEM64G enabled.
> > > 
> > > Cc: Filipe Manana <fdmanana@kernel.org>
> > > Suggested-by: Ira Weiny <ira.weiny@intel.com>
> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> > > ---
> > >  fs/btrfs/zstd.c | 34 ++++++++++++++--------------------
> > >  1 file changed, 14 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/fs/btrfs/zstd.c b/fs/btrfs/zstd.c
> > > index 0fe31a6f6e68..78e0272e770e 100644
> > > --- a/fs/btrfs/zstd.c
> > > +++ b/fs/btrfs/zstd.c
> > > @@ -403,7 +403,7 @@ int zstd_compress_pages(struct list_head *ws, 
> struct address_space *mapping,
> > >  
> > >  	/* map in the first page of input data */
> > >  	in_page = find_get_page(mapping, start >> PAGE_SHIFT);
> > > -	workspace->in_buf.src = kmap(in_page);
> > > +	workspace->in_buf.src = kmap_local_page(in_page);
> > >  	workspace->in_buf.pos = 0;
> > >  	workspace->in_buf.size = min_t(size_t, len, PAGE_SIZE);
> > >  
> > > @@ -415,7 +415,7 @@ int zstd_compress_pages(struct list_head *ws, 
> struct address_space *mapping,
> > >  		goto out;
> > >  	}
> > >  	pages[nr_pages++] = out_page;
> > > -	workspace->out_buf.dst = kmap(out_page);
> > > +	workspace->out_buf.dst = page_address(out_page);
> > >  	workspace->out_buf.pos = 0;
> > >  	workspace->out_buf.size = min_t(size_t, max_out, PAGE_SIZE);
> > >  
> > > @@ -450,9 +450,7 @@ int zstd_compress_pages(struct list_head *ws, 
> struct address_space *mapping,
> > >  		if (workspace->out_buf.pos == workspace->out_buf.size) 
> {
> > >  			tot_out += PAGE_SIZE;
> > >  			max_out -= PAGE_SIZE;
> > > -			kunmap(out_page);
> > >  			if (nr_pages == nr_dest_pages) {
> > > -				out_page = NULL;
> > >  				ret = -E2BIG;
> > >  				goto out;
> > >  			}
> > > @@ -462,7 +460,7 @@ int zstd_compress_pages(struct list_head *ws, 
> struct address_space *mapping,
> > >  				goto out;
> > >  			}
> > >  			pages[nr_pages++] = out_page;
> > > -			workspace->out_buf.dst = kmap(out_page);
> > > +			workspace->out_buf.dst = 
> page_address(out_page);
> > >  			workspace->out_buf.pos = 0;
> > >  			workspace->out_buf.size = min_t(size_t, 
> max_out,
> > >  							
> PAGE_SIZE);
> > > @@ -477,15 +475,15 @@ int zstd_compress_pages(struct list_head *ws, 
> struct address_space *mapping,
> > >  		/* Check if we need more input */
> > >  		if (workspace->in_buf.pos == workspace->in_buf.size) {
> > >  			tot_in += PAGE_SIZE;
> > > -			kunmap(in_page);
> > > +			kunmap_local(workspace->in_buf.src);
> > >  			put_page(in_page);
> > > -
> > >  			start += PAGE_SIZE;
> > >  			len -= PAGE_SIZE;
> > >  			in_page = find_get_page(mapping, start >> 
> PAGE_SHIFT);
> > > -			workspace->in_buf.src = kmap(in_page);
> > > +			workspace->in_buf.src = 
> kmap_local_page(in_page);
> > >  			workspace->in_buf.pos = 0;
> > >  			workspace->in_buf.size = min_t(size_t, len, 
> PAGE_SIZE);
> > > +			workspace->out_buf.dst = 
> page_address(out_page);
> > 
> > Why is this needed?
> 
> Sorry. This initialization is not needed at all.
> Probably made a mistake with copy-pasting snippets of code.
> 
> I'm going to send ASAP the fifth version of this series.

"fifth" -> "sixth".

> 
> > The rest looks good,
> 
> Thanks,
> 
> Fabio
> 
> > Ira
> > 
> > [snip]
> > 
> 
> 
> 
> 






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

end of thread, other threads:[~2022-07-06 10:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-04 15:23 [PATCH v5 0/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
2022-07-04 15:23 ` [PATCH v5 1/2] highmem: Make __kunmap_{local,atomic}() take "const void *" Fabio M. De Francesco
2022-07-04 15:23 ` [PATCH v5 2/2] btrfs: Replace kmap() with kmap_local_page() in zstd.c Fabio M. De Francesco
2022-07-05 19:04   ` Ira Weiny
2022-07-06 10:38     ` Fabio M. De Francesco
2022-07-06 10:48       ` Fabio M. De Francesco

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