| Message ID | 20220818224010.43778-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | fs-verity: use kmap_local_page() instead of kmap() | expand |
On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Convert the use of kmap() to its recommended replacement > kmap_local_page(). This avoids the overhead of doing a non-local > mapping, which is unnecessary in this case. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/verity/read_metadata.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) It looks good to me... Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> Thanks, Fabio > diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c > index 6ee849dc7bc183..2aefc5565152ad 100644 > --- a/fs/verity/read_metadata.c > +++ b/fs/verity/read_metadata.c > @@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode *inode, > break; > } > > - virt = kmap(page); > + virt = kmap_local_page(page); > if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) { > - kunmap(page); > + kunmap_local(virt); > put_page(page); > err = -EFAULT; > break; > } > - kunmap(page); > + kunmap_local(virt); > put_page(page); > > retval += bytes_to_copy; > > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868 > prerequisite-patch-id: 188e114bdf3546eb18e7984b70be8a7c773acec3 > -- > 2.37.1
On Friday, August 19, 2022 12:40:10 AM CEST Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Convert the use of kmap() to its recommended replacement > kmap_local_page(). This avoids the overhead of doing a non-local > mapping, which is unnecessary in this case. > > Signed-off-by: Eric Biggers <ebiggers@google.com> > --- > fs/verity/read_metadata.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > It looks good to me... Reviewed-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> Thanks, Fabio > diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c > index 6ee849dc7bc183..2aefc5565152ad 100644 > --- a/fs/verity/read_metadata.c > +++ b/fs/verity/read_metadata.c > @@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode *inode, > break; > } > > - virt = kmap(page); > + virt = kmap_local_page(page); > if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) { > - kunmap(page); > + kunmap_local(virt); > put_page(page); > err = -EFAULT; > break; > } > - kunmap(page); > + kunmap_local(virt); > put_page(page); > > retval += bytes_to_copy; > > base-commit: 568035b01cfb107af8d2e4bd2fb9aea22cf5b868 > prerequisite-patch-id: 188e114bdf3546eb18e7984b70be8a7c773acec3 > -- > 2.37.1 > >
On Fri, Aug 19, 2022 at 09:50:37AM +0200, Fabio M. De Francesco wrote: > On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > Convert the use of kmap() to its recommended replacement > > kmap_local_page(). This avoids the overhead of doing a non-local > > mapping, which is unnecessary in this case. > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > --- > > fs/verity/read_metadata.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > It looks good to me... > > > - virt = kmap(page); > > + virt = kmap_local_page(page); > > if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) > { > > - kunmap(page); > > + kunmap_local(virt); > > put_page(page); > > err = -EFAULT; > > break; > > } > > - kunmap(page); > > + kunmap_local(virt); Is this a common pattern? eg do we want something like: static inline int copy_user_page(void __user *dst, struct page *page, size_t offset, size_t len) { char *src = kmap_local_page(page) + offset; int err = 0; VM_BUG_ON(offset + len > PAGE_SIZE); if (copy_to_user(dst, src, len)) err = -EFAULT; kunmap_local(src); return err; } in highmem.h?
On venerdì 19 agosto 2022 20:29:31 CEST Matthew Wilcox wrote: > On Fri, Aug 19, 2022 at 09:50:37AM +0200, Fabio M. De Francesco wrote: > > On venerdì 19 agosto 2022 00:40:10 CEST Eric Biggers wrote: > > > From: Eric Biggers <ebiggers@google.com> > > > > > > Convert the use of kmap() to its recommended replacement > > > kmap_local_page(). This avoids the overhead of doing a non-local > > > mapping, which is unnecessary in this case. > > > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > > > --- > > > > > > fs/verity/read_metadata.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > It looks good to me... > > > > > - virt = kmap(page); > > > + virt = kmap_local_page(page); > > > > > > if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) > > > > { > > > > > - kunmap(page); > > > + kunmap_local(virt); > > > > > > put_page(page); > > > err = -EFAULT; > > > break; > > > > > > } > > > > > > - kunmap(page); > > > + kunmap_local(virt); > > Is this a common pattern? eg do we want something like: > > static inline int copy_user_page(void __user *dst, struct page *page, > size_t offset, size_t len) > { > char *src = kmap_local_page(page) + offset; > int err = 0; > > VM_BUG_ON(offset + len > PAGE_SIZE); > if (copy_to_user(dst, src, len)) > err = -EFAULT; > > kunmap_local(src); > return err; > } > > in highmem.h? Not sure that it is much common... Can the following command provide any insight? opensuse:/usr/src/git/kernels/linux> grep -rn copy_to_user -B7 . --exclude- dir\=Documentation | grep kmap ./drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c-2306- ptr = kmap_local_page(p); ./drivers/gpu/drm/i915/i915_gem.c-215- vaddr = kmap_local_page(page); ./drivers/gpu/drm/radeon/radeon_ttm.c-872- ptr = kmap(page); ./drivers/vfio/pci/mlx5/main.c-182- from_buff = kmap_local_page(page); ./arch/parisc/kernel/cache.c-580- kto = kmap_local_page(to); ./mm/memory.c-5474- maddr = kmap(page); ./fs/verity/read_metadata.c-56- virt = kmap(page); ./fs/aio.c-1252- ev = kmap_local_page(page); ./fs/exec.c-883- char *src = kmap_local_page(bprm->page[index]) + offset; If this command is good to provide any hint, it suggests that having copy_to_user() in highmem.h is not probably worth. Thanks, Fabio
diff --git a/fs/verity/read_metadata.c b/fs/verity/read_metadata.c index 6ee849dc7bc183..2aefc5565152ad 100644 --- a/fs/verity/read_metadata.c +++ b/fs/verity/read_metadata.c @@ -53,14 +53,14 @@ static int fsverity_read_merkle_tree(struct inode *inode, break; } - virt = kmap(page); + virt = kmap_local_page(page); if (copy_to_user(buf, virt + offs_in_page, bytes_to_copy)) { - kunmap(page); + kunmap_local(virt); put_page(page); err = -EFAULT; break; } - kunmap(page); + kunmap_local(virt); put_page(page); retval += bytes_to_copy;