* Re: [RESEND 3/7] mm/gup: Change GUP fast to use flags rather than a write 'bool'
From: Ira Weiny @ 2019-03-25 8:26 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, linux-s390, Yoshinori Sato, linux-rdma,
Jason Gunthorpe, Ingo Molnar, John Hubbard, Borislav Petkov,
Thomas Gleixner, netdev@vger.kernel.org,
Linux Kernel Mailing List, Ralf Baechle, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <CAA9_cmd9gUWMbpkP_AuxZ08iqvZdxjbtDoR-FpSjAyhZJisRZA@mail.gmail.com>
On Fri, Mar 22, 2019 at 03:05:53PM -0700, Dan Williams wrote:
> On Sun, Mar 17, 2019 at 7:36 PM <ira.weiny@intel.com> wrote:
> >
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > To facilitate additional options to get_user_pages_fast() change the
> > singular write parameter to be gup_flags.
> >
> > This patch does not change any functionality. New functionality will
> > follow in subsequent patches.
> >
> > Some of the get_user_pages_fast() call sites were unchanged because they
> > already passed FOLL_WRITE or 0 for the write parameter.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> >
> > ---
> > Changes from V1:
> > Rebase to current merge tree
> > arch/powerpc/mm/mmu_context_iommu.c no longer calls gup_fast
> > The gup_longterm was converted in patch 1
> >
> > arch/mips/mm/gup.c | 11 ++++++-----
> > arch/powerpc/kvm/book3s_64_mmu_hv.c | 4 ++--
> > arch/powerpc/kvm/e500_mmu.c | 2 +-
> > arch/s390/kvm/interrupt.c | 2 +-
> > arch/s390/mm/gup.c | 12 ++++++------
> > arch/sh/mm/gup.c | 11 ++++++-----
> > arch/sparc/mm/gup.c | 9 +++++----
> > arch/x86/kvm/paging_tmpl.h | 2 +-
> > arch/x86/kvm/svm.c | 2 +-
> > drivers/fpga/dfl-afu-dma-region.c | 2 +-
> > drivers/gpu/drm/via/via_dmablit.c | 3 ++-
> > drivers/infiniband/hw/hfi1/user_pages.c | 3 ++-
> > drivers/misc/genwqe/card_utils.c | 2 +-
> > drivers/misc/vmw_vmci/vmci_host.c | 2 +-
> > drivers/misc/vmw_vmci/vmci_queue_pair.c | 6 ++++--
> > drivers/platform/goldfish/goldfish_pipe.c | 3 ++-
> > drivers/rapidio/devices/rio_mport_cdev.c | 4 +++-
> > drivers/sbus/char/oradax.c | 2 +-
> > drivers/scsi/st.c | 3 ++-
> > drivers/staging/gasket/gasket_page_table.c | 4 ++--
> > drivers/tee/tee_shm.c | 2 +-
> > drivers/vfio/vfio_iommu_spapr_tce.c | 3 ++-
> > drivers/vhost/vhost.c | 2 +-
> > drivers/video/fbdev/pvr2fb.c | 2 +-
> > drivers/virt/fsl_hypervisor.c | 2 +-
> > drivers/xen/gntdev.c | 2 +-
> > fs/orangefs/orangefs-bufmap.c | 2 +-
> > include/linux/mm.h | 4 ++--
> > kernel/futex.c | 2 +-
> > lib/iov_iter.c | 7 +++++--
> > mm/gup.c | 10 +++++-----
> > mm/util.c | 8 ++++----
> > net/ceph/pagevec.c | 2 +-
> > net/rds/info.c | 2 +-
> > net/rds/rdma.c | 3 ++-
> > 35 files changed, 79 insertions(+), 63 deletions(-)
>
>
> >
> > diff --git a/arch/mips/mm/gup.c b/arch/mips/mm/gup.c
> > index 0d14e0d8eacf..4c2b4483683c 100644
> > --- a/arch/mips/mm/gup.c
> > +++ b/arch/mips/mm/gup.c
> > @@ -235,7 +235,7 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > * get_user_pages_fast() - pin user pages in memory
> > * @start: starting user address
> > * @nr_pages: number of pages from start to pin
> > - * @write: whether pages will be written to
> > + * @gup_flags: flags modifying pin behaviour
> > * @pages: array that receives pointers to the pages pinned.
> > * Should be at least nr_pages long.
> > *
> > @@ -247,8 +247,8 @@ int __get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > * requested. If nr_pages is 0 or negative, returns 0. If no pages
> > * were pinned, returns -errno.
> > */
> > -int get_user_pages_fast(unsigned long start, int nr_pages, int write,
> > - struct page **pages)
> > +int get_user_pages_fast(unsigned long start, int nr_pages,
> > + unsigned int gup_flags, struct page **pages)
>
> This looks a tad scary given all related thrash especially when it's
> only 1 user that wants to do get_user_page_fast_longterm, right?
Agreed but the discussion back in Feb agreed that it would be better to make
get_user_pages_fast() use flags rather than add another *_longterm call, and I
agree.
> Maybe
> something like the following. Note I explicitly moved the flags to the
> end so that someone half paying attention that calls
> __get_user_pages_fast will get a compile error if they specify the
> args in the same order.
I did this to remain consistent with the other public calls. They put the
"return" pages parameter at the end. For example get_user_pages() is defined
this way with pages and vmas at the end.
long get_user_pages(unsigned long start, unsigned long nr_pages,
unsigned int gup_flags, struct page **pages,
struct vm_area_struct **vmas)
I'm pretty sure I got all the callers. Is this worth making the signature
"non-standard" WRT the other calls?
Ira
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 76ba638ceda8..c6c743bc2c68 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1505,8 +1505,15 @@ static inline long
> get_user_pages_longterm(unsigned long start,
> }
> #endif /* CONFIG_FS_DAX */
>
> -int get_user_pages_fast(unsigned long start, int nr_pages, int write,
> - struct page **pages);
> +
> +int __get_user_pages_fast(unsigned long start, int nr_pages,
> + struct page **pages, unsigned int gup_flags);
> +
> +static inline int get_user_pages_fast(unsigned long start, int
> nr_pages, int write,
> + struct page **pages)
> +{
> + return __get_user_pages_fast(start, nr_pages, pages, write ?
> FOLL_WRITE);
> +}
>
> /* Container for pinned pfns / pages */
> struct frame_vector {
>
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Ira Weiny @ 2019-03-25 8:42 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, linux-s390, Yoshinori Sato, linux-rdma,
Jason Gunthorpe, Ingo Molnar, John Hubbard, Borislav Petkov,
Thomas Gleixner, netdev@vger.kernel.org,
Linux Kernel Mailing List, Ralf Baechle, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <CAA9_cmcx-Bqo=CFuSj7Xcap3e5uaAot2reL2T74C47Ut6_KtQw@mail.gmail.com>
On Fri, Mar 22, 2019 at 03:12:55PM -0700, Dan Williams wrote:
> On Sun, Mar 17, 2019 at 7:36 PM <ira.weiny@intel.com> wrote:
> >
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > DAX pages were previously unprotected from longterm pins when users
> > called get_user_pages_fast().
> >
> > Use the new FOLL_LONGTERM flag to check for DEVMAP pages and fall
> > back to regular GUP processing if a DEVMAP page is encountered.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > mm/gup.c | 29 +++++++++++++++++++++++++----
> > 1 file changed, 25 insertions(+), 4 deletions(-)
> >
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 0684a9536207..173db0c44678 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -1600,6 +1600,9 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
> > goto pte_unmap;
> >
> > if (pte_devmap(pte)) {
> > + if (unlikely(flags & FOLL_LONGTERM))
> > + goto pte_unmap;
> > +
> > pgmap = get_dev_pagemap(pte_pfn(pte), pgmap);
> > if (unlikely(!pgmap)) {
> > undo_dev_pagemap(nr, nr_start, pages);
> > @@ -1739,8 +1742,11 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> > if (!pmd_access_permitted(orig, flags & FOLL_WRITE))
> > return 0;
> >
> > - if (pmd_devmap(orig))
> > + if (pmd_devmap(orig)) {
> > + if (unlikely(flags & FOLL_LONGTERM))
> > + return 0;
> > return __gup_device_huge_pmd(orig, pmdp, addr, end, pages, nr);
> > + }
> >
> > refs = 0;
> > page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
> > @@ -1777,8 +1783,11 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
> > if (!pud_access_permitted(orig, flags & FOLL_WRITE))
> > return 0;
> >
> > - if (pud_devmap(orig))
> > + if (pud_devmap(orig)) {
> > + if (unlikely(flags & FOLL_LONGTERM))
> > + return 0;
> > return __gup_device_huge_pud(orig, pudp, addr, end, pages, nr);
> > + }
> >
> > refs = 0;
> > page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> > @@ -2066,8 +2075,20 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
> > start += nr << PAGE_SHIFT;
> > pages += nr;
> >
> > - ret = get_user_pages_unlocked(start, nr_pages - nr, pages,
> > - gup_flags);
> > + if (gup_flags & FOLL_LONGTERM) {
> > + down_read(¤t->mm->mmap_sem);
> > + ret = __gup_longterm_locked(current, current->mm,
> > + start, nr_pages - nr,
> > + pages, NULL, gup_flags);
> > + up_read(¤t->mm->mmap_sem);
> > + } else {
> > + /*
> > + * retain FAULT_FOLL_ALLOW_RETRY optimization if
> > + * possible
> > + */
> > + ret = get_user_pages_unlocked(start, nr_pages - nr,
> > + pages, gup_flags);
>
> I couldn't immediately grok why this path needs to branch on
> FOLL_LONGTERM? Won't get_user_pages_unlocked(..., FOLL_LONGTERM) do
> the right thing?
Unfortunately holding the lock is required to support FOLL_LONGTERM (to check
the VMAs) but we don't want to hold the lock to be optimal (specifically allow
FAULT_FOLL_ALLOW_RETRY). So I'm maintaining the optimization for *_fast users
who do not specify FOLL_LONGTERM.
Another way to do this would have been to define __gup_longterm_unlocked with
the above logic, but that seemed overkill at this point.
Ira
^ permalink raw reply
* Re: [RESEND 5/7] IB/hfi1: Use the new FOLL_LONGTERM flag to get_user_pages_fast()
From: Ira Weiny @ 2019-03-25 8:43 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, linux-s390, Yoshinori Sato, linux-rdma,
Jason Gunthorpe, Ingo Molnar, John Hubbard, Borislav Petkov,
Thomas Gleixner, netdev@vger.kernel.org,
Linux Kernel Mailing List, Ralf Baechle, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <CAA9_cmdQjMekSFU09gLc87-PVx2iHeeh2jC6KeFY1UeadpPh4A@mail.gmail.com>
On Fri, Mar 22, 2019 at 03:14:26PM -0700, Dan Williams wrote:
> On Sun, Mar 17, 2019 at 7:36 PM <ira.weiny@intel.com> wrote:
> >
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > Use the new FOLL_LONGTERM to get_user_pages_fast() to protect against
> > FS DAX pages being mapped.
> >
> > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> > drivers/infiniband/hw/hfi1/user_pages.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/hfi1/user_pages.c b/drivers/infiniband/hw/hfi1/user_pages.c
> > index 78ccacaf97d0..6a7f9cd5a94e 100644
> > --- a/drivers/infiniband/hw/hfi1/user_pages.c
> > +++ b/drivers/infiniband/hw/hfi1/user_pages.c
> > @@ -104,9 +104,11 @@ int hfi1_acquire_user_pages(struct mm_struct *mm, unsigned long vaddr, size_t np
> > bool writable, struct page **pages)
> > {
> > int ret;
> > + unsigned int gup_flags = writable ? FOLL_WRITE : 0;
>
> Maybe:
>
> unsigned int gup_flags = FOLL_LONGTERM | (writable ? FOLL_WRITE : 0);
>
> ?
Sure looks good.
Ira
>
> >
> > - ret = get_user_pages_fast(vaddr, npages, writable ? FOLL_WRITE : 0,
> > - pages);
> > + gup_flags |= FOLL_LONGTERM;
> > +
> > + ret = get_user_pages_fast(vaddr, npages, gup_flags, pages);
> > if (ret < 0)
> > return ret;
> >
> > --
> > 2.20.1
> >
>
^ permalink raw reply
* Re: [RESEND 1/7] mm/gup: Replace get_user_pages_longterm() with FOLL_LONGTERM
From: Dan Williams @ 2019-03-25 16:45 UTC (permalink / raw)
To: Ira Weiny
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, Michal Hocko, linux-mm, Rich Felker,
Paul Mackerras, sparclinux, linux-s390, Yoshinori Sato,
linux-rdma, Aneesh Kumar K . V, Jason Gunthorpe, Ingo Molnar,
linux-mips, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <20190325061941.GA16366@iweiny-DESK2.sc.intel.com>
On Mon, Mar 25, 2019 at 7:21 AM Ira Weiny <ira.weiny@intel.com> wrote:
[..]
> > > @@ -1268,10 +1246,14 @@ static long check_and_migrate_cma_pages(unsigned long start, long nr_pages,
> > > putback_movable_pages(&cma_page_list);
> > > }
> > > /*
> > > - * We did migrate all the pages, Try to get the page references again
> > > - * migrating any new CMA pages which we failed to isolate earlier.
> > > + * We did migrate all the pages, Try to get the page references
> > > + * again migrating any new CMA pages which we failed to isolate
> > > + * earlier.
> > > */
> > > - nr_pages = get_user_pages(start, nr_pages, gup_flags, pages, vmas);
> > > + nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages,
> > > + pages, vmas, NULL,
> > > + gup_flags);
> > > +
> >
> > Why did this need to change to __get_user_pages_locked?
>
> __get_uer_pages_locked() is now the "internal call" for get_user_pages.
>
> Technically it did not _have_ to change but there is no need to call
> get_user_pages() again because the FOLL_TOUCH flags is already set. Also this
> call then matches the __get_user_pages_locked() which was called on the pages
> we migrated from. Mostly this keeps the code "symmetrical" in that we called
> __get_user_pages_locked() on the pages we are migrating from and the same call
> on the pages we migrated to.
>
> While the change here looks funny I think the final code is better.
Agree, but I think that either needs to be noted in the changelog so
it's not a surprise, or moved to a follow-on cleanup patch.
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Jason Gunthorpe @ 2019-03-25 16:47 UTC (permalink / raw)
To: Ira Weiny
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, Dan Williams, linux-s390, Yoshinori Sato, linux-rdma,
Ingo Molnar, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <20190325084225.GC16366@iweiny-DESK2.sc.intel.com>
On Mon, Mar 25, 2019 at 01:42:26AM -0700, Ira Weiny wrote:
> On Fri, Mar 22, 2019 at 03:12:55PM -0700, Dan Williams wrote:
> > On Sun, Mar 17, 2019 at 7:36 PM <ira.weiny@intel.com> wrote:
> > >
> > > From: Ira Weiny <ira.weiny@intel.com>
> > >
> > > DAX pages were previously unprotected from longterm pins when users
> > > called get_user_pages_fast().
> > >
> > > Use the new FOLL_LONGTERM flag to check for DEVMAP pages and fall
> > > back to regular GUP processing if a DEVMAP page is encountered.
> > >
> > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > mm/gup.c | 29 +++++++++++++++++++++++++----
> > > 1 file changed, 25 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/mm/gup.c b/mm/gup.c
> > > index 0684a9536207..173db0c44678 100644
> > > +++ b/mm/gup.c
> > > @@ -1600,6 +1600,9 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
> > > goto pte_unmap;
> > >
> > > if (pte_devmap(pte)) {
> > > + if (unlikely(flags & FOLL_LONGTERM))
> > > + goto pte_unmap;
> > > +
> > > pgmap = get_dev_pagemap(pte_pfn(pte), pgmap);
> > > if (unlikely(!pgmap)) {
> > > undo_dev_pagemap(nr, nr_start, pages);
> > > @@ -1739,8 +1742,11 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> > > if (!pmd_access_permitted(orig, flags & FOLL_WRITE))
> > > return 0;
> > >
> > > - if (pmd_devmap(orig))
> > > + if (pmd_devmap(orig)) {
> > > + if (unlikely(flags & FOLL_LONGTERM))
> > > + return 0;
> > > return __gup_device_huge_pmd(orig, pmdp, addr, end, pages, nr);
> > > + }
> > >
> > > refs = 0;
> > > page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
> > > @@ -1777,8 +1783,11 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
> > > if (!pud_access_permitted(orig, flags & FOLL_WRITE))
> > > return 0;
> > >
> > > - if (pud_devmap(orig))
> > > + if (pud_devmap(orig)) {
> > > + if (unlikely(flags & FOLL_LONGTERM))
> > > + return 0;
> > > return __gup_device_huge_pud(orig, pudp, addr, end, pages, nr);
> > > + }
> > >
> > > refs = 0;
> > > page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> > > @@ -2066,8 +2075,20 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
> > > start += nr << PAGE_SHIFT;
> > > pages += nr;
> > >
> > > - ret = get_user_pages_unlocked(start, nr_pages - nr, pages,
> > > - gup_flags);
> > > + if (gup_flags & FOLL_LONGTERM) {
> > > + down_read(¤t->mm->mmap_sem);
> > > + ret = __gup_longterm_locked(current, current->mm,
> > > + start, nr_pages - nr,
> > > + pages, NULL, gup_flags);
> > > + up_read(¤t->mm->mmap_sem);
> > > + } else {
> > > + /*
> > > + * retain FAULT_FOLL_ALLOW_RETRY optimization if
> > > + * possible
> > > + */
> > > + ret = get_user_pages_unlocked(start, nr_pages - nr,
> > > + pages, gup_flags);
> >
> > I couldn't immediately grok why this path needs to branch on
> > FOLL_LONGTERM? Won't get_user_pages_unlocked(..., FOLL_LONGTERM) do
> > the right thing?
>
> Unfortunately holding the lock is required to support FOLL_LONGTERM (to check
> the VMAs) but we don't want to hold the lock to be optimal (specifically allow
> FAULT_FOLL_ALLOW_RETRY). So I'm maintaining the optimization for *_fast users
> who do not specify FOLL_LONGTERM.
>
> Another way to do this would have been to define __gup_longterm_unlocked with
> the above logic, but that seemed overkill at this point.
get_user_pages_unlocked() is an exported symbol, shouldn't it work
with the FOLL_LONGTERM flag?
I think it should even though we have no user..
Otherwise the GUP API just gets more confusing.
Jason
^ permalink raw reply
* Re: [RESEND 1/7] mm/gup: Replace get_user_pages_longterm() with FOLL_LONGTERM
From: Ira Weiny @ 2019-03-25 8:46 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, Michal Hocko, linux-mm, Rich Felker,
Paul Mackerras, sparclinux, linux-s390, Yoshinori Sato,
linux-rdma, Aneesh Kumar K . V, Jason Gunthorpe, Ingo Molnar,
linux-mips, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <CAPcyv4hPxoX1+u=fjzCeVYOd9Bck9d=A9SQ-mjzeMA2tf9B9dA@mail.gmail.com>
On Mon, Mar 25, 2019 at 09:45:12AM -0700, Dan Williams wrote:
> On Mon, Mar 25, 2019 at 7:21 AM Ira Weiny <ira.weiny@intel.com> wrote:
> [..]
> > > > @@ -1268,10 +1246,14 @@ static long check_and_migrate_cma_pages(unsigned long start, long nr_pages,
> > > > putback_movable_pages(&cma_page_list);
> > > > }
> > > > /*
> > > > - * We did migrate all the pages, Try to get the page references again
> > > > - * migrating any new CMA pages which we failed to isolate earlier.
> > > > + * We did migrate all the pages, Try to get the page references
> > > > + * again migrating any new CMA pages which we failed to isolate
> > > > + * earlier.
> > > > */
> > > > - nr_pages = get_user_pages(start, nr_pages, gup_flags, pages, vmas);
> > > > + nr_pages = __get_user_pages_locked(tsk, mm, start, nr_pages,
> > > > + pages, vmas, NULL,
> > > > + gup_flags);
> > > > +
> > >
> > > Why did this need to change to __get_user_pages_locked?
> >
> > __get_uer_pages_locked() is now the "internal call" for get_user_pages.
> >
> > Technically it did not _have_ to change but there is no need to call
> > get_user_pages() again because the FOLL_TOUCH flags is already set. Also this
> > call then matches the __get_user_pages_locked() which was called on the pages
> > we migrated from. Mostly this keeps the code "symmetrical" in that we called
> > __get_user_pages_locked() on the pages we are migrating from and the same call
> > on the pages we migrated to.
> >
> > While the change here looks funny I think the final code is better.
>
> Agree, but I think that either needs to be noted in the changelog so
> it's not a surprise, or moved to a follow-on cleanup patch.
>
Can do...
Thanks!
Ira
^ permalink raw reply
* Re: [PATCH v4 3/4] ocxl: Remove superfluous 'extern' from headers
From: Greg Kurz @ 2019-03-25 16:55 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
alastair, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190325053456.14599-4-alastair@au1.ibm.com>
On Mon, 25 Mar 2019 16:34:54 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> The 'extern' keyword adds no value here.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> drivers/misc/ocxl/ocxl_internal.h | 54 +++++++++++++++----------------
> include/misc/ocxl.h | 36 ++++++++++-----------
> 2 files changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index a32f2151029f..321b29e77f45 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -16,7 +16,6 @@
>
> extern struct pci_driver ocxl_pci_driver;
>
> -
> struct ocxl_fn {
> struct device dev;
> int bar_used[3];
> @@ -92,41 +91,40 @@ struct ocxl_process_element {
> __be32 software_state;
> };
>
> +struct ocxl_afu *ocxl_afu_get(struct ocxl_afu *afu);
> +void ocxl_afu_put(struct ocxl_afu *afu);
>
> -extern struct ocxl_afu *ocxl_afu_get(struct ocxl_afu *afu);
> -extern void ocxl_afu_put(struct ocxl_afu *afu);
> -
> -extern int ocxl_create_cdev(struct ocxl_afu *afu);
> -extern void ocxl_destroy_cdev(struct ocxl_afu *afu);
> -extern int ocxl_register_afu(struct ocxl_afu *afu);
> -extern void ocxl_unregister_afu(struct ocxl_afu *afu);
> +int ocxl_create_cdev(struct ocxl_afu *afu);
> +void ocxl_destroy_cdev(struct ocxl_afu *afu);
> +int ocxl_register_afu(struct ocxl_afu *afu);
> +void ocxl_unregister_afu(struct ocxl_afu *afu);
>
> -extern int ocxl_file_init(void);
> -extern void ocxl_file_exit(void);
> +int ocxl_file_init(void);
> +void ocxl_file_exit(void);
>
> -extern int ocxl_pasid_afu_alloc(struct ocxl_fn *fn, u32 size);
> -extern void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> -extern int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> -extern void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> +int ocxl_pasid_afu_alloc(struct ocxl_fn *fn, u32 size);
> +void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> +int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> +void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
>
> -extern struct ocxl_context *ocxl_context_alloc(void);
> -extern int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> +struct ocxl_context *ocxl_context_alloc(void);
> +int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> struct address_space *mapping);
> -extern int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> -extern int ocxl_context_mmap(struct ocxl_context *ctx,
> +int ocxl_context_attach(struct ocxl_context *ctx, u64 amr);
> +int ocxl_context_mmap(struct ocxl_context *ctx,
> struct vm_area_struct *vma);
> -extern int ocxl_context_detach(struct ocxl_context *ctx);
> -extern void ocxl_context_detach_all(struct ocxl_afu *afu);
> -extern void ocxl_context_free(struct ocxl_context *ctx);
> +int ocxl_context_detach(struct ocxl_context *ctx);
> +void ocxl_context_detach_all(struct ocxl_afu *afu);
> +void ocxl_context_free(struct ocxl_context *ctx);
>
> -extern int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> -extern void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
> +int ocxl_sysfs_add_afu(struct ocxl_afu *afu);
> +void ocxl_sysfs_remove_afu(struct ocxl_afu *afu);
>
> -extern int ocxl_afu_irq_alloc(struct ocxl_context *ctx, u64 *irq_offset);
> -extern int ocxl_afu_irq_free(struct ocxl_context *ctx, u64 irq_offset);
> -extern void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> -extern int ocxl_afu_irq_set_fd(struct ocxl_context *ctx, u64 irq_offset,
> +int ocxl_afu_irq_alloc(struct ocxl_context *ctx, u64 *irq_offset);
> +int ocxl_afu_irq_free(struct ocxl_context *ctx, u64 irq_offset);
> +void ocxl_afu_irq_free_all(struct ocxl_context *ctx);
> +int ocxl_afu_irq_set_fd(struct ocxl_context *ctx, u64 irq_offset,
> int eventfd);
> -extern u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, u64 irq_offset);
> +u64 ocxl_afu_irq_get_addr(struct ocxl_context *ctx, u64 irq_offset);
>
> #endif /* _OCXL_INTERNAL_H_ */
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 9ff6ddc28e22..4544573cc93c 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -53,7 +53,7 @@ struct ocxl_fn_config {
> * Read the configuration space of a function and fill in a
> * ocxl_fn_config structure with all the function details
> */
> -extern int ocxl_config_read_function(struct pci_dev *dev,
> +int ocxl_config_read_function(struct pci_dev *dev,
> struct ocxl_fn_config *fn);
>
> /*
> @@ -62,14 +62,14 @@ extern int ocxl_config_read_function(struct pci_dev *dev,
> * AFU indexes can be sparse, so a driver should check all indexes up
> * to the maximum found in the function description
> */
> -extern int ocxl_config_check_afu_index(struct pci_dev *dev,
> +int ocxl_config_check_afu_index(struct pci_dev *dev,
> struct ocxl_fn_config *fn, int afu_idx);
>
> /*
> * Read the configuration space of a function for the AFU specified by
> * the index 'afu_idx'. Fills in a ocxl_afu_config structure
> */
> -extern int ocxl_config_read_afu(struct pci_dev *dev,
> +int ocxl_config_read_afu(struct pci_dev *dev,
> struct ocxl_fn_config *fn,
> struct ocxl_afu_config *afu,
> u8 afu_idx);
> @@ -77,7 +77,7 @@ extern int ocxl_config_read_afu(struct pci_dev *dev,
> /*
> * Get the max PASID value that can be used by the function
> */
> -extern int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
>
> /*
> * Tell an AFU, by writing in the configuration space, the PASIDs that
> @@ -87,7 +87,7 @@ extern int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> * 'afu_control_offset' is the offset of the AFU control DVSEC which
> * can be found in the function configuration
> */
> -extern void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> +void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> int afu_control_offset,
> int pasid_base, u32 pasid_count_log);
>
> @@ -98,7 +98,7 @@ extern void ocxl_config_set_afu_pasid(struct pci_dev *dev,
> * 'supported' is the total number of actags desired by all the AFUs
> * of the function.
> */
> -extern int ocxl_config_get_actag_info(struct pci_dev *dev,
> +int ocxl_config_get_actag_info(struct pci_dev *dev,
> u16 *base, u16 *enabled, u16 *supported);
>
> /*
> @@ -108,7 +108,7 @@ extern int ocxl_config_get_actag_info(struct pci_dev *dev,
> * 'func_offset' is the offset of the Function DVSEC that can found in
> * the function configuration
> */
> -extern void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> +void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> u32 actag_base, u32 actag_count);
>
> /*
> @@ -118,7 +118,7 @@ extern void ocxl_config_set_actag(struct pci_dev *dev, int func_offset,
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern void ocxl_config_set_afu_actag(struct pci_dev *dev,
> +void ocxl_config_set_afu_actag(struct pci_dev *dev,
> int afu_control_offset,
> int actag_base, int actag_count);
>
> @@ -128,7 +128,7 @@ extern void ocxl_config_set_afu_actag(struct pci_dev *dev,
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern void ocxl_config_set_afu_state(struct pci_dev *dev,
> +void ocxl_config_set_afu_state(struct pci_dev *dev,
> int afu_control_offset, int enable);
>
> /*
> @@ -139,7 +139,7 @@ extern void ocxl_config_set_afu_state(struct pci_dev *dev,
> * between the host and device, and set the Transaction Layer on both
> * accordingly.
> */
> -extern int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
> +int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
>
> /*
> * Request an AFU to terminate a PASID.
> @@ -152,7 +152,7 @@ extern int ocxl_config_set_TL(struct pci_dev *dev, int tl_dvsec);
> * 'afu_control_offset' is the offset of the AFU control DVSEC for the
> * desired AFU. It can be found in the AFU configuration
> */
> -extern int ocxl_config_terminate_pasid(struct pci_dev *dev,
> +int ocxl_config_terminate_pasid(struct pci_dev *dev,
> int afu_control_offset, int pasid);
>
> /*
> @@ -165,13 +165,13 @@ extern int ocxl_config_terminate_pasid(struct pci_dev *dev,
> * Returns a 'link handle' that should be used for further calls for
> * the link
> */
> -extern int ocxl_link_setup(struct pci_dev *dev, int PE_mask,
> +int ocxl_link_setup(struct pci_dev *dev, int PE_mask,
> void **link_handle);
>
> /*
> * Remove the association between the function and its link.
> */
> -extern void ocxl_link_release(struct pci_dev *dev, void *link_handle);
> +void ocxl_link_release(struct pci_dev *dev, void *link_handle);
>
> /*
> * Add a Process Element to the Shared Process Area for a link.
> @@ -183,7 +183,7 @@ extern void ocxl_link_release(struct pci_dev *dev, void *link_handle);
> * 'xsl_err_data' is an argument passed to the above callback, if
> * defined
> */
> -extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> +int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> u64 amr, struct mm_struct *mm,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
> @@ -195,12 +195,12 @@ extern int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> * pasid: the PASID for the AFU context
> * tid: the new thread id for the process element
> */
> -extern int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
>
> /*
> * Remove a Process Element from the Shared Process Area for a link
> */
> -extern int ocxl_link_remove_pe(void *link_handle, int pasid);
> +int ocxl_link_remove_pe(void *link_handle, int pasid);
>
> /*
> * Allocate an AFU interrupt associated to the link.
> @@ -212,12 +212,12 @@ extern int ocxl_link_remove_pe(void *link_handle, int pasid);
> * interrupt. It is an MMIO address which needs to be remapped (one
> * page).
> */
> -extern int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> +int ocxl_link_irq_alloc(void *link_handle, int *hw_irq,
> u64 *obj_handle);
>
> /*
> * Free a previously allocated AFU interrupt
> */
> -extern void ocxl_link_free_irq(void *link_handle, int hw_irq);
> +void ocxl_link_free_irq(void *link_handle, int hw_irq);
>
> #endif /* _MISC_OCXL_H_ */
^ permalink raw reply
* Re: [PATCH v4 0/4] ocxl: OpenCAPI Cleanup
From: Greg Kurz @ 2019-03-25 16:49 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
alastair, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190325053456.14599-1-alastair@au1.ibm.com>
Hi Alastair,
I forgot to mention it during v3 but please don't link new version
of a patchset to the previous one with --in-reply-to. This is to
ensure I can see them in my email client without having to scroll
back many days in the past (which likely means a fair number of
e-mails on linuxppc-dev).
Cheers,
--
Greg
On Mon, 25 Mar 2019 16:34:51 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Some minor cleanups for the OpenCAPI driver as a prerequisite
> for an ocxl driver refactoring to allow the driver core to
> be utilised by external drivers.
>
> Changelog:
> V4:
> - Drop printf format changes as they omit the format indicator for '0'
> V3:
> - Add missed header in 'ocxl: Remove some unused exported symbols'.
> This addresses the introduced sparse warnings
> V2:
> - remove intermediate assignment of 'link' var in
> 'Rename struct link to ocxl_link'
> - Don't shift definition of ocxl_context_attach in
> 'Remove some unused exported symbols'
>
>
> Alastair D'Silva (4):
> ocxl: Rename struct link to ocxl_link
> ocxl: read_pasid never returns an error, so make it void
> ocxl: Remove superfluous 'extern' from headers
> ocxl: Remove some unused exported symbols
>
> drivers/misc/ocxl/config.c | 13 ++---
> drivers/misc/ocxl/file.c | 5 +-
> drivers/misc/ocxl/link.c | 36 ++++++-------
> drivers/misc/ocxl/ocxl_internal.h | 85 +++++++++++++++++++------------
> include/misc/ocxl.h | 53 ++++++-------------
> 5 files changed, 91 insertions(+), 101 deletions(-)
>
^ permalink raw reply
* Re: [PATCH v4 4/4] ocxl: Remove some unused exported symbols
From: Greg Kurz @ 2019-03-25 16:57 UTC (permalink / raw)
To: Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
alastair, Frederic Barrat, linuxppc-dev
In-Reply-To: <20190325053456.14599-5-alastair@au1.ibm.com>
On Mon, 25 Mar 2019 16:34:55 +1100
"Alastair D'Silva" <alastair@au1.ibm.com> wrote:
> From: Alastair D'Silva <alastair@d-silva.org>
>
> Remove some unused exported symbols.
>
> Signed-off-by: Alastair D'Silva <alastair@d-silva.org>
> ---
Reviewed-by: Greg Kurz <groug@kaod.org>
> drivers/misc/ocxl/config.c | 4 +---
> drivers/misc/ocxl/ocxl_internal.h | 23 +++++++++++++++++++++++
> include/misc/ocxl.h | 23 -----------------------
> 3 files changed, 24 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/misc/ocxl/config.c b/drivers/misc/ocxl/config.c
> index 4dc11897237d..5e65acb8e134 100644
> --- a/drivers/misc/ocxl/config.c
> +++ b/drivers/misc/ocxl/config.c
> @@ -2,8 +2,8 @@
> // Copyright 2017 IBM Corp.
> #include <linux/pci.h>
> #include <asm/pnv-ocxl.h>
> -#include <misc/ocxl.h>
> #include <misc/ocxl-config.h>
> +#include "ocxl_internal.h"
>
> #define EXTRACT_BIT(val, bit) (!!(val & BIT(bit)))
> #define EXTRACT_BITS(val, s, e) ((val & GENMASK(e, s)) >> s)
> @@ -299,7 +299,6 @@ int ocxl_config_check_afu_index(struct pci_dev *dev,
> }
> return 1;
> }
> -EXPORT_SYMBOL_GPL(ocxl_config_check_afu_index);
>
> static int read_afu_name(struct pci_dev *dev, struct ocxl_fn_config *fn,
> struct ocxl_afu_config *afu)
> @@ -535,7 +534,6 @@ int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count)
> {
> return pnv_ocxl_get_pasid_count(dev, count);
> }
> -EXPORT_SYMBOL_GPL(ocxl_config_get_pasid_info);
>
> void ocxl_config_set_afu_pasid(struct pci_dev *dev, int pos, int pasid_base,
> u32 pasid_count_log)
> diff --git a/drivers/misc/ocxl/ocxl_internal.h b/drivers/misc/ocxl/ocxl_internal.h
> index 321b29e77f45..06fd98c989c8 100644
> --- a/drivers/misc/ocxl/ocxl_internal.h
> +++ b/drivers/misc/ocxl/ocxl_internal.h
> @@ -107,6 +107,29 @@ void ocxl_pasid_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
> int ocxl_actag_afu_alloc(struct ocxl_fn *fn, u32 size);
> void ocxl_actag_afu_free(struct ocxl_fn *fn, u32 start, u32 size);
>
> +/*
> + * Get the max PASID value that can be used by the function
> + */
> +int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> +
> +/*
> + * Check if an AFU index is valid for the given function.
> + *
> + * AFU indexes can be sparse, so a driver should check all indexes up
> + * to the maximum found in the function description
> + */
> +int ocxl_config_check_afu_index(struct pci_dev *dev,
> + struct ocxl_fn_config *fn, int afu_idx);
> +
> +/**
> + * Update values within a Process Element
> + *
> + * link_handle: the link handle associated with the process element
> + * pasid: the PASID for the AFU context
> + * tid: the new thread id for the process element
> + */
> +int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> +
> struct ocxl_context *ocxl_context_alloc(void);
> int ocxl_context_init(struct ocxl_context *ctx, struct ocxl_afu *afu,
> struct address_space *mapping);
> diff --git a/include/misc/ocxl.h b/include/misc/ocxl.h
> index 4544573cc93c..9530d3be1b30 100644
> --- a/include/misc/ocxl.h
> +++ b/include/misc/ocxl.h
> @@ -56,15 +56,6 @@ struct ocxl_fn_config {
> int ocxl_config_read_function(struct pci_dev *dev,
> struct ocxl_fn_config *fn);
>
> -/*
> - * Check if an AFU index is valid for the given function.
> - *
> - * AFU indexes can be sparse, so a driver should check all indexes up
> - * to the maximum found in the function description
> - */
> -int ocxl_config_check_afu_index(struct pci_dev *dev,
> - struct ocxl_fn_config *fn, int afu_idx);
> -
> /*
> * Read the configuration space of a function for the AFU specified by
> * the index 'afu_idx'. Fills in a ocxl_afu_config structure
> @@ -74,11 +65,6 @@ int ocxl_config_read_afu(struct pci_dev *dev,
> struct ocxl_afu_config *afu,
> u8 afu_idx);
>
> -/*
> - * Get the max PASID value that can be used by the function
> - */
> -int ocxl_config_get_pasid_info(struct pci_dev *dev, int *count);
> -
> /*
> * Tell an AFU, by writing in the configuration space, the PASIDs that
> * it can use. Range starts at 'pasid_base' and its size is a multiple
> @@ -188,15 +174,6 @@ int ocxl_link_add_pe(void *link_handle, int pasid, u32 pidr, u32 tidr,
> void (*xsl_err_cb)(void *data, u64 addr, u64 dsisr),
> void *xsl_err_data);
>
> -/**
> - * Update values within a Process Element
> - *
> - * link_handle: the link handle associated with the process element
> - * pasid: the PASID for the AFU context
> - * tid: the new thread id for the process element
> - */
> -int ocxl_link_update_pe(void *link_handle, int pasid, __u16 tid);
> -
> /*
> * Remove a Process Element from the Shared Process Area for a link
> */
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Ira Weiny @ 2019-03-25 9:23 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, Dan Williams, linux-s390, Yoshinori Sato, linux-rdma,
Ingo Molnar, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <20190325164713.GC9949@ziepe.ca>
On Mon, Mar 25, 2019 at 01:47:13PM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 25, 2019 at 01:42:26AM -0700, Ira Weiny wrote:
> > On Fri, Mar 22, 2019 at 03:12:55PM -0700, Dan Williams wrote:
> > > On Sun, Mar 17, 2019 at 7:36 PM <ira.weiny@intel.com> wrote:
> > > >
> > > > From: Ira Weiny <ira.weiny@intel.com>
> > > >
> > > > DAX pages were previously unprotected from longterm pins when users
> > > > called get_user_pages_fast().
> > > >
> > > > Use the new FOLL_LONGTERM flag to check for DEVMAP pages and fall
> > > > back to regular GUP processing if a DEVMAP page is encountered.
> > > >
> > > > Signed-off-by: Ira Weiny <ira.weiny@intel.com>
> > > > mm/gup.c | 29 +++++++++++++++++++++++++----
> > > > 1 file changed, 25 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/mm/gup.c b/mm/gup.c
> > > > index 0684a9536207..173db0c44678 100644
> > > > +++ b/mm/gup.c
> > > > @@ -1600,6 +1600,9 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
> > > > goto pte_unmap;
> > > >
> > > > if (pte_devmap(pte)) {
> > > > + if (unlikely(flags & FOLL_LONGTERM))
> > > > + goto pte_unmap;
> > > > +
> > > > pgmap = get_dev_pagemap(pte_pfn(pte), pgmap);
> > > > if (unlikely(!pgmap)) {
> > > > undo_dev_pagemap(nr, nr_start, pages);
> > > > @@ -1739,8 +1742,11 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> > > > if (!pmd_access_permitted(orig, flags & FOLL_WRITE))
> > > > return 0;
> > > >
> > > > - if (pmd_devmap(orig))
> > > > + if (pmd_devmap(orig)) {
> > > > + if (unlikely(flags & FOLL_LONGTERM))
> > > > + return 0;
> > > > return __gup_device_huge_pmd(orig, pmdp, addr, end, pages, nr);
> > > > + }
> > > >
> > > > refs = 0;
> > > > page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
> > > > @@ -1777,8 +1783,11 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
> > > > if (!pud_access_permitted(orig, flags & FOLL_WRITE))
> > > > return 0;
> > > >
> > > > - if (pud_devmap(orig))
> > > > + if (pud_devmap(orig)) {
> > > > + if (unlikely(flags & FOLL_LONGTERM))
> > > > + return 0;
> > > > return __gup_device_huge_pud(orig, pudp, addr, end, pages, nr);
> > > > + }
> > > >
> > > > refs = 0;
> > > > page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> > > > @@ -2066,8 +2075,20 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
> > > > start += nr << PAGE_SHIFT;
> > > > pages += nr;
> > > >
> > > > - ret = get_user_pages_unlocked(start, nr_pages - nr, pages,
> > > > - gup_flags);
> > > > + if (gup_flags & FOLL_LONGTERM) {
> > > > + down_read(¤t->mm->mmap_sem);
> > > > + ret = __gup_longterm_locked(current, current->mm,
> > > > + start, nr_pages - nr,
> > > > + pages, NULL, gup_flags);
> > > > + up_read(¤t->mm->mmap_sem);
> > > > + } else {
> > > > + /*
> > > > + * retain FAULT_FOLL_ALLOW_RETRY optimization if
> > > > + * possible
> > > > + */
> > > > + ret = get_user_pages_unlocked(start, nr_pages - nr,
> > > > + pages, gup_flags);
> > >
> > > I couldn't immediately grok why this path needs to branch on
> > > FOLL_LONGTERM? Won't get_user_pages_unlocked(..., FOLL_LONGTERM) do
> > > the right thing?
> >
> > Unfortunately holding the lock is required to support FOLL_LONGTERM (to check
> > the VMAs) but we don't want to hold the lock to be optimal (specifically allow
> > FAULT_FOLL_ALLOW_RETRY). So I'm maintaining the optimization for *_fast users
> > who do not specify FOLL_LONGTERM.
> >
> > Another way to do this would have been to define __gup_longterm_unlocked with
> > the above logic, but that seemed overkill at this point.
>
> get_user_pages_unlocked() is an exported symbol, shouldn't it work
> with the FOLL_LONGTERM flag?
>
> I think it should even though we have no user..
>
> Otherwise the GUP API just gets more confusing.
I agree WRT to the API. But I think callers of get_user_pages_unlocked() are
not going to get the behavior they want if they specify FOLL_LONGTERM.
What I could do is BUG_ON (or just WARN_ON) if unlocked is called with
FOLL_LONGTERM similar to the code in get_user_pages_locked() which does not
allow locked and vmas to be passed together:
if (locked) {
/* if VM_FAULT_RETRY can be returned, vmas become invalid */
BUG_ON(vmas);
/* check caller initialized locked */
BUG_ON(*locked != 1);
}
Combining Dan's comment and yours; I could do something like below? (not even
compile tested) Coded with WARN_ON because technically I _think_ the
FOLL_LONGTERM would "work" but not be optimal. I'm just not 100% sure. A
BUG_ON would be most protective IMO.
diff --git a/mm/gup.c b/mm/gup.c
index 173db0c44678..8e31411f485f 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -1014,6 +1014,29 @@ long get_user_pages_locked(unsigned long start, unsigned long nr_pages,
}
EXPORT_SYMBOL(get_user_pages_locked);
+long __gup_longterm_unlocked(unsigned long start, unsigned long nr_pages,
+ struct page **pages, unsigned int gup_flags)
+{
+ struct mm_struct *mm = current->mm;
+ int locked = 1;
+ long ret;
+
+ down_read(&mm->mmap_sem);
+ if (gup_flags & FOLL_LONGTERM) {
+ ret = __gup_longterm_locked(current, mm,
+ start, nr_pages - nr,
+ pages, NULL, gup_flags);
+ } else {
+ ret = __get_user_pages_locked(current, mm, start, nr_pages,
+ pages, NULL, &locked,
+ gup_flags | FOLL_TOUCH);
+ }
+ if (locked)
+ up_read(&mm->mmap_sem);
+
+ return ret;
+}
+
/*
* get_user_pages_unlocked() is suitable to replace the form:
*
@@ -1032,16 +1055,9 @@ EXPORT_SYMBOL(get_user_pages_locked);
long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages,
struct page **pages, unsigned int gup_flags)
{
- struct mm_struct *mm = current->mm;
- int locked = 1;
- long ret;
+ WARN_ON(gup_flags & FOLL_LONGTERM);
- down_read(&mm->mmap_sem);
- ret = __get_user_pages_locked(current, mm, start, nr_pages, pages, NULL,
- &locked, gup_flags | FOLL_TOUCH);
- if (locked)
- up_read(&mm->mmap_sem);
- return ret;
+ __gup_longterm_unlocked(start, nr_pages, pages, gup_flags);
}
EXPORT_SYMBOL(get_user_pages_unlocked);
@@ -2075,20 +2091,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages,
start += nr << PAGE_SHIFT;
pages += nr;
- if (gup_flags & FOLL_LONGTERM) {
- down_read(¤t->mm->mmap_sem);
- ret = __gup_longterm_locked(current, current->mm,
- start, nr_pages - nr,
- pages, NULL, gup_flags);
- up_read(¤t->mm->mmap_sem);
- } else {
- /*
- * retain FAULT_FOLL_ALLOW_RETRY optimization if
- * possible
- */
- ret = get_user_pages_unlocked(start, nr_pages - nr,
- pages, gup_flags);
- }
+ __gup_longterm_unlocked();
/* Have to be a bit careful with return values */
if (nr > 0) {
^ permalink raw reply related
* Re: [PATCH v4 0/4] ocxl: OpenCAPI Cleanup
From: Frederic Barrat @ 2019-03-25 17:34 UTC (permalink / raw)
To: Greg Kurz, Alastair D'Silva
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, Andrew Donnellan,
alastair, linuxppc-dev
In-Reply-To: <20190325174919.5c877cf9@bahia.lab.toulouse-stg.fr.ibm.com>
Le 25/03/2019 à 17:49, Greg Kurz a écrit :
> Hi Alastair,
>
> I forgot to mention it during v3 but please don't link new version
> of a patchset to the previous one with --in-reply-to. This is to
> ensure I can see them in my email client without having to scroll
> back many days in the past (which likely means a fair number of
> e-mails on linuxppc-dev).
I'm also seeing the other series (ocxl refactoring) somehow under the
same thread. I haven't checked why, and there may be some mail client
bug in the way, I just mention it in case you see a reason for it in the
way you submit the patch set.
Fred
>
> Cheers,
>
> --
> Greg
>
> On Mon, 25 Mar 2019 16:34:51 +1100
> "Alastair D'Silva" <alastair@au1.ibm.com> wrote:
>
>> From: Alastair D'Silva <alastair@d-silva.org>
>>
>> Some minor cleanups for the OpenCAPI driver as a prerequisite
>> for an ocxl driver refactoring to allow the driver core to
>> be utilised by external drivers.
>>
>> Changelog:
>> V4:
>> - Drop printf format changes as they omit the format indicator for '0'
>> V3:
>> - Add missed header in 'ocxl: Remove some unused exported symbols'.
>> This addresses the introduced sparse warnings
>> V2:
>> - remove intermediate assignment of 'link' var in
>> 'Rename struct link to ocxl_link'
>> - Don't shift definition of ocxl_context_attach in
>> 'Remove some unused exported symbols'
>>
>>
>> Alastair D'Silva (4):
>> ocxl: Rename struct link to ocxl_link
>> ocxl: read_pasid never returns an error, so make it void
>> ocxl: Remove superfluous 'extern' from headers
>> ocxl: Remove some unused exported symbols
>>
>> drivers/misc/ocxl/config.c | 13 ++---
>> drivers/misc/ocxl/file.c | 5 +-
>> drivers/misc/ocxl/link.c | 36 ++++++-------
>> drivers/misc/ocxl/ocxl_internal.h | 85 +++++++++++++++++++------------
>> include/misc/ocxl.h | 53 ++++++-------------
>> 5 files changed, 91 insertions(+), 101 deletions(-)
>>
>
^ permalink raw reply
* Re: [PATCH 2/2] arch: add pidfd and io_uring syscalls everywhere
From: Paul Burton @ 2019-03-25 17:37 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Rich Felker, linux-ia64@vger.kernel.org, linux-sh@vger.kernel.org,
Heiko Carstens, linux-mips@vger.kernel.org,
James E . J . Bottomley, Max Filippov, Paul Mackerras,
sparclinux@vger.kernel.org, linux-s390@vger.kernel.org,
Helge Deller, Russell King, Geert Uytterhoeven, Catalin Marinas,
James Hogan, Firoz Khan, Matt Turner, Fenghua Yu, Will Deacon,
linux-m68k@lists.linux-m68k.org, Ivan Kokshaysky,
linux-arm-kernel@lists.infradead.org, Richard Henderson,
Michal Simek, Tony Luck, linux-parisc@vger.kernel.org,
linux-kernel@vger.kernel.org, Ralf Baechle,
linux-alpha@vger.kernel.org, Martin Schwidefsky, Andrew Morton,
linuxppc-dev@lists.ozlabs.org, David S . Miller
In-Reply-To: <20190325144737.703921-1-arnd@arndb.de>
Hi Arnd,
On Mon, Mar 25, 2019 at 03:47:37PM +0100, Arnd Bergmann wrote:
> Add the io_uring and pidfd_send_signal system calls to all architectures.
>
> These system calls are designed to handle both native and compat tasks,
> so all entries are the same across architectures, only arm-compat and
> the generic tale still use an old format.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>%
> diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
> index c85502e67b44..c4a49f7d57bb 100644
> --- a/arch/mips/kernel/syscalls/syscall_n64.tbl
> +++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
> @@ -338,3 +338,7 @@
> 327 n64 rseq sys_rseq
> 328 n64 io_pgetevents sys_io_pgetevents
> # 329 through 423 are reserved to sync up with other architectures
> +424 common pidfd_send_signal sys_pidfd_send_signal
> +425 common io_uring_setup sys_io_uring_setup
> +426 common io_uring_enter sys_io_uring_enter
> +427 common io_uring_register sys_io_uring_register
Shouldn't these declare the ABI as "n64"?
I don't see anywhere that it would actually change the generated code,
but a comment at the top of the file says that every entry should use
"n64" and so far they all do. Did you have something else in mind here?
Thanks,
Paul
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Jason Gunthorpe @ 2019-03-25 17:51 UTC (permalink / raw)
To: Ira Weiny
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, Dan Williams, linux-s390, Yoshinori Sato, linux-rdma,
Ingo Molnar, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <20190325092314.GF16366@iweiny-DESK2.sc.intel.com>
On Mon, Mar 25, 2019 at 02:23:15AM -0700, Ira Weiny wrote:
> > > Unfortunately holding the lock is required to support FOLL_LONGTERM (to check
> > > the VMAs) but we don't want to hold the lock to be optimal (specifically allow
> > > FAULT_FOLL_ALLOW_RETRY). So I'm maintaining the optimization for *_fast users
> > > who do not specify FOLL_LONGTERM.
> > >
> > > Another way to do this would have been to define __gup_longterm_unlocked with
> > > the above logic, but that seemed overkill at this point.
> >
> > get_user_pages_unlocked() is an exported symbol, shouldn't it work
> > with the FOLL_LONGTERM flag?
> >
> > I think it should even though we have no user..
> >
> > Otherwise the GUP API just gets more confusing.
>
> I agree WRT to the API. But I think callers of get_user_pages_unlocked() are
> not going to get the behavior they want if they specify FOLL_LONGTERM.
Oh? Isn't the only thing FOLL_LONGTERM does is block the call on DAX?
Why does the locking mode matter to this test?
> What I could do is BUG_ON (or just WARN_ON) if unlocked is called with
> FOLL_LONGTERM similar to the code in get_user_pages_locked() which does not
> allow locked and vmas to be passed together:
The GUP call should fail if you are doing something like this. But I'd
rather not see confusing specialc cases in code without a clear
comment explaining why it has to be there.
Jason
^ permalink raw reply
* Re: [PATCH v3] powerpc/64: Fix memcmp reading past the end of src/dest
From: Segher Boessenkool @ 2019-03-25 17:54 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev, chandan
In-Reply-To: <877ecnp1zf.fsf@concordia.ellerman.id.au>
On Mon, Mar 25, 2019 at 11:33:56PM +1100, Michael Ellerman wrote:
> Segher Boessenkool <segher@kernel.crashing.org> writes:
> > On Fri, Mar 22, 2019 at 11:37:24PM +1100, Michael Ellerman wrote:
> >> + clrldi r6,r4,(64-12) // r6 = r4 & 0xfff
> >
> > You can just write
> > rlwinm r6,r4,0,0x0fff
>
> > if that is clearer? Or do you still want a comment with that :-)
>
> I don't think it's clearer doing a rotate of zero bits :)
>
> And yeah I'd probably still leave the comment, so I'm inclined to stick
> with the clrldi?
I always have to think what the clrldi etc. do exactly, while with rlwinm
it is obvious. But yeah this may be different for other people who are
used to different idiom.
> Would be nice if the assembler could support:
>
> andi r6, r4, 0x0fff
>
> And turn it into the rlwinm, or rldicl :)
The extended mnemonics are *simple*, *one-to-one* mappings. Having
"andi. 6,4,0x0f0f" a valid insn, but an extended mnemonic "andi 6,4,0x0f0f"
that is not (and the other way around for say 0xff0000ff) would violate that.
You could do some assembler macro, that can also expand to multiple insns
where that is useful. Also one for loading constants, etc. The downside
to that is you often do care how many insns are generated.
Instead you could do a macro for only those cases that can be done with *one*
insn. But that then is pretty restricted in use, and people have to learn
what values are valid.
I don't see a perfect solution.
Segher
^ permalink raw reply
* Re: [RESEND 1/7] mm/gup: Replace get_user_pages_longterm() with FOLL_LONGTERM
From: Ira Weiny @ 2019-03-25 10:27 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, Michal Hocko, linux-mm, Rich Felker,
Paul Mackerras, sparclinux, linux-s390, Yoshinori Sato,
linux-rdma, Aneesh Kumar K . V, Jason Gunthorpe, Ingo Molnar,
linux-mips, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <CAA9_cmffz1VBOJ0ykBtcj+hiznn-kbbuotu1uUhPiJtXiFjJXg@mail.gmail.com>
On Fri, Mar 22, 2019 at 02:24:40PM -0700, Dan Williams wrote:
> On Sun, Mar 17, 2019 at 7:36 PM <ira.weiny@intel.com> wrote:
[snip]
> > + * __gup_longterm_locked() is a wrapper for __get_uer_pages_locked which
>
> s/uer/user/
>
> > + * allows us to process the FOLL_LONGTERM flag if present.
> > + *
> > + * FOLL_LONGTERM Checks for either DAX VMAs or PPC CMA regions and either fails
> > + * the pin or attempts to migrate the page as appropriate.
> > + *
> > + * In the filesystem-dax case mappings are subject to the lifetime enforced by
> > + * the filesystem and we need guarantees that longterm users like RDMA and V4L2
> > + * only establish mappings that have a kernel enforced revocation mechanism.
> > + *
> > + * In the CMA case pages can't be pinned in a CMA region as this would
> > + * unnecessarily fragment that region. So CMA attempts to migrate the page
> > + * before pinning.
> > *
> > * "longterm" == userspace controlled elevated page count lifetime.
> > * Contrast this to iov_iter_get_pages() usages which are transient.
>
> Ah, here's the longterm documentation, but if I was a developer
> considering whether to use FOLL_LONGTERM or not I would expect to find
> the documentation at the flag definition site.
>
> I think it has become more clear since get_user_pages_longterm() was
> initially merged that we need to warn people not to use it, or at
> least seriously reconsider whether they want an interface to support
> indefinite pins.
I will move the comment to the flag definition but...
In reviewing this comment it occurs to me that the addition of special casing
CMA regions via FOLL_LONGTERM has made it less experimental/temporary and now
simply implies intent to the GUP code as to the use of the pages.
As I'm not super familiar with the CMA use case I can't say for certain but it
seems that it is not a temporary solution.
So I'm not going to refrain from a FIXME WRT removing the flag.
New suggested text below.
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6831077d126c..5db9d8e894aa 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -2596,7 +2596,28 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
#define FOLL_REMOTE 0x2000 /* we are working on non-current tsk/mm */
#define FOLL_COW 0x4000 /* internal GUP flag */
#define FOLL_ANON 0x8000 /* don't do file mappings */
-#define FOLL_LONGTERM 0x10000 /* mapping is intended for a long term pin */
+#define FOLL_LONGTERM 0x10000 /* mapping lifetime is indefinite: see below */
+
+/*
+ * NOTE on FOLL_LONGTERM:
+ *
+ * FOLL_LONGTERM indicates that the page will be held for an indefinite time
+ * period _often_ under userspace control. This is contrasted with
+ * iov_iter_get_pages() where usages which are transient.
+ *
+ * FIXME: For pages which are part of a filesystem, mappings are subject to the
+ * lifetime enforced by the filesystem and we need guarantees that longterm
+ * users like RDMA and V4L2 only establish mappings which coordinate usage with
+ * the filesystem. Ideas for this coordination include revoking the longterm
+ * pin, delaying writeback, bounce buffer page writeback, etc. As FS DAX was
+ * added after the problem with filesystems was found FS DAX VMAs are
+ * specifically failed. Filesystem pages are still subject to bugs and use of
+ * FOLL_LONGTERM should be avoided on those pages.
+ *
+ * In the CMA case: longterm pins in a CMA region would unnecessarily fragment
+ * that region. And so CMA attempts to migrate the page before pinning when
+ * FOLL_LONGTERM is specified.
+ */
static inline int vm_fault_to_errno(vm_fault_t vm_fault, int foll_flags)
{
^ permalink raw reply related
* Re: powerpc/mm: Only define MAX_PHYSMEM_BITS in SPARSEMEM configurations
From: Andreas Schwab @ 2019-03-25 20:12 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Ben Hutchings, linuxppc-dev, Paul Mackerras
In-Reply-To: <87a7hjp4oi.fsf@concordia.ellerman.id.au>
On Mär 25 2019, Michael Ellerman <mpe@ellerman.id.au> wrote:
> So I'm inclined to just switch to always using SPARSEMEM on 64-bit
> Book3S, because that's what's well tested and we hardly need more code
> paths to test. Unless anyone has a strong objection, I haven't actually
> benchmarked FLATMEM vs SPARSEMEM on a G5.
Configuring with SPARSEMEM saves about 32Mb of memory.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply
* RE: [PATCH v4 0/4] ocxl: OpenCAPI Cleanup
From: Alastair D'Silva @ 2019-03-25 21:45 UTC (permalink / raw)
To: 'Frederic Barrat', 'Greg Kurz',
'Alastair D'Silva'
Cc: 'Andrew Donnellan', 'Greg Kroah-Hartman',
linuxppc-dev, linux-kernel, 'Arnd Bergmann'
In-Reply-To: <5d6792fe-f4db-b973-c654-460533f69f2f@linux.ibm.com>
> -----Original Message-----
> From: Frederic Barrat <fbarrat@linux.ibm.com>
> Sent: Tuesday, 26 March 2019 4:34 AM
> To: Greg Kurz <groug@kaod.org>; Alastair D'Silva <alastair@au1.ibm.com>
> Cc: alastair@d-silva.org; Arnd Bergmann <arnd@arndb.de>; Greg Kroah-
> Hartman <gregkh@linuxfoundation.org>; linux-kernel@vger.kernel.org;
> Andrew Donnellan <andrew.donnellan@au1.ibm.com>; linuxppc-
> dev@lists.ozlabs.org
> Subject: Re: [PATCH v4 0/4] ocxl: OpenCAPI Cleanup
>
>
>
> Le 25/03/2019 à 17:49, Greg Kurz a écrit :
> > Hi Alastair,
> >
> > I forgot to mention it during v3 but please don't link new version of
> > a patchset to the previous one with --in-reply-to. This is to ensure I
> > can see them in my email client without having to scroll back many
> > days in the past (which likely means a fair number of e-mails on
> > linuxppc-dev).
>
>
> I'm also seeing the other series (ocxl refactoring) somehow under the same
> thread. I haven't checked why, and there may be some mail client bug in the
> way, I just mention it in case you see a reason for it in the way you submit
> the patch set.
>
I probably grabbed the wrong message-id.
--
Alastair D'Silva mob: 0423 762 819
skype: alastair_dsilva msn: alastair@d-silva.org
blog: http://alastair.d-silva.org Twitter: @EvilDeece
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Ira Weiny @ 2019-03-25 14:21 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, Dan Williams, linux-s390, Yoshinori Sato, linux-rdma,
Ingo Molnar, John Hubbard, Borislav Petkov, Thomas Gleixner,
netdev@vger.kernel.org, Linux Kernel Mailing List, Ralf Baechle,
Martin Schwidefsky, Andrew Morton, linuxppc-dev, David S. Miller,
Kirill A. Shutemov
In-Reply-To: <20190325175150.GA21008@ziepe.ca>
On Mon, Mar 25, 2019 at 02:51:50PM -0300, Jason Gunthorpe wrote:
> On Mon, Mar 25, 2019 at 02:23:15AM -0700, Ira Weiny wrote:
> > > > Unfortunately holding the lock is required to support FOLL_LONGTERM (to check
> > > > the VMAs) but we don't want to hold the lock to be optimal (specifically allow
> > > > FAULT_FOLL_ALLOW_RETRY). So I'm maintaining the optimization for *_fast users
> > > > who do not specify FOLL_LONGTERM.
> > > >
> > > > Another way to do this would have been to define __gup_longterm_unlocked with
> > > > the above logic, but that seemed overkill at this point.
> > >
> > > get_user_pages_unlocked() is an exported symbol, shouldn't it work
> > > with the FOLL_LONGTERM flag?
> > >
> > > I think it should even though we have no user..
> > >
> > > Otherwise the GUP API just gets more confusing.
> >
> > I agree WRT to the API. But I think callers of get_user_pages_unlocked() are
> > not going to get the behavior they want if they specify FOLL_LONGTERM.
>
> Oh? Isn't the only thing FOLL_LONGTERM does is block the call on DAX?
From an API yes.
> Why does the locking mode matter to this test?
DAX checks for VMA's being Filesystem DAX. Therefore, it requires collection
of VMA's as the GUP code executes. The unlocked version can drop the lock and
therefore the VMAs may become invalid. Therefore, the 2 code paths are
incompatible.
Users of GUP unlocked are going to want the benefit of FAULT_FOLL_ALLOW_RETRY.
So I don't anticipate anyone using FOLL_LONGTERM with
get_user_pages_unlocked().
FWIW this thread is making me think my original patch which simply implemented
get_user_pages_fast_longterm() would be more clear. There is some evidence
that the GUP API was trending that way (see get_user_pages_remote). That seems
wrong but I don't know how to ensure users don't specify the wrong flag.
>
> > What I could do is BUG_ON (or just WARN_ON) if unlocked is called with
> > FOLL_LONGTERM similar to the code in get_user_pages_locked() which does not
> > allow locked and vmas to be passed together:
>
> The GUP call should fail if you are doing something like this. But I'd
> rather not see confusing specialc cases in code without a clear
> comment explaining why it has to be there.
Code comment would be necessary, sure. Was just throwing ideas out there.
Ira
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Dan Williams @ 2019-03-25 22:36 UTC (permalink / raw)
To: Ira Weiny
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, linux-s390, Yoshinori Sato, linux-rdma,
Jason Gunthorpe, Ingo Molnar, John Hubbard, Borislav Petkov,
Thomas Gleixner, netdev@vger.kernel.org,
Linux Kernel Mailing List, Ralf Baechle, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <20190325142125.GH16366@iweiny-DESK2.sc.intel.com>
On Mon, Mar 25, 2019 at 3:22 PM Ira Weiny <ira.weiny@intel.com> wrote:
[..]
> FWIW this thread is making me think my original patch which simply implemented
> get_user_pages_fast_longterm() would be more clear. There is some evidence
> that the GUP API was trending that way (see get_user_pages_remote). That seems
> wrong but I don't know how to ensure users don't specify the wrong flag.
What about just making the existing get_user_pages_longterm() have a
fast path option?
^ permalink raw reply
* Re: [RESEND 4/7] mm/gup: Add FOLL_LONGTERM capability to GUP fast
From: Ira Weiny @ 2019-03-25 14:54 UTC (permalink / raw)
To: Dan Williams
Cc: Michal Hocko, Linux-sh, Peter Zijlstra, James Hogan,
Heiko Carstens, linux-mips, linux-mm, Rich Felker, Paul Mackerras,
sparclinux, linux-s390, Yoshinori Sato, linux-rdma,
Jason Gunthorpe, Ingo Molnar, John Hubbard, Borislav Petkov,
Thomas Gleixner, netdev@vger.kernel.org,
Linux Kernel Mailing List, Ralf Baechle, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller, Kirill A. Shutemov
In-Reply-To: <CAPcyv4hG8WDhsWinXHYkReHKS6gdQ3gAHMcfVWvuP4c4SYBzXQ@mail.gmail.com>
On Mon, Mar 25, 2019 at 03:36:28PM -0700, Dan Williams wrote:
> On Mon, Mar 25, 2019 at 3:22 PM Ira Weiny <ira.weiny@intel.com> wrote:
> [..]
> > FWIW this thread is making me think my original patch which simply implemented
> > get_user_pages_fast_longterm() would be more clear. There is some evidence
> > that the GUP API was trending that way (see get_user_pages_remote). That seems
> > wrong but I don't know how to ensure users don't specify the wrong flag.
>
> What about just making the existing get_user_pages_longterm() have a
> fast path option?
That would work but was not the direction we agreed upon before.[1]
At this point I would rather see this patch set applied, focus on fixing the
filesystem issues, and once that is done determine if FOLL_LONGTERM is needed
in any GUP calls.
Ira
[1] https://lkml.org/lkml/2019/2/11/2038
^ permalink raw reply
* Bad file pattern in MAINTAINERS section 'IBM Power Virtual Accelerator Switchboard'
From: Joe Perches @ 2019-03-25 23:35 UTC (permalink / raw)
To: linux-kernel; +Cc: Sukadev Bhattiprolu, linuxppc-dev
In-Reply-To: <7cd8d12f59bcacd18a78f599b46dac555f7f16c0.camel@perches.com>
A file pattern line in this section of the MAINTAINERS file in linux-next
does not have a match in the linux source files.
This could occur because a matching filename was never added, was deleted
or renamed in some other commit.
The commits that added and if found renamed or removed the file pattern
are shown below.
Please fix this defect appropriately.
1: ---------------------------------------------------------------------------
linux-next MAINTAINERS section:
7396 IBM Power Virtual Accelerator Switchboard
7397 M: Sukadev Bhattiprolu
7398 L: linuxppc-dev@lists.ozlabs.org
7399 S: Supported
7400 F: arch/powerpc/platforms/powernv/vas*
7401 F: arch/powerpc/platforms/powernv/copy-paste.h
7402 F: arch/powerpc/include/asm/vas.h
--> 7403 F: arch/powerpc/include/uapi/asm/vas.h
2: ---------------------------------------------------------------------------
The most recent commit that added or modified file pattern 'arch/powerpc/include/uapi/asm/vas.h':
commit 4dea2d1a927c61114a168d4509b56329ea6effb7
Author: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Date: Mon Aug 28 23:23:33 2017 -0700
powerpc/powernv/vas: Define vas_init() and vas_exit()
Implement vas_init() and vas_exit() functions for a new VAS module.
This VAS module is essentially a library for other device drivers
and kernel users of the NX coprocessors like NX-842 and NX-GZIP.
In the future this will be extended to add support for user space
to access the NX coprocessors.
VAS is currently only supported with 64K page size.
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
.../devicetree/bindings/powerpc/ibm,vas.txt | 22 +++
MAINTAINERS | 8 ++
arch/powerpc/platforms/powernv/Kconfig | 14 ++
arch/powerpc/platforms/powernv/Makefile | 1 +
arch/powerpc/platforms/powernv/vas-window.c | 19 +++
arch/powerpc/platforms/powernv/vas.c | 151 +++++++++++++++++++++
arch/powerpc/platforms/powernv/vas.h | 2 +
7 files changed, 217 insertions(+)
3: ---------------------------------------------------------------------------
No commit with file pattern 'arch/powerpc/include/uapi/asm/vas.h' was found
^ permalink raw reply
* Re: Bad file pattern in MAINTAINERS section 'IBM Power Virtual Accelerator Switchboard'
From: Joe Perches @ 2019-03-25 23:38 UTC (permalink / raw)
To: linux-kernel; +Cc: Sukadev Bhattiprolu, linuxppc-dev
In-Reply-To: <20190325233521.31094-1-joe@perches.com>
On Mon, 2019-03-25 at 16:35 -0700, Joe Perches wrote:
> A file pattern line in this section of the MAINTAINERS file in linux-next
> does not have a match in the linux source files.
>
> This could occur because a matching filename was never added, was deleted
> or renamed in some other commit.
>
> The commits that added and if found renamed or removed the file pattern
> are shown below.
>
> Please fix this defect appropriately.
>
> 1: ---------------------------------------------------------------------------
>
> linux-next MAINTAINERS section:
>
> 7396 IBM Power Virtual Accelerator Switchboard
> 7397 M: Sukadev Bhattiprolu
btw: Your name needs an email address here too.
This should be:
M: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com>
^ permalink raw reply
* Re: [PATCH] powerpc: vmlinux.lds: Drop Binutils 2.18 workarounds
From: Michael Ellerman @ 2019-03-26 0:55 UTC (permalink / raw)
To: Joel Stanley, linuxppc-dev
In-Reply-To: <20190321003253.22100-1-joel@jms.id.au>
Joel Stanley <joel@jms.id.au> writes:
> Segher added some workarounds for GCC 4.2 and bintuils 2.18. We now set
> 4.6 and 2.20 as the minimum, so they can be dropped.
>
> This is mostly a revert of c69cccc95fe4 ("powerpc: Fix build bug with
> binutils < 2.18 and GCC < 4.2").
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> arch/powerpc/kernel/vmlinux.lds.S | 35 ++++---------------------------
> 1 file changed, 4 insertions(+), 31 deletions(-)
Seems this breaks some toolchains, at least the one from kernel.org:
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/powerpc64-linux/bin/powerpc64-linux-ld: .tmp_vmlinux1: Not enough room for program headers, try linking with -N
/opt/cross/kisskb/korg/gcc-8.1.0-nolibc/powerpc64-linux/bin/powerpc64-linux-ld: final link failed: Bad value
make[1]: *** [/kisskb/src/Makefile:1024: vmlinux] Error 1
http://kisskb.ellerman.id.au/kisskb/buildresult/13743374/
Not sure why.
That's binutils 2.30.
cheers
^ permalink raw reply
* 答复: Re: [PATCH v2] powerpc/8xx: fix possible object reference leak
From: wen.yang99 @ 2019-03-26 1:19 UTC (permalink / raw)
To: mpe; +Cc: peng.hao2, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <87d0mfp6w6.fsf@concordia.ellerman.id.au>
[-- Attachment #1.1: Type: text/plain, Size: 1935 bytes --]
>> The call to of_find_compatible_node returns a node pointer with refcount
>> incremented thus it must be explicitly decremented after the last
>> usage.
>> irq_domain_add_linear also calls of_node_get to increase refcount,
>> so irq_domain will not be affected when it is released.
>>
>> Detected by coccinelle with the following warnings:
>> ./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 136, but without a corresponding object release within this function.
>>
>> Fixes: a8db8cf0d894 ("irq_domain: Replace irq_alloc_host() with
>> revmap-specific initializers")
>> Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
>> Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> Reviewed-by: Peng Hao <peng.hao2@zte.com.cn>
>> Cc: Vitaly Bordug <vitb@kernel.crashing.org>
>> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>> Cc: Paul Mackerras <paulus@samba.org>
>> Cc: Michael Ellerman <mpe@ellerman.id.au>
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-kernel@vger.kernel.org
>> ---
>> arch/powerpc/platforms/8xx/pic.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
>> index 8d5a25d..4453df6 100644
>> --- a/arch/powerpc/platforms/8xx/pic.c
>> +++ b/arch/powerpc/platforms/8xx/pic.c
>> @@ -153,9 +153,7 @@ int mpc8xx_pic_init(void)
>> if (mpc8xx_pic_host == NULL) {
>> printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
>> ret = -ENOMEM;
>> - goto out;
>> }
>> - return 0;
>
> It's fragile to rely on ret being equal to zero here. If the code
> further up is changed it's easy enough to miss that ret is expected to
> be zero.
>
> Better to set it to zero here explicitly, this is the success path after
> all, eg:
>
> ret = 0;
>
Thank you for your comments.
I'll fix it soon.
Thanks and regards,
Wen
^ permalink raw reply
* [PATCH v3] powerpc/8xx: fix possible object reference leak
From: Peng Hao @ 2019-03-26 10:29 UTC (permalink / raw)
To: vitb, benh, paulus, mpe, christophe.leroy
Cc: linuxppc-dev, Wen Yang, linux-kernel
From: Wen Yang <wen.yang99@zte.com.cn>
The call to of_find_compatible_node returns a node pointer with refcount
incremented thus it must be explicitly decremented after the last
usage.
irq_domain_add_linear also calls of_node_get to increase refcount,
so irq_domain will not be affected when it is released.
Detected by coccinelle with the following warnings:
./arch/powerpc/platforms/8xx/pic.c:158:1-7: ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line 136, but without a corresponding object release within this function.
Fixes: a8db8cf0d894 ("irq_domain: Replace irq_alloc_host() with
revmap-specific initializers")
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Suggested-by: Christophe Leroy <christophe.leroy@c-s.fr>
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Peng Hao <peng.hao2@zte.com.cn>
Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Vitaly Bordug <vitb@kernel.crashing.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-kernel@vger.kernel.org
---
v3->v2: set ret to zero explicitly.
v2->v1: add a Fixes tag.
arch/powerpc/platforms/8xx/pic.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/powerpc/platforms/8xx/pic.c b/arch/powerpc/platforms/8xx/pic.c
index 8d5a25d..9993998 100644
--- a/arch/powerpc/platforms/8xx/pic.c
+++ b/arch/powerpc/platforms/8xx/pic.c
@@ -153,9 +153,9 @@ int mpc8xx_pic_init(void)
if (mpc8xx_pic_host == NULL) {
printk(KERN_ERR "MPC8xx PIC: failed to allocate irq host!\n");
ret = -ENOMEM;
- goto out;
}
- return 0;
+
+ ret = 0;
out:
of_node_put(np);
--
2.9.5
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox