* Re: [PATCH] fs/cramfs: Convert kmap() to kmap_local_data()
2022-10-12 9:13 [PATCH] fs/cramfs: Convert kmap() to kmap_local_data() Fabio M. De Francesco
@ 2022-10-12 18:54 ` Nicolas Pitre
2022-10-17 6:35 ` Christian Brauner
2022-12-30 22:31 ` Ira Weiny
2 siblings, 0 replies; 5+ messages in thread
From: Nicolas Pitre @ 2022-10-12 18:54 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Christian Brauner, linux-kernel, bpf, Venkataramanan, Anirudh,
Ira Weiny
[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]
On Wed, 12 Oct 2022, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page().
>
> There are two main problems with kmap(): (1) It comes with an overhead as
> the mapping space is restricted and protected by a global lock for
> synchronization and (2) it also requires global TLB invalidation when the
> kmap’s pool wraps and it might block when the mapping space is fully
> utilized until a slot becomes available.
>
> With kmap_local_page() the mappings are per thread, CPU local, can take
> page faults, and can be called from any context (including interrupts).
> It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> the tasks can be preempted and, when they are scheduled to run again, the
> kernel virtual addresses are restored and still valid.
>
> Since its use in fs/cramfs is safe everywhere, it should be preferred.
>
> Therefore, replace kmap() with kmap_local_page() in fs/cramfs. Instead
> of open-coding kmap_local_page() + memcpy(), use memcpy_from_page().
>
> Cc: "Venkataramanan, Anirudh" <anirudh.venkataramanan@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
Reviewed-by: Nicolas Pitre <nico@fluxnic.net>
> ---
> fs/cramfs/inode.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 61ccf7722fc3..c17cbba5d45e 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -238,8 +238,7 @@ static void *cramfs_blkdev_read(struct super_block *sb, unsigned int offset,
> struct page *page = pages[i];
>
> if (page) {
> - memcpy(data, kmap(page), PAGE_SIZE);
> - kunmap(page);
> + memcpy_from_page(data, page, 0, PAGE_SIZE);
> put_page(page);
> } else
> memset(data, 0, PAGE_SIZE);
> @@ -815,7 +814,7 @@ static int cramfs_read_folio(struct file *file, struct folio *folio)
>
> maxblock = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> bytes_filled = 0;
> - pgdata = kmap(page);
> + pgdata = kmap_local_page(page);
>
> if (page->index < maxblock) {
> struct super_block *sb = inode->i_sb;
> @@ -903,13 +902,13 @@ static int cramfs_read_folio(struct file *file, struct folio *folio)
>
> memset(pgdata + bytes_filled, 0, PAGE_SIZE - bytes_filled);
> flush_dcache_page(page);
> - kunmap(page);
> + kunmap_local(pgdata);
> SetPageUptodate(page);
> unlock_page(page);
> return 0;
>
> err:
> - kunmap(page);
> + kunmap_local(pgdata);
> ClearPageUptodate(page);
> SetPageError(page);
> unlock_page(page);
> --
> 2.37.3
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fs/cramfs: Convert kmap() to kmap_local_data()
2022-10-12 9:13 [PATCH] fs/cramfs: Convert kmap() to kmap_local_data() Fabio M. De Francesco
2022-10-12 18:54 ` Nicolas Pitre
@ 2022-10-17 6:35 ` Christian Brauner
2022-12-30 22:31 ` Ira Weiny
2 siblings, 0 replies; 5+ messages in thread
From: Christian Brauner @ 2022-10-17 6:35 UTC (permalink / raw)
To: Fabio M. De Francesco, H
Cc: Nicolas Pitre, linux-kernel, bpf, Venkataramanan, Anirudh,
Ira Weiny
On Wed, Oct 12, 2022 at 11:13:56AM +0200, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page().
>
> There are two main problems with kmap(): (1) It comes with an overhead as
> the mapping space is restricted and protected by a global lock for
> synchronization and (2) it also requires global TLB invalidation when the
> kmap’s pool wraps and it might block when the mapping space is fully
> utilized until a slot becomes available.
>
> With kmap_local_page() the mappings are per thread, CPU local, can take
> page faults, and can be called from any context (including interrupts).
> It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> the tasks can be preempted and, when they are scheduled to run again, the
> kernel virtual addresses are restored and still valid.
>
> Since its use in fs/cramfs is safe everywhere, it should be preferred.
>
> Therefore, replace kmap() with kmap_local_page() in fs/cramfs. Instead
> of open-coding kmap_local_page() + memcpy(), use memcpy_from_page().
>
> Cc: "Venkataramanan, Anirudh" <anirudh.venkataramanan@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
Looks good to me,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] fs/cramfs: Convert kmap() to kmap_local_data()
2022-10-12 9:13 [PATCH] fs/cramfs: Convert kmap() to kmap_local_data() Fabio M. De Francesco
2022-10-12 18:54 ` Nicolas Pitre
2022-10-17 6:35 ` Christian Brauner
@ 2022-12-30 22:31 ` Ira Weiny
2022-12-31 20:55 ` Nicolas Pitre
2 siblings, 1 reply; 5+ messages in thread
From: Ira Weiny @ 2022-12-30 22:31 UTC (permalink / raw)
To: Fabio M. De Francesco
Cc: Nicolas Pitre, Christian Brauner, linux-kernel, bpf,
Venkataramanan, Anirudh
On Wed, Oct 12, 2022 at 11:13:56AM +0200, Fabio M. De Francesco wrote:
> The use of kmap() is being deprecated in favor of kmap_local_page().
>
> There are two main problems with kmap(): (1) It comes with an overhead as
> the mapping space is restricted and protected by a global lock for
> synchronization and (2) it also requires global TLB invalidation when the
> kmap’s pool wraps and it might block when the mapping space is fully
> utilized until a slot becomes available.
>
> With kmap_local_page() the mappings are per thread, CPU local, can take
> page faults, and can be called from any context (including interrupts).
> It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> the tasks can be preempted and, when they are scheduled to run again, the
> kernel virtual addresses are restored and still valid.
>
> Since its use in fs/cramfs is safe everywhere, it should be preferred.
>
> Therefore, replace kmap() with kmap_local_page() in fs/cramfs. Instead
> of open-coding kmap_local_page() + memcpy(), use memcpy_from_page().
>
> Cc: "Venkataramanan, Anirudh" <anirudh.venkataramanan@intel.com>
> Suggested-by: Ira Weiny <ira.weiny@intel.com>
Nicolas, I was auditing the kmap conversion status and I see you reviewed this
but did not merge it. Are you still the maintainer for fs/cramfs? Should
Fabio send this to someone else?
FWIW LGTM
Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com>
> ---
> fs/cramfs/inode.c | 9 ++++-----
> 1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cramfs/inode.c b/fs/cramfs/inode.c
> index 61ccf7722fc3..c17cbba5d45e 100644
> --- a/fs/cramfs/inode.c
> +++ b/fs/cramfs/inode.c
> @@ -238,8 +238,7 @@ static void *cramfs_blkdev_read(struct super_block *sb, unsigned int offset,
> struct page *page = pages[i];
>
> if (page) {
> - memcpy(data, kmap(page), PAGE_SIZE);
> - kunmap(page);
> + memcpy_from_page(data, page, 0, PAGE_SIZE);
> put_page(page);
> } else
> memset(data, 0, PAGE_SIZE);
> @@ -815,7 +814,7 @@ static int cramfs_read_folio(struct file *file, struct folio *folio)
>
> maxblock = (inode->i_size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> bytes_filled = 0;
> - pgdata = kmap(page);
> + pgdata = kmap_local_page(page);
>
> if (page->index < maxblock) {
> struct super_block *sb = inode->i_sb;
> @@ -903,13 +902,13 @@ static int cramfs_read_folio(struct file *file, struct folio *folio)
>
> memset(pgdata + bytes_filled, 0, PAGE_SIZE - bytes_filled);
> flush_dcache_page(page);
> - kunmap(page);
> + kunmap_local(pgdata);
> SetPageUptodate(page);
> unlock_page(page);
> return 0;
>
> err:
> - kunmap(page);
> + kunmap_local(pgdata);
> ClearPageUptodate(page);
> SetPageError(page);
> unlock_page(page);
> --
> 2.37.3
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] fs/cramfs: Convert kmap() to kmap_local_data()
2022-12-30 22:31 ` Ira Weiny
@ 2022-12-31 20:55 ` Nicolas Pitre
0 siblings, 0 replies; 5+ messages in thread
From: Nicolas Pitre @ 2022-12-31 20:55 UTC (permalink / raw)
To: Ira Weiny
Cc: Fabio M. De Francesco, Christian Brauner, linux-kernel, bpf,
Venkataramanan, Anirudh
[-- Attachment #1: Type: text/plain, Size: 1548 bytes --]
On Fri, 30 Dec 2022, Ira Weiny wrote:
> On Wed, Oct 12, 2022 at 11:13:56AM +0200, Fabio M. De Francesco wrote:
> > The use of kmap() is being deprecated in favor of kmap_local_page().
> >
> > There are two main problems with kmap(): (1) It comes with an overhead as
> > the mapping space is restricted and protected by a global lock for
> > synchronization and (2) it also requires global TLB invalidation when the
> > kmap’s pool wraps and it might block when the mapping space is fully
> > utilized until a slot becomes available.
> >
> > With kmap_local_page() the mappings are per thread, CPU local, can take
> > page faults, and can be called from any context (including interrupts).
> > It is faster than kmap() in kernels with HIGHMEM enabled. Furthermore,
> > the tasks can be preempted and, when they are scheduled to run again, the
> > kernel virtual addresses are restored and still valid.
> >
> > Since its use in fs/cramfs is safe everywhere, it should be preferred.
> >
> > Therefore, replace kmap() with kmap_local_page() in fs/cramfs. Instead
> > of open-coding kmap_local_page() + memcpy(), use memcpy_from_page().
> >
> > Cc: "Venkataramanan, Anirudh" <anirudh.venkataramanan@intel.com>
> > Suggested-by: Ira Weiny <ira.weiny@intel.com>
>
> Nicolas, I was auditing the kmap conversion status and I see you reviewed this
> but did not merge it. Are you still the maintainer for fs/cramfs? Should
> Fabio send this to someone else?
Please send it to Al Viro with my reviewed-by.
Nicolas
^ permalink raw reply [flat|nested] 5+ messages in thread