On Thu, Mar 28, 2019 at 09:12:21AM -0700, Ira Weiny wrote:
> On Mon, Mar 25, 2019 at 10:40:06AM -0400, Jerome Glisse wrote:
> > From: Jérôme Glisse <jgli...@redhat.com>
> >
> > A common use case for HMM mirror is user trying to mirror a range
> > and before they could program the hardware it get invalidated by
> > some core mm event. Instead of having user re-try right away to
> > mirror the range provide a completion mechanism for them to wait
> > for any active invalidation affecting the range.
> >
> > This also changes how hmm_range_snapshot() and hmm_range_fault()
> > works by not relying on vma so that we can drop the mmap_sem
> > when waiting and lookup the vma again on retry.
> >
> > Changes since v1:
> > - squashed: Dan Carpenter: potential deadlock in nonblocking code
> >
> > Signed-off-by: Jérôme Glisse <jgli...@redhat.com>
> > Reviewed-by: Ralph Campbell <rcampb...@nvidia.com>
> > Cc: Andrew Morton <a...@linux-foundation.org>
> > Cc: John Hubbard <jhubb...@nvidia.com>
> > Cc: Dan Williams <dan.j.willi...@intel.com>
> > Cc: Dan Carpenter <dan.carpen...@oracle.com>
> > Cc: Matthew Wilcox <wi...@infradead.org>
> > ---
> > include/linux/hmm.h | 208 ++++++++++++++---
> > mm/hmm.c | 528 +++++++++++++++++++++-----------------------
> > 2 files changed, 428 insertions(+), 308 deletions(-)
> >
> > diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> > index e9afd23c2eac..79671036cb5f 100644
> > --- a/include/linux/hmm.h
> > +++ b/include/linux/hmm.h
> > @@ -77,8 +77,34 @@
> > #include <linux/migrate.h>
> > #include <linux/memremap.h>
> > #include <linux/completion.h>
> > +#include <linux/mmu_notifier.h>
> >
> > -struct hmm;
> > +
> > +/*
> > + * struct hmm - HMM per mm struct
> > + *
> > + * @mm: mm struct this HMM struct is bound to
> > + * @lock: lock protecting ranges list
> > + * @ranges: list of range being snapshotted
> > + * @mirrors: list of mirrors for this mm
> > + * @mmu_notifier: mmu notifier to track updates to CPU page table
> > + * @mirrors_sem: read/write semaphore protecting the mirrors list
> > + * @wq: wait queue for user waiting on a range invalidation
> > + * @notifiers: count of active mmu notifiers
> > + * @dead: is the mm dead ?
> > + */
> > +struct hmm {
> > + struct mm_struct *mm;
> > + struct kref kref;
> > + struct mutex lock;
> > + struct list_head ranges;
> > + struct list_head mirrors;
> > + struct mmu_notifier mmu_notifier;
> > + struct rw_semaphore mirrors_sem;
> > + wait_queue_head_t wq;
> > + long notifiers;
> > + bool dead;
> > +};
> >
> > /*
> > * hmm_pfn_flag_e - HMM flag enums
> > @@ -155,6 +181,38 @@ struct hmm_range {
> > bool valid;
> > };
> >
> > +/*
> > + * hmm_range_wait_until_valid() - wait for range to be valid
> > + * @range: range affected by invalidation to wait on
> > + * @timeout: time out for wait in ms (ie abort wait after that period of
> > time)
> > + * Returns: true if the range is valid, false otherwise.
> > + */
> > +static inline bool hmm_range_wait_until_valid(struct hmm_range *range,
> > + unsigned long timeout)
> > +{
> > + /* Check if mm is dead ? */
> > + if (range->hmm == NULL || range->hmm->dead || range->hmm->mm == NULL) {
> > + range->valid = false;
> > + return false;
> > + }
> > + if (range->valid)
> > + return true;
> > + wait_event_timeout(range->hmm->wq, range->valid || range->hmm->dead,
> > + msecs_to_jiffies(timeout));
> > + /* Return current valid status just in case we get lucky */
> > + return range->valid;
> > +}
> > +
> > +/*
> > + * hmm_range_valid() - test if a range is valid or not
> > + * @range: range
> > + * Returns: true if the range is valid, false otherwise.
> > + */
> > +static inline bool hmm_range_valid(struct hmm_range *range)
> > +{
> > + return range->valid;
> > +}
> > +
> > /*
> > * hmm_pfn_to_page() - return struct page pointed to by a valid HMM pfn
> > * @range: range use to decode HMM pfn value
> > @@ -357,51 +415,133 @@ void hmm_mirror_unregister(struct hmm_mirror
> > *mirror);
> >
> >
> > /*
> > - * To snapshot the CPU page table, call hmm_vma_get_pfns(), then take a
> > device
> > - * driver lock that serializes device page table updates, then call
> > - * hmm_vma_range_done(), to check if the snapshot is still valid. The same
> > - * device driver page table update lock must also be used in the
> > - * hmm_mirror_ops.sync_cpu_device_pagetables() callback, so that CPU page
> > - * table invalidation serializes on it.
> > + * To snapshot the CPU page table you first have to call
> > hmm_range_register()
> > + * to register the range. If hmm_range_register() return an error then
> > some-
> > + * thing is horribly wrong and you should fail loudly. If it returned true
> > then
> > + * you can wait for the range to be stable with
> > hmm_range_wait_until_valid()
> > + * function, a range is valid when there are no concurrent changes to the
> > CPU
> > + * page table for the range.
> > + *
> > + * Once the range is valid you can call hmm_range_snapshot() if that
> > returns
> > + * without error then you can take your device page table lock (the same
> > lock
> > + * you use in the HMM mirror sync_cpu_device_pagetables() callback). After
> > + * taking that lock you have to check the range validity, if it is still
> > valid
> > + * (ie hmm_range_valid() returns true) then you can program the device page
> > + * table, otherwise you have to start again. Pseudo code:
> > + *
> > + * mydevice_prefault(mydevice, mm, start, end)
> > + * {
> > + * struct hmm_range range;
> > + * ...
> > *
> > - * YOU MUST CALL hmm_vma_range_done() ONCE AND ONLY ONCE EACH TIME YOU CALL
> > - * hmm_range_snapshot() WITHOUT ERROR !
> > + * ret = hmm_range_register(&range, mm, start, end);
> > + * if (ret)
> > + * return ret;
> > *
> > - * IF YOU DO NOT FOLLOW THE ABOVE RULE THE SNAPSHOT CONTENT MIGHT BE
> > INVALID !
> > - */
> > -long hmm_range_snapshot(struct hmm_range *range);
> > -bool hmm_vma_range_done(struct hmm_range *range);
> > -
> > -
> > -/*
> > - * Fault memory on behalf of device driver. Unlike handle_mm_fault(), this
> > will
> > - * not migrate any device memory back to system memory. The HMM pfn array
> > will
> > - * be updated with the fault result and current snapshot of the CPU page
> > table
> > - * for the range.
> > + * down_read(mm->mmap_sem);
> > + * again:
> > + *
> > + * if (!hmm_range_wait_until_valid(&range, TIMEOUT)) {
> > + * up_read(&mm->mmap_sem);
> > + * hmm_range_unregister(range);
> > + * // Handle time out, either sleep or retry or something else
> > + * ...
> > + * return -ESOMETHING; || goto again;
> > + * }
> > + *
> > + * ret = hmm_range_snapshot(&range); or hmm_range_fault(&range);
> > + * if (ret == -EAGAIN) {
> > + * down_read(mm->mmap_sem);
> > + * goto again;
> > + * } else if (ret == -EBUSY) {
> > + * goto again;
> > + * }
> > + *
> > + * up_read(&mm->mmap_sem);
> > + * if (ret) {
> > + * hmm_range_unregister(range);
> > + * return ret;
> > + * }
> > + *
> > + * // It might not have snap-shoted the whole range but only the
> > first
> > + * // npages, the return values is the number of valid pages from
> > the
> > + * // start of the range.
> > + * npages = ret;
> > *
> > - * The mmap_sem must be taken in read mode before entering and it might be
> > - * dropped by the function if the block argument is false. In that case,
> > the
> > - * function returns -EAGAIN.
> > + * ...
> > *
> > - * Return value does not reflect if the fault was successful for every
> > single
> > - * address or not. Therefore, the caller must to inspect the HMM pfn array
> > to
> > - * determine fault status for each address.
> > + * mydevice_page_table_lock(mydevice);
> > + * if (!hmm_range_valid(range)) {
> > + * mydevice_page_table_unlock(mydevice);
> > + * goto again;
> > + * }
> > *
> > - * Trying to fault inside an invalid vma will result in -EINVAL.
> > + * mydevice_populate_page_table(mydevice, range, npages);
> > + * ...
> > + * mydevice_take_page_table_unlock(mydevice);
> > + * hmm_range_unregister(range);
> > *
> > - * See the function description in mm/hmm.c for further documentation.
> > + * return 0;
> > + * }
> > + *
> > + * The same scheme apply to hmm_range_fault() (ie replace
> > hmm_range_snapshot()
> > + * with hmm_range_fault() in above pseudo code).
> > + *
> > + * YOU MUST CALL hmm_range_unregister() ONCE AND ONLY ONCE EACH TIME YOU
> > CALL
> > + * hmm_range_register() AND hmm_range_register() RETURNED TRUE ! IF YOU DO
> > NOT
> > + * FOLLOW THIS RULE MEMORY CORRUPTION WILL ENSUE !
> > */
> > +int hmm_range_register(struct hmm_range *range,
> > + struct mm_struct *mm,
> > + unsigned long start,
> > + unsigned long end);
> > +void hmm_range_unregister(struct hmm_range *range);
>
> The above comment is great! But I think you also need to update
> Documentation/vm/hmm.rst:hmm_range_snapshot() to show the use of
> hmm_range_[un]register()
>
> > +long hmm_range_snapshot(struct hmm_range *range);
> > long hmm_range_fault(struct hmm_range *range, bool block);
> >
> > +/*
> > + * HMM_RANGE_DEFAULT_TIMEOUT - default timeout (ms) when waiting for a
> > range
> > + *
> > + * When waiting for mmu notifiers we need some kind of time out otherwise
> > we
> > + * could potentialy wait for ever, 1000ms ie 1s sounds like a long time to
> > + * wait already.
> > + */
> > +#define HMM_RANGE_DEFAULT_TIMEOUT 1000
> > +
> > /* This is a temporary helper to avoid merge conflict between trees. */
> > +static inline bool hmm_vma_range_done(struct hmm_range *range)
> > +{
> > + bool ret = hmm_range_valid(range);
> > +
> > + hmm_range_unregister(range);
> > + return ret;
> > +}
> > +
> > static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> > {
> > - long ret = hmm_range_fault(range, block);
> > - if (ret == -EBUSY)
> > - ret = -EAGAIN;
> > - else if (ret == -EAGAIN)
> > - ret = -EBUSY;
> > - return ret < 0 ? ret : 0;
> > + long ret;
> > +
> > + ret = hmm_range_register(range, range->vma->vm_mm,
> > + range->start, range->end);
> > + if (ret)
> > + return (int)ret;
> > +
> > + if (!hmm_range_wait_until_valid(range, HMM_RANGE_DEFAULT_TIMEOUT)) {
> > + up_read(&range->vma->vm_mm->mmap_sem);
> > + return -EAGAIN;
> > + }
> > +
> > + ret = hmm_range_fault(range, block);
> > + if (ret <= 0) {
> > + if (ret == -EBUSY || !ret) {
> > + up_read(&range->vma->vm_mm->mmap_sem);
> > + ret = -EBUSY;
> > + } else if (ret == -EAGAIN)
> > + ret = -EBUSY;
> > + hmm_range_unregister(range);
> > + return ret;
> > + }
> > + return 0;
>
> Is hmm_vma_fault() also temporary to keep the nouveau driver working? It
> looks
> like it to me.
>
> This and hmm_vma_range_done() above are part of the old interface which is in
> the Documentation correct? As stated above we should probably change that
> documentation with this patch to ensure no new users of these 2 functions
> appear.
Ok will update the documentation, note that i already posted patches to use
this new API see the ODP RDMA link in the cover letter.
Cheers,
Jérôme
