* Re: [PATCH v5 3/6] mm/gup: remove vmas parameter from get_user_pages_remote()
2023-05-16 9:49 ` Anders Roxell
@ 2023-05-16 18:37 ` Lorenzo Stoakes
2023-05-16 22:02 ` Andrew Morton
1 sibling, 0 replies; 4+ messages in thread
From: Lorenzo Stoakes @ 2023-05-16 18:37 UTC (permalink / raw)
To: Anders Roxell
Cc: linux-mm, linux-kernel, Andrew Morton, Matthew Wilcox,
David Hildenbrand, linux-arm-kernel, kvm, linux-s390,
linux-fsdevel, linux-perf-users, linux-security-module,
Catalin Marinas, Will Deacon, Christian Borntraeger,
Janosch Frank, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Sven Schnelle, Eric Biederman, Kees Cook,
Alexander Viro, Christian Brauner, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter,
Kentaro Takeda, Tetsuo Handa, Paul Moore, James Morris,
Serge E . Hallyn, Paolo Bonzini, Jens Axboe, Pavel Begunkov,
Jason Gunthorpe, John Hubbard
On Tue, May 16, 2023 at 11:49:19AM +0200, Anders Roxell wrote:
> On 2023-05-14 22:26, Lorenzo Stoakes wrote:
> > The only instances of get_user_pages_remote() invocations which used the
> > vmas parameter were for a single page which can instead simply look up the
> > VMA directly. In particular:-
> >
> > - __update_ref_ctr() looked up the VMA but did nothing with it so we simply
> > remove it.
> >
> > - __access_remote_vm() was already using vma_lookup() when the original
> > lookup failed so by doing the lookup directly this also de-duplicates the
> > code.
> >
> > We are able to perform these VMA operations as we already hold the
> > mmap_lock in order to be able to call get_user_pages_remote().
> >
> > As part of this work we add get_user_page_vma_remote() which abstracts the
> > VMA lookup, error handling and decrementing the page reference count should
> > the VMA lookup fail.
> >
> > This forms part of a broader set of patches intended to eliminate the vmas
> > parameter altogether.
> >
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com> (for arm64)
> > Acked-by: David Hildenbrand <david@redhat.com>
> > Reviewed-by: Janosch Frank <frankja@linux.ibm.com> (for s390)
> > Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
> > ---
> > arch/arm64/kernel/mte.c | 17 +++++++++--------
> > arch/s390/kvm/interrupt.c | 2 +-
> > fs/exec.c | 2 +-
> > include/linux/mm.h | 34 +++++++++++++++++++++++++++++++---
> > kernel/events/uprobes.c | 13 +++++--------
> > mm/gup.c | 12 ++++--------
> > mm/memory.c | 14 +++++++-------
> > mm/rmap.c | 2 +-
> > security/tomoyo/domain.c | 2 +-
> > virt/kvm/async_pf.c | 3 +--
> > 10 files changed, 61 insertions(+), 40 deletions(-)
> >
>
> [...]
>
> > diff --git a/mm/memory.c b/mm/memory.c
> > index 146bb94764f8..63632a5eafc1 100644
> > --- a/mm/memory.c
> > +++ b/mm/memory.c
> > @@ -5590,7 +5590,6 @@ EXPORT_SYMBOL_GPL(generic_access_phys);
> > int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> > int len, unsigned int gup_flags)
> > {
> > - struct vm_area_struct *vma;
> > void *old_buf = buf;
> > int write = gup_flags & FOLL_WRITE;
> >
> > @@ -5599,13 +5598,15 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> >
> > /* ignore errors, just check how much was successfully transferred */
> > while (len) {
> > - int bytes, ret, offset;
> > + int bytes, offset;
> > void *maddr;
> > - struct page *page = NULL;
> > + struct vm_area_struct *vma;
> > + struct page *page = get_user_page_vma_remote(mm, addr,
> > + gup_flags, &vma);
> > +
> > + if (IS_ERR_OR_NULL(page)) {
> > + int ret = 0;
>
> I see the warning below when building without CONFIG_HAVE_IOREMAP_PROT set.
>
> make --silent --keep-going --jobs=32 \
> O=/home/anders/.cache/tuxmake/builds/1244/build ARCH=arm \
> CROSS_COMPILE=arm-linux-gnueabihf- /home/anders/src/kernel/next/mm/memory.c: In function '__access_remote_vm':
> /home/anders/src/kernel/next/mm/memory.c:5608:29: warning: unused variable 'ret' [-Wunused-variable]
> 5608 | int ret = 0;
> | ^~~
>
Ah damn, nice spot thanks!
>
> >
> > - ret = get_user_pages_remote(mm, addr, 1,
> > - gup_flags, &page, &vma, NULL);
> > - if (ret <= 0) {
> > #ifndef CONFIG_HAVE_IOREMAP_PROT
> > break;
> > #else
> > @@ -5613,7 +5614,6 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
> > * Check if this is a VM_IO | VM_PFNMAP VMA, which
> > * we can access using slightly different code.
> > */
> > - vma = vma_lookup(mm, addr);
> > if (!vma)
> > break;
> > if (vma->vm_ops && vma->vm_ops->access)
>
> Cheers,
> Anders
I enclose a -fix patch for this below:-
----8<----
From 6a4bb033a1ec60920e4945e7e063443f91489d06 Mon Sep 17 00:00:00 2001
From: Lorenzo Stoakes <lstoakes@gmail.com>
Date: Tue, 16 May 2023 19:16:22 +0100
Subject: [PATCH] mm/gup: remove vmas parameter from get_user_pages_remote()
Fix unused variable warning as reported by Anders Roxell.
Signed-off-by: Lorenzo Stoakes <lstoakes@gmail.com>
---
mm/memory.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/memory.c b/mm/memory.c
index 63632a5eafc1..b1b25e61294a 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -5605,11 +5605,11 @@ int __access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf,
gup_flags, &vma);
if (IS_ERR_OR_NULL(page)) {
- int ret = 0;
-
#ifndef CONFIG_HAVE_IOREMAP_PROT
break;
#else
+ int ret = 0;
+
/*
* Check if this is a VM_IO | VM_PFNMAP VMA, which
* we can access using slightly different code.
--
2.40.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH v5 3/6] mm/gup: remove vmas parameter from get_user_pages_remote()
2023-05-16 9:49 ` Anders Roxell
2023-05-16 18:37 ` Lorenzo Stoakes
@ 2023-05-16 22:02 ` Andrew Morton
1 sibling, 0 replies; 4+ messages in thread
From: Andrew Morton @ 2023-05-16 22:02 UTC (permalink / raw)
To: Anders Roxell
Cc: Lorenzo Stoakes, linux-mm, linux-kernel, Matthew Wilcox,
David Hildenbrand, linux-arm-kernel, kvm, linux-s390,
linux-fsdevel, linux-perf-users, linux-security-module,
Catalin Marinas, Will Deacon, Christian Borntraeger,
Janosch Frank, Claudio Imbrenda, Heiko Carstens, Vasily Gorbik,
Alexander Gordeev, Sven Schnelle, Eric Biederman, Kees Cook,
Alexander Viro, Christian Brauner, Peter Zijlstra, Ingo Molnar,
Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, Ian Rogers, Adrian Hunter,
Kentaro Takeda, Tetsuo Handa, Paul Moore, James Morris,
Serge E . Hallyn, Paolo Bonzini, Jens Axboe, Pavel Begunkov,
Jason Gunthorpe, John Hubbard
On Tue, 16 May 2023 11:49:19 +0200 Anders Roxell <anders.roxell@linaro.org> wrote:
> On 2023-05-14 22:26, Lorenzo Stoakes wrote:
> > The only instances of get_user_pages_remote() invocations which used the
> > vmas parameter were for a single page which can instead simply look up the
> > VMA directly. In particular:-
> >
> > - __update_ref_ctr() looked up the VMA but did nothing with it so we simply
> > remove it.
> >
> > - __access_remote_vm() was already using vma_lookup() when the original
> > lookup failed so by doing the lookup directly this also de-duplicates the
> > code.
> >
> > We are able to perform these VMA operations as we already hold the
> > mmap_lock in order to be able to call get_user_pages_remote().
> >
> > As part of this work we add get_user_page_vma_remote() which abstracts the
> > VMA lookup, error handling and decrementing the page reference count should
> > the VMA lookup fail.
> >
> > This forms part of a broader set of patches intended to eliminate the vmas
> > parameter altogether.
> >
> > - int bytes, ret, offset;
> > + int bytes, offset;
> > void *maddr;
> > - struct page *page = NULL;
> > + struct vm_area_struct *vma;
> > + struct page *page = get_user_page_vma_remote(mm, addr,
> > + gup_flags, &vma);
> > +
> > + if (IS_ERR_OR_NULL(page)) {
> > + int ret = 0;
>
> I see the warning below when building without CONFIG_HAVE_IOREMAP_PROT set.
>
> make --silent --keep-going --jobs=32 \
> O=/home/anders/.cache/tuxmake/builds/1244/build ARCH=arm \
> CROSS_COMPILE=arm-linux-gnueabihf- /home/anders/src/kernel/next/mm/memory.c: In function '__access_remote_vm':
> /home/anders/src/kernel/next/mm/memory.c:5608:29: warning: unused variable 'ret' [-Wunused-variable]
> 5608 | int ret = 0;
> | ^~~
Thanks, I did the obvious.
Also s/ret/res/, as `ret' is kinda reserved for "this is what this
function will return".
--- a/mm/memory.c~mm-gup-remove-vmas-parameter-from-get_user_pages_remote-fix
+++ a/mm/memory.c
@@ -5605,11 +5605,11 @@ int __access_remote_vm(struct mm_struct
gup_flags, &vma);
if (IS_ERR_OR_NULL(page)) {
- int ret = 0;
-
#ifndef CONFIG_HAVE_IOREMAP_PROT
break;
#else
+ int res = 0;
+
/*
* Check if this is a VM_IO | VM_PFNMAP VMA, which
* we can access using slightly different code.
@@ -5617,11 +5617,11 @@ int __access_remote_vm(struct mm_struct
if (!vma)
break;
if (vma->vm_ops && vma->vm_ops->access)
- ret = vma->vm_ops->access(vma, addr, buf,
+ res = vma->vm_ops->access(vma, addr, buf,
len, write);
- if (ret <= 0)
+ if (res <= 0)
break;
- bytes = ret;
+ bytes = res;
#endif
} else {
bytes = len;
_
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 4+ messages in thread