| Message ID | 20220630163527.9776-1-fmdefrancesco@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 3a608cfee97e99b3fff9ffe62246a098042e725d |
| Headers | show |
| Series | fs: Replace kmap{,_atomic}() with kmap_local_page() | expand |
"Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes: > The use of kmap() and kmap_atomic() are being deprecated in favor of > kmap_local_page(). > > With kmap_local_page(), the mappings are per thread, CPU local and not > globally visible. Furthermore, the mappings can be acquired from any > context (including interrupts). > > Therefore, use kmap_local_page() in exec.c because these mappings are per > thread, CPU local, and not globally visible. > > Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with > HIGHMEM64GB enabled. Can someone please refresh my memory on what is going on. I remember there were limitations that kmap_atomic had that are hard to meet so something I think it was kmap_local was invented and created to be the kmap_atomic replacement. What are the requirements on kmap_local? In copy_strings kmap is called in contexts that can sleep in page faults so any nearly any requirement except a thread local use is invalidated. As you have described kmap_local above it does not sound like kmap_local is safe in this context, but that could just be a problem in description that my poor memory does is not recalling the necessary details to correct. Eric > Suggested-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > fs/exec.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 0989fb8472a1..4a2129c0d422 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > > if (kmapped_page) { > flush_dcache_page(kmapped_page); > - kunmap(kmapped_page); > + kunmap_local(kaddr); > put_arg_page(kmapped_page); > } > kmapped_page = page; > - kaddr = kmap(kmapped_page); > + kaddr = kmap_local_page(kmapped_page); > kpos = pos & PAGE_MASK; > flush_arg_page(bprm, kpos, kmapped_page); > } > @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > out: > if (kmapped_page) { > flush_dcache_page(kmapped_page); > - kunmap(kmapped_page); > + kunmap_local(kaddr); > put_arg_page(kmapped_page); > } > return ret; > @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm, > > for (index = MAX_ARG_PAGES - 1; index >= stop; index--) { > unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0; > - char *src = kmap(bprm->page[index]) + offset; > + char *src = kmap_local_page(bprm->page[index]) + offset; > sp -= PAGE_SIZE - offset; > if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0) > ret = -EFAULT; > - kunmap(bprm->page[index]); > + kunmap_local(src); > if (ret) > goto out; > } > @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm) > ret = -EFAULT; > goto out; > } > - kaddr = kmap_atomic(page); > + kaddr = kmap_local_page(page); > > for (; offset < PAGE_SIZE && kaddr[offset]; > offset++, bprm->p++) > ; > > - kunmap_atomic(kaddr); > + kunmap_local(kaddr); > put_arg_page(page); > } while (offset == PAGE_SIZE);
On giovedì 30 giugno 2022 19:38:08 CEST Eric W. Biederman wrote: > "Fabio M. De Francesco" <fmdefrancesco@gmail.com> writes: > > > The use of kmap() and kmap_atomic() are being deprecated in favor of > > kmap_local_page(). > > > > With kmap_local_page(), the mappings are per thread, CPU local and not > > globally visible. Furthermore, the mappings can be acquired from any > > context (including interrupts). > > > > Therefore, use kmap_local_page() in exec.c because these mappings are per > > thread, CPU local, and not globally visible. > > > > Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with > > HIGHMEM64GB enabled. > > Can someone please refresh my memory on what is going on. > > I remember there were limitations that kmap_atomic had that are hard to > meet so something I think it was kmap_local was invented and created > to be the kmap_atomic replacement. Please read highmem.rst. I've updated that document weeks ago: https://docs.kernel.org/vm/highmem.html?highlight=highmem Currently it contains many more information I can ever place here in order to answer your questions. Believe me, this is not by any means a way to elude your questions. I'm pretty sure that by reading that document you'll have a clear vision on what is going on :-) > > What are the requirements on kmap_local? In copy_strings > kmap is called in contexts that can sleep in page faults No problems with kmap_local_page() with regard to page faults (again, please read the above-mentioned document). From that document... "It’s valid to take pagefaults in a local kmap region []". "Each call of kmap_atomic() in the kernel creates a non-preemptible section and disable pagefaults. This could be a source of unwanted latency. Therefore users should prefer kmap_local_page() instead of kmap_atomic().". > so any > nearly any requirement except a thread local use is invalidated. > > As you have described kmap_local above it does not sound like kmap_local > is safe in this context, Sorry, probably I should add that taking page faults is allowed. Would you prefer I send a v2 and add this information? Thanks, Fabio > but that could just be a problem in description > that my poor memory does is not recalling the necessary details to > correct. > > Eric > > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > > --- > > fs/exec.c | 14 +++++++------- > > 1 file changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/fs/exec.c b/fs/exec.c > > index 0989fb8472a1..4a2129c0d422 100644 > > --- a/fs/exec.c > > +++ b/fs/exec.c > > @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > > > > if (kmapped_page) { > > flush_dcache_page(kmapped_page); > > - kunmap(kmapped_page); > > + kunmap_local(kaddr); > > put_arg_page(kmapped_page); > > } > > kmapped_page = page; > > - kaddr = kmap(kmapped_page); > > + kaddr = kmap_local_page(kmapped_page); > > kpos = pos & PAGE_MASK; > > flush_arg_page(bprm, kpos, kmapped_page); > > } > > @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > > out: > > if (kmapped_page) { > > flush_dcache_page(kmapped_page); > > - kunmap(kmapped_page); > > + kunmap_local(kaddr); > > put_arg_page(kmapped_page); > > } > > return ret; > > @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm, > > > > for (index = MAX_ARG_PAGES - 1; index >= stop; index--) { > > unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0; > > - char *src = kmap(bprm->page[index]) + offset; > > + char *src = kmap_local_page(bprm->page[index]) + offset; > > sp -= PAGE_SIZE - offset; > > if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0) > > ret = -EFAULT; > > - kunmap(bprm->page[index]); > > + kunmap_local(src); > > if (ret) > > goto out; > > } > > @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm) > > ret = -EFAULT; > > goto out; > > } > > - kaddr = kmap_atomic(page); > > + kaddr = kmap_local_page(page); > > > > for (; offset < PAGE_SIZE && kaddr[offset]; > > offset++, bprm->p++) > > ; > > > > - kunmap_atomic(kaddr); > > + kunmap_local(kaddr); > > put_arg_page(page); > > } while (offset == PAGE_SIZE); >
On Thu, Jun 30, 2022 at 06:35:27PM +0200, Fabio M. De Francesco wrote: > The use of kmap() and kmap_atomic() are being deprecated in favor of > kmap_local_page(). > > With kmap_local_page(), the mappings are per thread, CPU local and not > globally visible. Furthermore, the mappings can be acquired from any > context (including interrupts). > > Therefore, use kmap_local_page() in exec.c because these mappings are per > thread, CPU local, and not globally visible. > > Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with > HIGHMEM64GB enabled. > > Suggested-by: Ira Weiny <ira.weiny@intel.com> This looks good but there is a kmap_atomic() in this file which I _think_ can be converted as well. But that is good as a separate patch. Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> > --- > fs/exec.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) > > diff --git a/fs/exec.c b/fs/exec.c > index 0989fb8472a1..4a2129c0d422 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > > if (kmapped_page) { > flush_dcache_page(kmapped_page); > - kunmap(kmapped_page); > + kunmap_local(kaddr); > put_arg_page(kmapped_page); > } > kmapped_page = page; > - kaddr = kmap(kmapped_page); > + kaddr = kmap_local_page(kmapped_page); > kpos = pos & PAGE_MASK; > flush_arg_page(bprm, kpos, kmapped_page); > } > @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv, > out: > if (kmapped_page) { > flush_dcache_page(kmapped_page); > - kunmap(kmapped_page); > + kunmap_local(kaddr); > put_arg_page(kmapped_page); > } > return ret; > @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm, > > for (index = MAX_ARG_PAGES - 1; index >= stop; index--) { > unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0; > - char *src = kmap(bprm->page[index]) + offset; > + char *src = kmap_local_page(bprm->page[index]) + offset; > sp -= PAGE_SIZE - offset; > if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0) > ret = -EFAULT; > - kunmap(bprm->page[index]); > + kunmap_local(src); > if (ret) > goto out; > } > @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm) > ret = -EFAULT; > goto out; > } > - kaddr = kmap_atomic(page); > + kaddr = kmap_local_page(page); > > for (; offset < PAGE_SIZE && kaddr[offset]; > offset++, bprm->p++) > ; > > - kunmap_atomic(kaddr); > + kunmap_local(kaddr); > put_arg_page(page); > } while (offset == PAGE_SIZE); > > -- > 2.36.1 >
On venerdì 8 luglio 2022 22:18:35 CEST Ira Weiny wrote: > On Thu, Jun 30, 2022 at 06:35:27PM +0200, Fabio M. De Francesco wrote: > > The use of kmap() and kmap_atomic() are being deprecated in favor of > > kmap_local_page(). > > > > With kmap_local_page(), the mappings are per thread, CPU local and not > > globally visible. Furthermore, the mappings can be acquired from any > > context (including interrupts). > > > > Therefore, use kmap_local_page() in exec.c because these mappings are per > > thread, CPU local, and not globally visible. > > > > Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with > > HIGHMEM64GB enabled. > > > > Suggested-by: Ira Weiny <ira.weiny@intel.com> > > This looks good but there is a kmap_atomic() in this file which I _think_ can > be converted as well. But that is good as a separate patch. > > Reviewed-by: Ira Weiny <ira.weiny@intel.com> > Thanks for your review! I didn't notice that kmap_atomic(). I'll send a conversion with a separate patch. Fabio
diff --git a/fs/exec.c b/fs/exec.c index 0989fb8472a1..4a2129c0d422 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -583,11 +583,11 @@ static int copy_strings(int argc, struct user_arg_ptr argv, if (kmapped_page) { flush_dcache_page(kmapped_page); - kunmap(kmapped_page); + kunmap_local(kaddr); put_arg_page(kmapped_page); } kmapped_page = page; - kaddr = kmap(kmapped_page); + kaddr = kmap_local_page(kmapped_page); kpos = pos & PAGE_MASK; flush_arg_page(bprm, kpos, kmapped_page); } @@ -601,7 +601,7 @@ static int copy_strings(int argc, struct user_arg_ptr argv, out: if (kmapped_page) { flush_dcache_page(kmapped_page); - kunmap(kmapped_page); + kunmap_local(kaddr); put_arg_page(kmapped_page); } return ret; @@ -883,11 +883,11 @@ int transfer_args_to_stack(struct linux_binprm *bprm, for (index = MAX_ARG_PAGES - 1; index >= stop; index--) { unsigned int offset = index == stop ? bprm->p & ~PAGE_MASK : 0; - char *src = kmap(bprm->page[index]) + offset; + char *src = kmap_local_page(bprm->page[index]) + offset; sp -= PAGE_SIZE - offset; if (copy_to_user((void *) sp, src, PAGE_SIZE - offset) != 0) ret = -EFAULT; - kunmap(bprm->page[index]); + kunmap_local(src); if (ret) goto out; } @@ -1680,13 +1680,13 @@ int remove_arg_zero(struct linux_binprm *bprm) ret = -EFAULT; goto out; } - kaddr = kmap_atomic(page); + kaddr = kmap_local_page(page); for (; offset < PAGE_SIZE && kaddr[offset]; offset++, bprm->p++) ; - kunmap_atomic(kaddr); + kunmap_local(kaddr); put_arg_page(page); } while (offset == PAGE_SIZE);
The use of kmap() and kmap_atomic() are being deprecated in favor of kmap_local_page(). With kmap_local_page(), the mappings are per thread, CPU local and not globally visible. Furthermore, the mappings can be acquired from any context (including interrupts). Therefore, use kmap_local_page() in exec.c because these mappings are per thread, CPU local, and not globally visible. Tested with xfstests on a QEMU + KVM 32-bits VM booting a kernel with HIGHMEM64GB enabled. Suggested-by: Ira Weiny <ira.weiny@intel.com> Signed-off-by: Fabio M. De Francesco <fmdefrancesco@gmail.com> --- fs/exec.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)