* Re: [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
From: John Hubbard @ 2019-06-07 3:40 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-11-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> This list is always read and written while holding hmm->lock so there is
> no need for the confusing _rcu annotations.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> mm/hmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index c2fecb3ecb11e1..709d138dd49027 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range,
> mutex_lock(&hmm->lock);
>
> range->hmm = hmm;
> - list_add_rcu(&range->list, &hmm->ranges);
> + list_add(&range->list, &hmm->ranges);
>
> /*
> * If there are any concurrent notifiers we have to wait for them for
> @@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range)
> return;
>
> mutex_lock(&hmm->lock);
> - list_del_rcu(&range->list);
> + list_del(&range->list);
> mutex_unlock(&hmm->lock);
>
> /* Drop reference taken by hmm_range_register() */
>
Good point. I'd overlooked this many times.
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
From: John Hubbard @ 2019-06-07 3:37 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-10-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON
> and poison bytes to detect this condition.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> v2
> - Keep range start/end valid after unregistration (Jerome)
> ---
> mm/hmm.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 6802de7080d172..c2fecb3ecb11e1 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range)
> struct hmm *hmm = range->hmm;
>
> /* Sanity check this really should not happen. */
That comment can also be deleted, as it has the same meaning as
the WARN_ON() that you just added.
> - if (hmm == NULL || range->end <= range->start)
> + if (WARN_ON(range->end <= range->start))
> return;
>
> mutex_lock(&hmm->lock);
> @@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range)
> range->valid = false;
> mmput(hmm->mm);
> hmm_put(hmm);
> - range->hmm = NULL;
> +
> + /* The range is now invalid, leave it poisoned. */
To be precise, we are poisoning the range's back pointer to it's
owning hmm instance. Maybe this is clearer:
/*
* The range is now invalid, so poison it's hmm pointer.
* Leave other range-> fields in place, for the caller's use.
*/
...or something like that?
> + range->valid = false;
> + memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
> }
> EXPORT_SYMBOL(hmm_range_unregister);
>
>
The above are very minor documentation points, so:
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 08/11] mm/hmm: Remove racy protection against double-unregistration
From: John Hubbard @ 2019-06-07 3:29 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-9-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> No other register/unregister kernel API attempts to provide this kind of
> protection as it is inherently racy, so just drop it.
>
> Callers should provide their own protection, it appears nouveau already
> does, but just in case drop a debugging POISON.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> mm/hmm.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index c702cd72651b53..6802de7080d172 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -284,18 +284,13 @@ EXPORT_SYMBOL(hmm_mirror_register);
> */
> void hmm_mirror_unregister(struct hmm_mirror *mirror)
> {
> - struct hmm *hmm = READ_ONCE(mirror->hmm);
> -
> - if (hmm == NULL)
> - return;
> + struct hmm *hmm = mirror->hmm;
>
> down_write(&hmm->mirrors_sem);
> list_del_init(&mirror->list);
> - /* To protect us against double unregister ... */
> - mirror->hmm = NULL;
> up_write(&hmm->mirrors_sem);
> -
> hmm_put(hmm);
> + memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
I hadn't thought of POISON_* for these types of cases, it's a
good technique to remember.
I noticed that this is now done outside of the lock, but that
follows directly from your commit description, so that all looks
correct.
> }
> EXPORT_SYMBOL(hmm_mirror_unregister);
>
>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 07/11] mm/hmm: Use lockdep instead of comments
From: John Hubbard @ 2019-06-07 3:19 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-8-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> So we can check locking at runtime.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> v2
> - Fix missing & in lockdeps (Jason)
> ---
> mm/hmm.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index f67ba32983d9f1..c702cd72651b53 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -254,11 +254,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = {
> *
> * To start mirroring a process address space, the device driver must register
> * an HMM mirror struct.
> - *
> - * THE mm->mmap_sem MUST BE HELD IN WRITE MODE !
> */
> int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
> {
> + lockdep_assert_held_exclusive(&mm->mmap_sem);
> +
> /* Sanity check */
> if (!mm || !mirror || !mirror->ops)
> return -EINVAL;
>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 06/11] mm/hmm: Hold on to the mmget for the lifetime of the range
From: John Hubbard @ 2019-06-07 3:15 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-7-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> Range functions like hmm_range_snapshot() and hmm_range_fault() call
> find_vma, which requires hodling the mmget() and the mmap_sem for the mm.
>
> Make this simpler for the callers by holding the mmget() inside the range
> for the lifetime of the range. Other functions that accept a range should
> only be called if the range is registered.
>
> This has the side effect of directly preventing hmm_release() from
> happening while a range is registered. That means range->dead cannot be
> false during the lifetime of the range, so remove dead and
> hmm_mirror_mm_is_alive() entirely.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> v2:
> - Use Jerome's idea of just holding the mmget() for the range lifetime,
> rework the patch to use that as as simplification to remove dead in
> one step
> ---
> include/linux/hmm.h | 26 --------------------------
> mm/hmm.c | 28 ++++++++++------------------
> 2 files changed, 10 insertions(+), 44 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 2ab35b40992b24..0e20566802967a 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -91,7 +91,6 @@
> * @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;
> @@ -104,7 +103,6 @@ struct hmm {
> wait_queue_head_t wq;
> struct rcu_head rcu;
> long notifiers;
> - bool dead;
> };
>
> /*
> @@ -469,30 +467,6 @@ struct hmm_mirror {
> int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm);
> void hmm_mirror_unregister(struct hmm_mirror *mirror);
>
> -/*
> - * hmm_mirror_mm_is_alive() - test if mm is still alive
> - * @mirror: the HMM mm mirror for which we want to lock the mmap_sem
> - * Return: false if the mm is dead, true otherwise
> - *
> - * This is an optimization, it will not always accurately return false if the
> - * mm is dead; i.e., there can be false negatives (process is being killed but
> - * HMM is not yet informed of that). It is only intended to be used to optimize
> - * out cases where the driver is about to do something time consuming and it
> - * would be better to skip it if the mm is dead.
> - */
> -static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
> -{
> - struct mm_struct *mm;
> -
> - if (!mirror || !mirror->hmm)
> - return false;
> - mm = READ_ONCE(mirror->hmm->mm);
> - if (mirror->hmm->dead || !mm)
> - return false;
> -
> - return true;
> -}
> -
> /*
> * Please see Documentation/vm/hmm.rst for how to use the range API.
> */
> diff --git a/mm/hmm.c b/mm/hmm.c
> index dc30edad9a8a02..f67ba32983d9f1 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -80,7 +80,6 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> mutex_init(&hmm->lock);
> kref_init(&hmm->kref);
> hmm->notifiers = 0;
> - hmm->dead = false;
> hmm->mm = mm;
>
> hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
> @@ -124,20 +123,17 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> - struct hmm_range *range;
>
> /* hmm is in progress to free */
> if (!kref_get_unless_zero(&hmm->kref))
> return;
>
> - /* Report this HMM as dying. */
> - hmm->dead = true;
> -
> - /* Wake-up everyone waiting on any range. */
> mutex_lock(&hmm->lock);
> - list_for_each_entry(range, &hmm->ranges, list)
> - range->valid = false;
> - wake_up_all(&hmm->wq);
> + /*
> + * Since hmm_range_register() holds the mmget() lock hmm_release() is
> + * prevented as long as a range exists.
> + */
> + WARN_ON(!list_empty(&hmm->ranges));
> mutex_unlock(&hmm->lock);
>
> down_write(&hmm->mirrors_sem);
> @@ -909,8 +905,8 @@ int hmm_range_register(struct hmm_range *range,
> range->start = start;
> range->end = end;
>
> - /* Check if hmm_mm_destroy() was call. */
> - if (hmm->mm == NULL || hmm->dead)
> + /* Prevent hmm_release() from running while the range is valid */
> + if (!mmget_not_zero(hmm->mm))
> return -EFAULT;
>
> range->hmm = hmm;
> @@ -955,6 +951,7 @@ void hmm_range_unregister(struct hmm_range *range)
>
> /* Drop reference taken by hmm_range_register() */
> range->valid = false;
> + mmput(hmm->mm);
> hmm_put(hmm);
> range->hmm = NULL;
> }
> @@ -982,10 +979,7 @@ long hmm_range_snapshot(struct hmm_range *range)
> struct vm_area_struct *vma;
> struct mm_walk mm_walk;
>
> - /* Check if hmm_mm_destroy() was call. */
> - if (hmm->mm == NULL || hmm->dead)
> - return -EFAULT;
> -
> + lockdep_assert_held(&hmm->mm->mmap_sem);
> do {
> /* If range is no longer valid force retry. */
> if (!range->valid)
> @@ -1080,9 +1074,7 @@ long hmm_range_fault(struct hmm_range *range, bool block)
> struct mm_walk mm_walk;
> int ret;
>
> - /* Check if hmm_mm_destroy() was call. */
> - if (hmm->mm == NULL || hmm->dead)
> - return -EFAULT;
> + lockdep_assert_held(&hmm->mm->mmap_sem);
>
> do {
> /* If range is no longer valid force retry. */
>
Nice cleanup.
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: John Hubbard @ 2019-06-07 3:06 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-6-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> The wait_event_timeout macro already tests the condition as its first
> action, so there is no reason to open code another version of this, all
> that does is skip the might_sleep() debugging in common cases, which is
> not helpful.
>
> Further, based on prior patches, we can no simplify the required condition
"now simplify"
> test:
> - If range is valid memory then so is range->hmm
> - If hmm_release() has run then range->valid is set to false
> at the same time as dead, so no reason to check both.
> - A valid hmm has a valid hmm->mm.
>
> Also, add the READ_ONCE for range->valid as there is no lock held here.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> include/linux/hmm.h | 12 ++----------
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 4ee3acabe5ed22..2ab35b40992b24 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -218,17 +218,9 @@ static inline unsigned long hmm_range_page_size(const struct hmm_range *range)
> 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,
> + wait_event_timeout(range->hmm->wq, range->valid,
> msecs_to_jiffies(timeout));
> - /* Return current valid status just in case we get lucky */
> - return range->valid;
> + return READ_ONCE(range->valid);
Just to ensure that I actually understand the model: I'm assuming that the
READ_ONCE is there solely to ensure that range->valid is read *after* the
wait_event_timeout() returns. Is that correct?
> }
>
> /*
>
In any case, it looks good, so:
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 04/11] mm/hmm: Simplify hmm_get_or_create and make it reliable
From: John Hubbard @ 2019-06-07 2:54 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel
In-Reply-To: <20190606184438.31646-5-jgg@ziepe.ca>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> As coded this function can false-fail in various racy situations. Make it
> reliable by running only under the write side of the mmap_sem and avoiding
> the false-failing compare/exchange pattern.
>
> Also make the locking very easy to understand by only ever reading or
> writing mm->hmm while holding the write side of the mmap_sem.
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> v2:
> - Fix error unwind of mmgrab (Jerome)
> - Use hmm local instead of 2nd container_of (Jerome)
> ---
> mm/hmm.c | 80 ++++++++++++++++++++------------------------------------
> 1 file changed, 29 insertions(+), 51 deletions(-)
>
> diff --git a/mm/hmm.c b/mm/hmm.c
> index cc7c26fda3300e..dc30edad9a8a02 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -40,16 +40,6 @@
> #if IS_ENABLED(CONFIG_HMM_MIRROR)
> static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
>
> -static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> -{
> - struct hmm *hmm = READ_ONCE(mm->hmm);
> -
> - if (hmm && kref_get_unless_zero(&hmm->kref))
> - return hmm;
> -
> - return NULL;
> -}
> -
> /**
> * hmm_get_or_create - register HMM against an mm (HMM internal)
> *
> @@ -64,11 +54,20 @@ static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> */
> static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> {
> - struct hmm *hmm = mm_get_hmm(mm);
> - bool cleanup = false;
> + struct hmm *hmm;
>
> - if (hmm)
> - return hmm;
> + lockdep_assert_held_exclusive(&mm->mmap_sem);
> +
> + if (mm->hmm) {
> + if (kref_get_unless_zero(&mm->hmm->kref))
> + return mm->hmm;
> + /*
> + * The hmm is being freed by some other CPU and is pending a
> + * RCU grace period, but this CPU can NULL now it since we
> + * have the mmap_sem.
> + */
> + mm->hmm = NULL;
> + }
>
> hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
> if (!hmm)
> @@ -83,57 +82,36 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> hmm->notifiers = 0;
> hmm->dead = false;
> hmm->mm = mm;
> - mmgrab(hmm->mm);
> -
> - spin_lock(&mm->page_table_lock);
> - if (!mm->hmm)
> - mm->hmm = hmm;
> - else
> - cleanup = true;
> - spin_unlock(&mm->page_table_lock);
>
> - if (cleanup)
> - goto error;
> -
> - /*
> - * We should only get here if hold the mmap_sem in write mode ie on
> - * registration of first mirror through hmm_mirror_register()
> - */
> hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
> - if (__mmu_notifier_register(&hmm->mmu_notifier, mm))
> - goto error_mm;
> + if (__mmu_notifier_register(&hmm->mmu_notifier, mm)) {
> + kfree(hmm);
> + return NULL;
> + }
>
> + mmgrab(hmm->mm);
> + mm->hmm = hmm;
> return hmm;
> -
> -error_mm:
> - spin_lock(&mm->page_table_lock);
> - if (mm->hmm == hmm)
> - mm->hmm = NULL;
> - spin_unlock(&mm->page_table_lock);
> -error:
> - mmdrop(hmm->mm);
> - kfree(hmm);
> - return NULL;
> }
>
> static void hmm_free_rcu(struct rcu_head *rcu)
> {
> - kfree(container_of(rcu, struct hmm, rcu));
> + struct hmm *hmm = container_of(rcu, struct hmm, rcu);
> +
> + down_write(&hmm->mm->mmap_sem);
> + if (hmm->mm->hmm == hmm)
> + hmm->mm->hmm = NULL;
> + up_write(&hmm->mm->mmap_sem);
> + mmdrop(hmm->mm);
> +
> + kfree(hmm);
> }
>
> static void hmm_free(struct kref *kref)
> {
> struct hmm *hmm = container_of(kref, struct hmm, kref);
> - struct mm_struct *mm = hmm->mm;
> -
> - mmu_notifier_unregister_no_release(&hmm->mmu_notifier, mm);
>
> - spin_lock(&mm->page_table_lock);
> - if (mm->hmm == hmm)
> - mm->hmm = NULL;
> - spin_unlock(&mm->page_table_lock);
> -
> - mmdrop(hmm->mm);
> + mmu_notifier_unregister_no_release(&hmm->mmu_notifier, hmm->mm);
> mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
> }
>
>
Yes.
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH v2 hmm 03/11] mm/hmm: Hold a mmgrab from hmm to mm
From: John Hubbard @ 2019-06-07 2:44 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-4-jgg-uk2M96/98Pc@public.gmane.org>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> So long a a struct hmm pointer exists, so should the struct mm it is
> linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it
> once the hmm refcount goes to zero.
>
> Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL
> mm->hmm delete the hmm_hmm_destroy().
>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
> ---
> v2:
> - Fix error unwind paths in hmm_get_or_create (Jerome/Jason)
> ---
> include/linux/hmm.h | 3 ---
> kernel/fork.c | 1 -
> mm/hmm.c | 22 ++++------------------
> 3 files changed, 4 insertions(+), 22 deletions(-)
>
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 2d519797cb134a..4ee3acabe5ed22 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror,
> }
>
> /* Below are for HMM internal use only! Not to be used by device driver! */
> -void hmm_mm_destroy(struct mm_struct *mm);
> -
> static inline void hmm_mm_init(struct mm_struct *mm)
> {
> mm->hmm = NULL;
> }
> #else /* IS_ENABLED(CONFIG_HMM_MIRROR) */
> -static inline void hmm_mm_destroy(struct mm_struct *mm) {}
> static inline void hmm_mm_init(struct mm_struct *mm) {}
> #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index b2b87d450b80b5..588c768ae72451 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm)
> WARN_ON_ONCE(mm == current->active_mm);
> mm_free_pgd(mm);
> destroy_context(mm);
> - hmm_mm_destroy(mm);
This is particularly welcome, not to have an "HMM is special" case
in such a core part of process/mm code.
> mmu_notifier_mm_destroy(mm);
> check_mm(mm);
> put_user_ns(mm->user_ns);
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 8796447299023c..cc7c26fda3300e 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -29,6 +29,7 @@
> #include <linux/swapops.h>
> #include <linux/hugetlb.h>
> #include <linux/memremap.h>
> +#include <linux/sched/mm.h>
> #include <linux/jump_label.h>
> #include <linux/dma-mapping.h>
> #include <linux/mmu_notifier.h>
> @@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> hmm->notifiers = 0;
> hmm->dead = false;
> hmm->mm = mm;
> + mmgrab(hmm->mm);
>
> spin_lock(&mm->page_table_lock);
> if (!mm->hmm)
> @@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
> error:
> + mmdrop(hmm->mm);
> kfree(hmm);
> return NULL;
> }
> @@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
>
> + mmdrop(hmm->mm);
> mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
> }
>
> @@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm)
> kref_put(&hmm->kref, hmm_free);
> }
>
> -void hmm_mm_destroy(struct mm_struct *mm)
> -{
> - struct hmm *hmm;
> -
> - spin_lock(&mm->page_table_lock);
> - hmm = mm_get_hmm(mm);
> - mm->hmm = NULL;
> - if (hmm) {
> - hmm->mm = NULL;
> - hmm->dead = true;
> - spin_unlock(&mm->page_table_lock);
> - hmm_put(hmm);
> - return;
> - }
> -
> - spin_unlock(&mm->page_table_lock);
> -}
> -
> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>
Failed to find any problems with this. :)
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 02/11] mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
From: John Hubbard @ 2019-06-07 2:36 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-3-jgg-uk2M96/98Pc@public.gmane.org>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
>
> Ralph observes that hmm_range_register() can only be called by a driver
> while a mirror is registered. Make this clear in the API by passing in the
> mirror structure as a parameter.
>
> This also simplifies understanding the lifetime model for struct hmm, as
> the hmm pointer must be valid as part of a registered mirror so all we
> need in hmm_register_range() is a simple kref_get.
>
> Suggested-by: Ralph Campbell <rcampbell@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
> ---
> v2
> - Include the oneline patch to nouveau_svm.c
> ---
> drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
> include/linux/hmm.h | 7 ++++---
> mm/hmm.c | 15 ++++++---------
> 3 files changed, 11 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_svm.c b/drivers/gpu/drm/nouveau/nouveau_svm.c
> index 93ed43c413f0bb..8c92374afcf227 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_svm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_svm.c
> @@ -649,7 +649,7 @@ nouveau_svm_fault(struct nvif_notify *notify)
> range.values = nouveau_svm_pfn_values;
> range.pfn_shift = NVIF_VMM_PFNMAP_V0_ADDR_SHIFT;
> again:
> - ret = hmm_vma_fault(&range, true);
> + ret = hmm_vma_fault(&svmm->mirror, &range, true);
> if (ret == 0) {
> mutex_lock(&svmm->mutex);
> if (!hmm_vma_range_done(&range)) {
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 688c5ca7068795..2d519797cb134a 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -505,7 +505,7 @@ static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
> * Please see Documentation/vm/hmm.rst for how to use the range API.
> */
> int hmm_range_register(struct hmm_range *range,
> - struct mm_struct *mm,
> + struct hmm_mirror *mirror,
> unsigned long start,
> unsigned long end,
> unsigned page_shift);
> @@ -541,7 +541,8 @@ static inline bool hmm_vma_range_done(struct hmm_range *range)
> }
>
> /* This is a temporary helper to avoid merge conflict between trees. */
> -static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> +static inline int hmm_vma_fault(struct hmm_mirror *mirror,
> + struct hmm_range *range, bool block)
> {
> long ret;
>
> @@ -554,7 +555,7 @@ static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> range->default_flags = 0;
> range->pfn_flags_mask = -1UL;
>
> - ret = hmm_range_register(range, range->vma->vm_mm,
> + ret = hmm_range_register(range, mirror,
> range->start, range->end,
> PAGE_SHIFT);
> if (ret)
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 547002f56a163d..8796447299023c 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -925,13 +925,13 @@ static void hmm_pfns_clear(struct hmm_range *range,
> * Track updates to the CPU page table see include/linux/hmm.h
> */
> int hmm_range_register(struct hmm_range *range,
> - struct mm_struct *mm,
> + struct hmm_mirror *mirror,
> unsigned long start,
> unsigned long end,
> unsigned page_shift)
> {
> unsigned long mask = ((1UL << page_shift) - 1UL);
> - struct hmm *hmm;
> + struct hmm *hmm = mirror->hmm;
>
> range->valid = false;
> range->hmm = NULL;
> @@ -945,15 +945,12 @@ int hmm_range_register(struct hmm_range *range,
> range->start = start;
> range->end = end;
>
> - hmm = hmm_get_or_create(mm);
> - if (!hmm)
> - return -EFAULT;
> -
> /* Check if hmm_mm_destroy() was call. */
> - if (hmm->mm == NULL || hmm->dead) {
> - hmm_put(hmm);
> + if (hmm->mm == NULL || hmm->dead)
> return -EFAULT;
> - }
> +
> + range->hmm = hmm;
> + kref_get(&hmm->kref);
>
> /* Initialize range to track CPU page table updates. */
> mutex_lock(&hmm->lock);
>
I'm not a qualified Nouveau reviewer, but this looks obviously correct,
so:
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH v2 hmm 01/11] mm/hmm: fix use after free with struct hmm in the mmu notifiers
From: John Hubbard @ 2019-06-07 2:29 UTC (permalink / raw)
To: Jason Gunthorpe, Jerome Glisse, Ralph Campbell,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-2-jgg-uk2M96/98Pc@public.gmane.org>
On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
> From: Jason Gunthorpe <jgg@mellanox.com>
...
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 8e7403f081f44a..547002f56a163d 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
...
> @@ -125,7 +130,7 @@ static void hmm_free(struct kref *kref)
> mm->hmm = NULL;
> spin_unlock(&mm->page_table_lock);
>
> - kfree(hmm);
> + mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
It occurred to me to wonder if it is best to use the MMU notifier's
instance of srcu, instead of creating a separate instance for HMM.
But this really does seem appropriate, since we are after all using
this to synchronize with MMU notifier callbacks. So, fine.
> }
>
> static inline void hmm_put(struct hmm *hmm)
> @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
>
> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> {
> - struct hmm *hmm = mm_get_hmm(mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> struct hmm_range *range;
>
> + /* hmm is in progress to free */
Well, sometimes, yes. :)
Maybe this wording is clearer (if we need any comment at all):
/* Bail out if hmm is in the process of being freed */
> + if (!kref_get_unless_zero(&hmm->kref))
> + return;
> +
> /* Report this HMM as dying. */
> hmm->dead = true;
>
> @@ -194,13 +203,15 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> - struct hmm *hmm = mm_get_hmm(nrange->mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
> struct hmm_mirror *mirror;
> struct hmm_update update;
> struct hmm_range *range;
> int ret = 0;
>
> - VM_BUG_ON(!hmm);
> + /* hmm is in progress to free */
Same here.
> + if (!kref_get_unless_zero(&hmm->kref))
> + return 0;
>
> update.start = nrange->start;
> update.end = nrange->end;
> @@ -245,9 +256,11 @@ static int hmm_invalidate_range_start(struct mmu_notifier *mn,
> static void hmm_invalidate_range_end(struct mmu_notifier *mn,
> const struct mmu_notifier_range *nrange)
> {
> - struct hmm *hmm = mm_get_hmm(nrange->mm);
> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>
> - VM_BUG_ON(!hmm);
> + /* hmm is in progress to free */
And here.
> + if (!kref_get_unless_zero(&hmm->kref))
> + return;
>
> mutex_lock(&hmm->lock);
> hmm->notifiers--;
>
Elegant fix. Regardless of the above chatter I added, you can add:
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
thanks,
--
John Hubbard
NVIDIA
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Dave Chinner @ 2019-06-06 22:28 UTC (permalink / raw)
To: Ira Weiny
Cc: Jan Kara, Dan Williams, Theodore Ts'o, Jeff Layton,
Matthew Wilcox, linux-xfs, Andrew Morton, John Hubbard,
Jérôme Glisse, linux-fsdevel, linux-kernel,
linux-nvdimm, linux-ext4, linux-mm, Jason Gunthorpe, linux-rdma
In-Reply-To: <20190606220329.GA11698@iweiny-DESK2.sc.intel.com>
On Thu, Jun 06, 2019 at 03:03:30PM -0700, Ira Weiny wrote:
> On Thu, Jun 06, 2019 at 12:42:03PM +0200, Jan Kara wrote:
> > On Wed 05-06-19 18:45:33, ira.weiny@intel.com wrote:
> > So I'd like to actually mandate that you *must* hold the file lease until
> > you unpin all pages in the given range (not just that you have an option to
> > hold a lease). And I believe the kernel should actually enforce this. That
> > way we maintain a sane state that if someone uses a physical location of
> > logical file offset on disk, he has a layout lease. Also once this is done,
> > sysadmin has a reasonably easy way to discover run-away RDMA application
> > and kill it if he wishes so.
>
> Fair enough.
>
> I was kind of heading that direction but had not thought this far forward. I
> was exploring how to have a lease remain on the file even after a "lease
> break". But that is incompatible with the current semantics of a "layout"
> lease (as currently defined in the kernel). [In the end I wanted to get an RFC
> out to see what people think of this idea so I did not look at keeping the
> lease.]
>
> Also hitch is that currently a lease is forcefully broken after
> <sysfs>/lease-break-time. To do what you suggest I think we would need a new
> lease type with the semantics you describe.
That just requires a flag when gaining the layout lease to say it is
an "unbreakable layout lease". That gives the kernel the information
needed to determine whether it should attempt to break the lease on
truncate or just return ETXTBSY....
i.e. it allows gup-pinning applications that want to behave nicely
with other users to drop their gup pins and release the lease when
something else wants to truncate/hole punch the file rather than
have truncate return an error. e.g. to allow apps to cleanly interop
with other breakable layout leases (e.g. pNFS) on the same
filesystem.
FWIW, I'd also like to see the "truncate fails when unbreakable
layout lease is held" behaviour to be common across all
filesystem/storage types, not be confined to DAX only. i.e. truncate
should return ETXTBSY when an unbreakable layout lease is held
by an application, not just when "DAX+gup-pinned" is triggered....
Whatever we decide, the behaviour of truncate et al needs to be
predictable, consistent and easily discoverable...
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Ira Weiny @ 2019-06-06 22:26 UTC (permalink / raw)
To: Jan Kara
Cc: Dan Williams, Theodore Ts'o, Jeff Layton, Dave Chinner,
Matthew Wilcox, linux-xfs, Andrew Morton, John Hubbard,
Jérôme Glisse, linux-fsdevel, linux-kernel,
linux-nvdimm, linux-ext4, linux-mm, Jason Gunthorpe, linux-rdma
In-Reply-To: <20190606220329.GA11698@iweiny-DESK2.sc.intel.com>
On Thu, Jun 06, 2019 at 03:03:30PM -0700, 'Ira Weiny' wrote:
> On Thu, Jun 06, 2019 at 12:42:03PM +0200, Jan Kara wrote:
> > On Wed 05-06-19 18:45:33, ira.weiny@intel.com wrote:
> > > From: Ira Weiny <ira.weiny@intel.com>
> > >
> > > ... V1,000,000 ;-)
> > >
> > > Pre-requisites:
> > > John Hubbard's put_user_pages() patch series.[1]
> > > Jan Kara's ext4_break_layouts() fixes[2]
> > >
> > > Based on the feedback from LSFmm and the LWN article which resulted. I've
> > > decided to take a slightly different tack on this problem.
> > >
> > > The real issue is that there is no use case for a user to have RDMA pinn'ed
> > > memory which is then truncated. So really any solution we present which:
> > >
> > > A) Prevents file system corruption or data leaks
> > > ...and...
> > > B) Informs the user that they did something wrong
> > >
> > > Should be an acceptable solution.
> > >
> > > Because this is slightly new behavior. And because this is gonig to be
> > > specific to DAX (because of the lack of a page cache) we have made the user
> > > "opt in" to this behavior.
> > >
> > > The following patches implement the following solution.
> > >
> > > 1) The user has to opt in to allowing GUP pins on a file with a layout lease
> > > (now made visible).
> > > 2) GUP will fail (EPERM) if a layout lease is not taken
> > > 3) Any truncate or hole punch operation on a GUP'ed DAX page will fail.
> > > 4) The user has the option of holding the layout lease to receive a SIGIO for
> > > notification to the original thread that another thread has tried to delete
> > > their data. Furthermore this indicates that if the user needs to GUP the
> > > file again they will need to retake the Layout lease before doing so.
> > >
> > >
> > > NOTE: If the user releases the layout lease or if it has been broken by
> > > another operation further GUP operations on the file will fail without
> > > re-taking the lease. This means that if a user would like to register
> > > pieces of a file and continue to register other pieces later they would
> > > be advised to keep the layout lease, get a SIGIO notification, and retake
> > > the lease.
> > >
> > > NOTE2: Truncation of pages which are not actively pinned will succeed.
> > > Similar to accessing an mmap to this area GUP pins of that memory may
> > > fail.
> >
> > So after some through I'm willing accept the fact that pinned DAX pages
> > will just make truncate / hole punch fail and shove it into a same bucket
> > of situations like "user can open a file and unlink won't delete it" or
> > "ETXTBUSY when user is executing a file being truncated". The problem I
> > have with this proposal is a lack of visibility from sysadmin POV. For
> > ETXTBUSY or "unlinked but open file" sysadmin can just do lsof, find the
> > problematic process and kill it. There's nothing like that with your
> > proposal since currently once you hold page reference, you can unmap the
> > file, drop layout lease, close the file, and there's no trace that you're
> > responsible for the pinned page anymore.
>
> Agreed. For some "GUP interfaces" one may be able to figure this out but I'm
> not familiar with any. For RDMA there has been some additions for tracking
> resources but I don't think any of that is useful here. Regardless from a FS
> POV this is awkward to have to understand all the independent interfaces, so I
> agree.
>
> >
> > So I'd like to actually mandate that you *must* hold the file lease until
> > you unpin all pages in the given range (not just that you have an option to
> > hold a lease). And I believe the kernel should actually enforce this. That
> > way we maintain a sane state that if someone uses a physical location of
> > logical file offset on disk, he has a layout lease. Also once this is done,
> > sysadmin has a reasonably easy way to discover run-away RDMA application
> > and kill it if he wishes so.
>
> Fair enough.
>
> I was kind of heading that direction but had not thought this far forward. I
> was exploring how to have a lease remain on the file even after a "lease
> break". But that is incompatible with the current semantics of a "layout"
> lease (as currently defined in the kernel). [In the end I wanted to get an RFC
> out to see what people think of this idea so I did not look at keeping the
> lease.]
>
> Also hitch is that currently a lease is forcefully broken after
> <sysfs>/lease-break-time. To do what you suggest I think we would need a new
> lease type with the semantics you describe.
>
> Previously I had thought this would be a good idea (for other reasons). But
> what does everyone think about using a "longterm lease" similar to [1] which
> has the semantics you proppose? In [1] I was not sure "longterm" was a good
> name but with your proposal I think it makes more sense.
>
> >
> > The question is on how to exactly enforce that lease is taken until all
> > pages are unpinned. I belive it could be done by tracking number of
> > long-term pinned pages within a lease. Gup_longterm could easily increment
> > the count when verifying the lease exists, gup_longterm users will somehow
> > need to propagate corresponding 'filp' (struct file pointer) to
> > put_user_pages_longterm() callsites so that they can look up appropriate
> > lease to drop reference
>
> I actually think that might be pretty easy. I actually added a ref count to
> the longterm lease before.[2] This was done to be able to take the lease
> within the GUP code. We don't need that functionality exactly but that patch
> implements some of what you propose. With a ref count on the lease we can
> refuse to release it until all GUP users have released it.
>
> >
> > - probably I'd just transition all gup_longterm()
> > users to a saner API similar to the one we have in mm/frame_vector.c where
> > we don't hand out page pointers but an encapsulating structure that does
> > all the necessary tracking.
>
> I'll take a look at that code. But that seems like a pretty big change.
>
> >
> > Removing a lease would need to block until all
> > pins are released - this is probably the most hairy part since we need to
> > handle a case if application just closes the file descriptor which would
> > release the lease but OTOH we need to make sure task exit does not deadlock.
> > Maybe we could block only on explicit lease unlock and just drop the layout
> > lease on file close and if there are still pinned pages, send SIGKILL to an
> > application as a reminder it did something stupid...
>
> As presented at LSFmm I'm not opposed to killing a process which does not
> "follow the rules". But I'm concerned about how to handle this across a fork.
>
> Limiting the open()/LEASE/GUP/close()/SIGKILL to a specific pid "leak"'s pins
> to a child through the RDMA context. This was the major issue Jason had with
> the SIGBUS proposal.
>
> Always sending a SIGKILL would prevent an RDMA process from doing something
> like system("ls") (would kill the child unnecessarily). Are we ok with that?
I might be wrong here. My memory said it closed all fd's but I'm not finding
any documentation of that. Perhaps we could say that the child would be
required to keep the fd open as well?
Ira
>
> >
> > What do people think about this?
>
> But generally I like the idea of the leases being sticky. Not sure about the
> SIGKILL.
>
> Thanks for the review,
> Ira
>
> [1] https://patchwork.kernel.org/patch/10921171/
> [2] https://patchwork.kernel.org/patch/10921177/
>
> >
> > Honza
> > >
> > >
> > > A general overview follows for background.
> > >
> > > It should be noted that one solution for this problem is to use RDMA's On
> > > Demand Paging (ODP). There are 2 big reasons this may not work.
> > >
> > > 1) The hardware being used for RDMA may not support ODP
> > > 2) ODP may be detrimental to the over all network (cluster or cloud)
> > > performance
> > >
> > > Therefore, in order to support RDMA to File system pages without On Demand
> > > Paging (ODP) a number of things need to be done.
> > >
> > > 1) GUP "longterm" users need to inform the other subsystems that they have
> > > taken a pin on a page which may remain pinned for a very "long time".[3]
> > >
> > > 2) Any page which is "controlled" by a file system needs to have special
> > > handling. The details of the handling depends on if the page is page cache
> > > fronted or not.
> > >
> > > 2a) A page cache fronted page which has been pinned by GUP long term can use a
> > > bounce buffer to allow the file system to write back snap shots of the page.
> > > This is handled by the FS recognizing the GUP long term pin and making a copy
> > > of the page to be written back.
> > > NOTE: this patch set does not address this path.
> > >
> > > 2b) A FS "controlled" page which is not page cache fronted is either easier
> > > to deal with or harder depending on the operation the filesystem is trying
> > > to do.
> > >
> > > 2ba) [Hard case] If the FS operation _is_ a truncate or hole punch the
> > > FS can no longer use the pages in question until the pin has been
> > > removed. This patch set presents a solution to this by introducing
> > > some reasonable restrictions on user space applications.
> > >
> > > 2bb) [Easy case] If the FS operation is _not_ a truncate or hole punch
> > > then there is nothing which need be done. Data is Read or Written
> > > directly to the page. This is an easy case which would currently work
> > > if not for GUP long term pins being disabled. Therefore this patch set
> > > need not change access to the file data but does allow for GUP pins
> > > after 2ba above is dealt with.
> > >
> > >
> > > This patch series and presents a solution for problem 2ba)
> > >
> > > [1] https://github.com/johnhubbard/linux/tree/gup_dma_core
> > >
> > > [2] ext4/dev branch:
> > >
> > > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/log/?h=dev
> > >
> > > Specific patches:
> > >
> > > [2a] ext4: wait for outstanding dio during truncate in nojournal mode
> > >
> > > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=82a25b027ca48d7ef197295846b352345853dfa8
> > >
> > > [2b] ext4: do not delete unlinked inode from orphan list on failed truncate
> > >
> > > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=ee0ed02ca93ef1ecf8963ad96638795d55af2c14
> > >
> > > [2c] ext4: gracefully handle ext4_break_layouts() failure during truncate
> > >
> > > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=b9c1c26739ec2d4b4fb70207a0a9ad6747e43f4c
> > >
> > > [3] The definition of long time is debatable but it has been established
> > > that RDMAs use of pages, minutes or hours after the pin is the extreme case
> > > which makes this problem most severe.
> > >
> > >
> > > Ira Weiny (10):
> > > fs/locks: Add trace_leases_conflict
> > > fs/locks: Export F_LAYOUT lease to user space
> > > mm/gup: Pass flags down to __gup_device_huge* calls
> > > mm/gup: Ensure F_LAYOUT lease is held prior to GUP'ing pages
> > > fs/ext4: Teach ext4 to break layout leases
> > > fs/ext4: Teach dax_layout_busy_page() to operate on a sub-range
> > > fs/ext4: Fail truncate if pages are GUP pinned
> > > fs/xfs: Teach xfs to use new dax_layout_busy_page()
> > > fs/xfs: Fail truncate if pages are GUP pinned
> > > mm/gup: Remove FOLL_LONGTERM DAX exclusion
> > >
> > > fs/Kconfig | 1 +
> > > fs/dax.c | 38 ++++++---
> > > fs/ext4/ext4.h | 2 +-
> > > fs/ext4/extents.c | 6 +-
> > > fs/ext4/inode.c | 26 +++++--
> > > fs/locks.c | 97 ++++++++++++++++++++---
> > > fs/xfs/xfs_file.c | 24 ++++--
> > > fs/xfs/xfs_inode.h | 5 +-
> > > fs/xfs/xfs_ioctl.c | 15 +++-
> > > fs/xfs/xfs_iops.c | 14 +++-
> > > fs/xfs/xfs_pnfs.c | 14 ++--
> > > include/linux/dax.h | 9 ++-
> > > include/linux/fs.h | 2 +-
> > > include/linux/mm.h | 2 +
> > > include/trace/events/filelock.h | 35 +++++++++
> > > include/uapi/asm-generic/fcntl.h | 3 +
> > > mm/gup.c | 129 ++++++++++++-------------------
> > > mm/huge_memory.c | 12 +++
> > > 18 files changed, 299 insertions(+), 135 deletions(-)
> > >
> > > --
> > > 2.20.1
> > >
> > --
> > Jan Kara <jack@suse.com>
> > SUSE Labs, CR
>
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Ira Weiny @ 2019-06-06 22:03 UTC (permalink / raw)
To: Jan Kara
Cc: Dan Williams, Theodore Ts'o, Jeff Layton, Dave Chinner,
Matthew Wilcox, linux-xfs, Andrew Morton, John Hubbard,
Jérôme Glisse, linux-fsdevel, linux-kernel,
linux-nvdimm, linux-ext4, linux-mm, Jason Gunthorpe, linux-rdma
In-Reply-To: <20190606104203.GF7433@quack2.suse.cz>
On Thu, Jun 06, 2019 at 12:42:03PM +0200, Jan Kara wrote:
> On Wed 05-06-19 18:45:33, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> >
> > ... V1,000,000 ;-)
> >
> > Pre-requisites:
> > John Hubbard's put_user_pages() patch series.[1]
> > Jan Kara's ext4_break_layouts() fixes[2]
> >
> > Based on the feedback from LSFmm and the LWN article which resulted. I've
> > decided to take a slightly different tack on this problem.
> >
> > The real issue is that there is no use case for a user to have RDMA pinn'ed
> > memory which is then truncated. So really any solution we present which:
> >
> > A) Prevents file system corruption or data leaks
> > ...and...
> > B) Informs the user that they did something wrong
> >
> > Should be an acceptable solution.
> >
> > Because this is slightly new behavior. And because this is gonig to be
> > specific to DAX (because of the lack of a page cache) we have made the user
> > "opt in" to this behavior.
> >
> > The following patches implement the following solution.
> >
> > 1) The user has to opt in to allowing GUP pins on a file with a layout lease
> > (now made visible).
> > 2) GUP will fail (EPERM) if a layout lease is not taken
> > 3) Any truncate or hole punch operation on a GUP'ed DAX page will fail.
> > 4) The user has the option of holding the layout lease to receive a SIGIO for
> > notification to the original thread that another thread has tried to delete
> > their data. Furthermore this indicates that if the user needs to GUP the
> > file again they will need to retake the Layout lease before doing so.
> >
> >
> > NOTE: If the user releases the layout lease or if it has been broken by
> > another operation further GUP operations on the file will fail without
> > re-taking the lease. This means that if a user would like to register
> > pieces of a file and continue to register other pieces later they would
> > be advised to keep the layout lease, get a SIGIO notification, and retake
> > the lease.
> >
> > NOTE2: Truncation of pages which are not actively pinned will succeed.
> > Similar to accessing an mmap to this area GUP pins of that memory may
> > fail.
>
> So after some through I'm willing accept the fact that pinned DAX pages
> will just make truncate / hole punch fail and shove it into a same bucket
> of situations like "user can open a file and unlink won't delete it" or
> "ETXTBUSY when user is executing a file being truncated". The problem I
> have with this proposal is a lack of visibility from sysadmin POV. For
> ETXTBUSY or "unlinked but open file" sysadmin can just do lsof, find the
> problematic process and kill it. There's nothing like that with your
> proposal since currently once you hold page reference, you can unmap the
> file, drop layout lease, close the file, and there's no trace that you're
> responsible for the pinned page anymore.
Agreed. For some "GUP interfaces" one may be able to figure this out but I'm
not familiar with any. For RDMA there has been some additions for tracking
resources but I don't think any of that is useful here. Regardless from a FS
POV this is awkward to have to understand all the independent interfaces, so I
agree.
>
> So I'd like to actually mandate that you *must* hold the file lease until
> you unpin all pages in the given range (not just that you have an option to
> hold a lease). And I believe the kernel should actually enforce this. That
> way we maintain a sane state that if someone uses a physical location of
> logical file offset on disk, he has a layout lease. Also once this is done,
> sysadmin has a reasonably easy way to discover run-away RDMA application
> and kill it if he wishes so.
Fair enough.
I was kind of heading that direction but had not thought this far forward. I
was exploring how to have a lease remain on the file even after a "lease
break". But that is incompatible with the current semantics of a "layout"
lease (as currently defined in the kernel). [In the end I wanted to get an RFC
out to see what people think of this idea so I did not look at keeping the
lease.]
Also hitch is that currently a lease is forcefully broken after
<sysfs>/lease-break-time. To do what you suggest I think we would need a new
lease type with the semantics you describe.
Previously I had thought this would be a good idea (for other reasons). But
what does everyone think about using a "longterm lease" similar to [1] which
has the semantics you proppose? In [1] I was not sure "longterm" was a good
name but with your proposal I think it makes more sense.
>
> The question is on how to exactly enforce that lease is taken until all
> pages are unpinned. I belive it could be done by tracking number of
> long-term pinned pages within a lease. Gup_longterm could easily increment
> the count when verifying the lease exists, gup_longterm users will somehow
> need to propagate corresponding 'filp' (struct file pointer) to
> put_user_pages_longterm() callsites so that they can look up appropriate
> lease to drop reference
I actually think that might be pretty easy. I actually added a ref count to
the longterm lease before.[2] This was done to be able to take the lease
within the GUP code. We don't need that functionality exactly but that patch
implements some of what you propose. With a ref count on the lease we can
refuse to release it until all GUP users have released it.
>
> - probably I'd just transition all gup_longterm()
> users to a saner API similar to the one we have in mm/frame_vector.c where
> we don't hand out page pointers but an encapsulating structure that does
> all the necessary tracking.
I'll take a look at that code. But that seems like a pretty big change.
>
> Removing a lease would need to block until all
> pins are released - this is probably the most hairy part since we need to
> handle a case if application just closes the file descriptor which would
> release the lease but OTOH we need to make sure task exit does not deadlock.
> Maybe we could block only on explicit lease unlock and just drop the layout
> lease on file close and if there are still pinned pages, send SIGKILL to an
> application as a reminder it did something stupid...
As presented at LSFmm I'm not opposed to killing a process which does not
"follow the rules". But I'm concerned about how to handle this across a fork.
Limiting the open()/LEASE/GUP/close()/SIGKILL to a specific pid "leak"'s pins
to a child through the RDMA context. This was the major issue Jason had with
the SIGBUS proposal.
Always sending a SIGKILL would prevent an RDMA process from doing something
like system("ls") (would kill the child unnecessarily). Are we ok with that?
>
> What do people think about this?
But generally I like the idea of the leases being sticky. Not sure about the
SIGKILL.
Thanks for the review,
Ira
[1] https://patchwork.kernel.org/patch/10921171/
[2] https://patchwork.kernel.org/patch/10921177/
>
> Honza
> >
> >
> > A general overview follows for background.
> >
> > It should be noted that one solution for this problem is to use RDMA's On
> > Demand Paging (ODP). There are 2 big reasons this may not work.
> >
> > 1) The hardware being used for RDMA may not support ODP
> > 2) ODP may be detrimental to the over all network (cluster or cloud)
> > performance
> >
> > Therefore, in order to support RDMA to File system pages without On Demand
> > Paging (ODP) a number of things need to be done.
> >
> > 1) GUP "longterm" users need to inform the other subsystems that they have
> > taken a pin on a page which may remain pinned for a very "long time".[3]
> >
> > 2) Any page which is "controlled" by a file system needs to have special
> > handling. The details of the handling depends on if the page is page cache
> > fronted or not.
> >
> > 2a) A page cache fronted page which has been pinned by GUP long term can use a
> > bounce buffer to allow the file system to write back snap shots of the page.
> > This is handled by the FS recognizing the GUP long term pin and making a copy
> > of the page to be written back.
> > NOTE: this patch set does not address this path.
> >
> > 2b) A FS "controlled" page which is not page cache fronted is either easier
> > to deal with or harder depending on the operation the filesystem is trying
> > to do.
> >
> > 2ba) [Hard case] If the FS operation _is_ a truncate or hole punch the
> > FS can no longer use the pages in question until the pin has been
> > removed. This patch set presents a solution to this by introducing
> > some reasonable restrictions on user space applications.
> >
> > 2bb) [Easy case] If the FS operation is _not_ a truncate or hole punch
> > then there is nothing which need be done. Data is Read or Written
> > directly to the page. This is an easy case which would currently work
> > if not for GUP long term pins being disabled. Therefore this patch set
> > need not change access to the file data but does allow for GUP pins
> > after 2ba above is dealt with.
> >
> >
> > This patch series and presents a solution for problem 2ba)
> >
> > [1] https://github.com/johnhubbard/linux/tree/gup_dma_core
> >
> > [2] ext4/dev branch:
> >
> > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/log/?h=dev
> >
> > Specific patches:
> >
> > [2a] ext4: wait for outstanding dio during truncate in nojournal mode
> >
> > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=82a25b027ca48d7ef197295846b352345853dfa8
> >
> > [2b] ext4: do not delete unlinked inode from orphan list on failed truncate
> >
> > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=ee0ed02ca93ef1ecf8963ad96638795d55af2c14
> >
> > [2c] ext4: gracefully handle ext4_break_layouts() failure during truncate
> >
> > - https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git/commit/?h=dev&id=b9c1c26739ec2d4b4fb70207a0a9ad6747e43f4c
> >
> > [3] The definition of long time is debatable but it has been established
> > that RDMAs use of pages, minutes or hours after the pin is the extreme case
> > which makes this problem most severe.
> >
> >
> > Ira Weiny (10):
> > fs/locks: Add trace_leases_conflict
> > fs/locks: Export F_LAYOUT lease to user space
> > mm/gup: Pass flags down to __gup_device_huge* calls
> > mm/gup: Ensure F_LAYOUT lease is held prior to GUP'ing pages
> > fs/ext4: Teach ext4 to break layout leases
> > fs/ext4: Teach dax_layout_busy_page() to operate on a sub-range
> > fs/ext4: Fail truncate if pages are GUP pinned
> > fs/xfs: Teach xfs to use new dax_layout_busy_page()
> > fs/xfs: Fail truncate if pages are GUP pinned
> > mm/gup: Remove FOLL_LONGTERM DAX exclusion
> >
> > fs/Kconfig | 1 +
> > fs/dax.c | 38 ++++++---
> > fs/ext4/ext4.h | 2 +-
> > fs/ext4/extents.c | 6 +-
> > fs/ext4/inode.c | 26 +++++--
> > fs/locks.c | 97 ++++++++++++++++++++---
> > fs/xfs/xfs_file.c | 24 ++++--
> > fs/xfs/xfs_inode.h | 5 +-
> > fs/xfs/xfs_ioctl.c | 15 +++-
> > fs/xfs/xfs_iops.c | 14 +++-
> > fs/xfs/xfs_pnfs.c | 14 ++--
> > include/linux/dax.h | 9 ++-
> > include/linux/fs.h | 2 +-
> > include/linux/mm.h | 2 +
> > include/trace/events/filelock.h | 35 +++++++++
> > include/uapi/asm-generic/fcntl.h | 3 +
> > mm/gup.c | 129 ++++++++++++-------------------
> > mm/huge_memory.c | 12 +++
> > 18 files changed, 299 insertions(+), 135 deletions(-)
> >
> > --
> > 2.20.1
> >
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR
^ permalink raw reply
* [GIT PULL] Please pull RDMA subsystem changes
From: Jason Gunthorpe @ 2019-06-06 20:14 UTC (permalink / raw)
To: Linus Torvalds, Doug Ledford
Cc: linux-rdma@vger.kernel.org, linux-kernel@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2980 bytes --]
Hi Linus,
Things are looking pretty quiet here in RDMA, not too many bug fixes rolling
in right now. Here is the first batch of proposed rc fixes.
Thanks,
Jason
The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:
Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git tags/for-linus
for you to fetch changes up to 4f240dfec6bcc852b124ea7c419fb590949fbd4c:
RDMA/efa: Remove MAYEXEC flag check from mmap flow (2019-05-29 13:13:03 -0300)
----------------------------------------------------------------
5.2 First rc pull request
The usual driver bug fixes and fixes for a couple of regressions introduced in
5.2:
- Fix a race on bootup with RDMA device renaming and srp. SRP also needs to
rename its internal sys files
- Fix a memory leak in hns
- Don't leak resources in efa on certain error unwinds
- Don't panic in certain error unwinds in ib_register_device
- Various small user visible bug fix patches for the hfi and efa drivers
- Fix the 32 bit compilation break
----------------------------------------------------------------
Gal Pressman (2):
RDMA/uverbs: Pass udata on uverbs error unwind
RDMA/efa: Remove MAYEXEC flag check from mmap flow
Jason Gunthorpe (1):
RDMA/core: Clear out the udata before error unwind
Kamal Heib (1):
RDMA/core: Fix panic when port_data isn't initialized
Kamenee Arumugam (1):
IB/hfi1: Validate page aligned for a given virtual address
Leon Romanovsky (2):
RDMA/srp: Rename SRP sysfs name after IB device rename trigger
RDMA/hns: Fix PD memory leak for internal allocation
Michal Kubecek (1):
mlx5: avoid 64-bit division
Mike Marciniszyn (3):
IB/rdmavt: Fix alloc_qpn() WARN_ON()
IB/hfi1: Insure freeze_work work_struct is canceled on shutdown
IB/{qib, hfi1, rdmavt}: Correct ibv_devinfo max_mr value
drivers/infiniband/core/device.c | 49 +++++++++++++++++++--------
drivers/infiniband/core/rdma_core.h | 2 ++
drivers/infiniband/core/uverbs_cmd.c | 30 +++++++++++-----
drivers/infiniband/core/uverbs_std_types_cq.c | 2 +-
drivers/infiniband/core/uverbs_std_types_mr.c | 2 +-
drivers/infiniband/hw/efa/efa_verbs.c | 1 -
drivers/infiniband/hw/hfi1/chip.c | 1 +
drivers/infiniband/hw/hfi1/user_exp_rcv.c | 3 ++
drivers/infiniband/hw/hfi1/verbs.c | 2 --
drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 1 +
drivers/infiniband/hw/mlx5/cmd.c | 9 +++--
drivers/infiniband/hw/mlx5/main.c | 2 +-
drivers/infiniband/hw/qib/qib_verbs.c | 2 --
drivers/infiniband/sw/rdmavt/mr.c | 2 ++
drivers/infiniband/sw/rdmavt/qp.c | 3 +-
drivers/infiniband/ulp/srp/ib_srp.c | 18 +++++++++-
include/rdma/ib_verbs.h | 1 +
17 files changed, 95 insertions(+), 35 deletions(-)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: RFC: Run a dedicated hmm.git for 5.3
From: Stephen Rothwell @ 2019-06-06 19:53 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Mike Marciniszyn, Doug Ledford, linux-mm@kvack.org,
linux-rdma@vger.kernel.org, Daniel Vetter, Dennis Dalessandro,
linux-kernel@vger.kernel.org, dri-devel, Kaike Wan,
Christoph Hellwig, Leon Romanovsky, Jerome Glisse, Moni Shoua,
Artemy Kovalyov, Dave Airlie, Andrew Morton, Linus Torvalds
In-Reply-To: <20190606152543.GE17392@mellanox.com>
[-- Attachment #1.1: Type: text/plain, Size: 2940 bytes --]
Hi Jason,
On Thu, 6 Jun 2019 15:25:49 +0000 Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Mon, May 27, 2019 at 04:12:47PM -0300, Jason Gunthorpe wrote:
> > On Sat, May 25, 2019 at 03:52:10PM -0700, Andrew Morton wrote:
> > > On Fri, 24 May 2019 09:44:55 -0300 Jason Gunthorpe <jgg@ziepe.ca> wrote:
> > >
> > > > Now that -mm merged the basic hmm API skeleton I think running like
> > > > this would get us quickly to the place we all want: comprehensive in tree
> > > > users of hmm.
> > > >
> > > > Andrew, would this be acceptable to you?
> > >
> > > Sure. Please take care not to permit this to reduce the amount of
> > > exposure and review which the core HMM pieces get.
> >
> > Certainly, thanks all
> >
> > Jerome: I started a HMM branch on v5.2-rc2 in the rdma.git here:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git
> > https://git.kernel.org/pub/scm/linux/kernel/git/rdma/rdma.git/log/?h=hmm
>
> I did a first round of collecting patches for hmm.git
>
> Andrew, I'm checking linux-next and to stay co-ordinated, I see the
> patches below are in your tree and now also in hmm.git. Can you please
> drop them from your tree?
>
> 5b693741de2ace mm/hmm.c: suppress compilation warnings when CONFIG_HUGETLB_PAGE is not set
> b2870fb882599a mm/hmm.c: only set FAULT_FLAG_ALLOW_RETRY for non-blocking
> dff7babf8ae9f1 mm/hmm.c: support automatic NUMA balancing
>
> I checked that the other two patches in -next also touching hmm.c are
> best suited to go through your tree:
>
> a76b9b318a7180 mm/devm_memremap_pages: fix final page put race
> fc64c058d01b98 mm/memremap: rename and consolidate SECTION_SIZE
>
> StephenR: Can you pick up the hmm branch from rdma.git for linux-next for
> this cycle? As above we are moving the patches from -mm to hmm.git, so
> there will be a conflict in -next until Andrew adjusts his tree,
> thanks!
I have added the hmm branch from today with currently just you as the
contact. I also removed the three commits above from Andrew's tree.
Thanks for adding your subsystem tree as a participant of linux-next. As
you may know, this is not a judgement of your code. The purpose of
linux-next is for integration testing and to lower the impact of
conflicts between subsystems in the next merge window.
You will need to ensure that the patches/commits in your tree/series have
been:
* submitted under GPL v2 (or later) and include the Contributor's
Signed-off-by,
* posted to the relevant mailing list,
* reviewed by you (or another maintainer of your subsystem tree),
* successfully unit tested, and
* destined for the current or next Linux merge window.
Basically, this should be just what you would send to Linus (or ask him
to fetch). It is allowed to be rebased if you deem it necessary.
--
Cheers,
Stephen Rothwell
sfr@canb.auug.org.au
[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Jason Gunthorpe @ 2019-06-06 19:46 UTC (permalink / raw)
To: Ira Weiny
Cc: John Hubbard, Dan Williams, Jan Kara, Theodore Ts'o,
Jeff Layton, Dave Chinner, Matthew Wilcox, linux-xfs,
Andrew Morton, Jérôme Glisse, linux-fsdevel,
linux-kernel, linux-nvdimm, linux-ext4, linux-mm, linux-rdma
In-Reply-To: <20190606171158.GB11374@iweiny-DESK2.sc.intel.com>
On Thu, Jun 06, 2019 at 10:11:58AM -0700, Ira Weiny wrote:
> 2) This is a bit more subtle and something I almost delayed sending these out
> for. Currently the implementation of a lease break actually removes the
> lease from the file. I did not want this to happen and I was thinking of
> delaying this patch set to implement something which keeps the lease around
> but I figured I should get something out for comments. Jan has proposed
> something along these lines and I agree with him so I'm going to ask you to
> read my response to him about the details.
>
>
> Anyway so the key here is that currently an app needs the SIGIO to retake
> the lease if they want to map the file again or in parts based on usage.
> For example, they may only want to map some of the file for when they are
> using it and then map another part later. Without the SIGIO they would lose
> their lease or would have to just take the lease for each GUP pin (which
> adds overhead). Like I said I did not like this but I left it to get
> something which works out.
So to be clear..
Even though the lease is broken the GUP remains, the pages remain
pined, and truncate/etc continues to fail?
I like Jan's take on this actually.. see other email.
Jason
^ permalink raw reply
* [PATCH v2 hmm 11/11] mm/hmm: Remove confusing comment and logic from hmm_release
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
hmm_release() is called exactly once per hmm. ops->release() cannot
accidentally trigger any action that would recurse back onto
hmm->mirrors_sem.
This fixes a use after-free race of the form:
CPU0 CPU1
hmm_release()
up_write(&hmm->mirrors_sem);
hmm_mirror_unregister(mirror)
down_write(&hmm->mirrors_sem);
up_write(&hmm->mirrors_sem);
kfree(mirror)
mirror->ops->release(mirror)
The only user we have today for ops->release is an empty function, so this
is unambiguously safe.
As a consequence of plugging this race drivers are not allowed to
register/unregister mirrors from within a release op.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
mm/hmm.c | 28 +++++++++-------------------
1 file changed, 9 insertions(+), 19 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index 709d138dd49027..3a45dd3d778248 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -136,26 +136,16 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
WARN_ON(!list_empty(&hmm->ranges));
mutex_unlock(&hmm->lock);
- down_write(&hmm->mirrors_sem);
- mirror = list_first_entry_or_null(&hmm->mirrors, struct hmm_mirror,
- list);
- while (mirror) {
- list_del_init(&mirror->list);
- if (mirror->ops->release) {
- /*
- * Drop mirrors_sem so the release callback can wait
- * on any pending work that might itself trigger a
- * mmu_notifier callback and thus would deadlock with
- * us.
- */
- up_write(&hmm->mirrors_sem);
+ down_read(&hmm->mirrors_sem);
+ list_for_each_entry(mirror, &hmm->mirrors, list) {
+ /*
+ * Note: The driver is not allowed to trigger
+ * hmm_mirror_unregister() from this thread.
+ */
+ if (mirror->ops->release)
mirror->ops->release(mirror);
- down_write(&hmm->mirrors_sem);
- }
- mirror = list_first_entry_or_null(&hmm->mirrors,
- struct hmm_mirror, list);
}
- up_write(&hmm->mirrors_sem);
+ up_read(&hmm->mirrors_sem);
hmm_put(hmm);
}
@@ -287,7 +277,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror)
struct hmm *hmm = mirror->hmm;
down_write(&hmm->mirrors_sem);
- list_del_init(&mirror->list);
+ list_del(&mirror->list);
up_write(&hmm->mirrors_sem);
hmm_put(hmm);
memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 10/11] mm/hmm: Do not use list*_rcu() for hmm->ranges
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
This list is always read and written while holding hmm->lock so there is
no need for the confusing _rcu annotations.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
---
mm/hmm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index c2fecb3ecb11e1..709d138dd49027 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -911,7 +911,7 @@ int hmm_range_register(struct hmm_range *range,
mutex_lock(&hmm->lock);
range->hmm = hmm;
- list_add_rcu(&range->list, &hmm->ranges);
+ list_add(&range->list, &hmm->ranges);
/*
* If there are any concurrent notifiers we have to wait for them for
@@ -941,7 +941,7 @@ void hmm_range_unregister(struct hmm_range *range)
return;
mutex_lock(&hmm->lock);
- list_del_rcu(&range->list);
+ list_del(&range->list);
mutex_unlock(&hmm->lock);
/* Drop reference taken by hmm_range_register() */
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 09/11] mm/hmm: Poison hmm_range during unregister
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Trying to misuse a range outside its lifetime is a kernel bug. Use WARN_ON
and poison bytes to detect this condition.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
---
v2
- Keep range start/end valid after unregistration (Jerome)
---
mm/hmm.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index 6802de7080d172..c2fecb3ecb11e1 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -937,7 +937,7 @@ void hmm_range_unregister(struct hmm_range *range)
struct hmm *hmm = range->hmm;
/* Sanity check this really should not happen. */
- if (hmm == NULL || range->end <= range->start)
+ if (WARN_ON(range->end <= range->start))
return;
mutex_lock(&hmm->lock);
@@ -948,7 +948,10 @@ void hmm_range_unregister(struct hmm_range *range)
range->valid = false;
mmput(hmm->mm);
hmm_put(hmm);
- range->hmm = NULL;
+
+ /* The range is now invalid, leave it poisoned. */
+ range->valid = false;
+ memset(&range->hmm, POISON_INUSE, sizeof(range->hmm));
}
EXPORT_SYMBOL(hmm_range_unregister);
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 08/11] mm/hmm: Remove racy protection against double-unregistration
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
No other register/unregister kernel API attempts to provide this kind of
protection as it is inherently racy, so just drop it.
Callers should provide their own protection, it appears nouveau already
does, but just in case drop a debugging POISON.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
---
mm/hmm.c | 9 ++-------
1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index c702cd72651b53..6802de7080d172 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -284,18 +284,13 @@ EXPORT_SYMBOL(hmm_mirror_register);
*/
void hmm_mirror_unregister(struct hmm_mirror *mirror)
{
- struct hmm *hmm = READ_ONCE(mirror->hmm);
-
- if (hmm == NULL)
- return;
+ struct hmm *hmm = mirror->hmm;
down_write(&hmm->mirrors_sem);
list_del_init(&mirror->list);
- /* To protect us against double unregister ... */
- mirror->hmm = NULL;
up_write(&hmm->mirrors_sem);
-
hmm_put(hmm);
+ memset(&mirror->hmm, POISON_INUSE, sizeof(mirror->hmm));
}
EXPORT_SYMBOL(hmm_mirror_unregister);
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 07/11] mm/hmm: Use lockdep instead of comments
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
So we can check locking at runtime.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
---
v2
- Fix missing & in lockdeps (Jason)
---
mm/hmm.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index f67ba32983d9f1..c702cd72651b53 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -254,11 +254,11 @@ static const struct mmu_notifier_ops hmm_mmu_notifier_ops = {
*
* To start mirroring a process address space, the device driver must register
* an HMM mirror struct.
- *
- * THE mm->mmap_sem MUST BE HELD IN WRITE MODE !
*/
int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm)
{
+ lockdep_assert_held_exclusive(&mm->mmap_sem);
+
/* Sanity check */
if (!mm || !mirror || !mirror->ops)
return -EINVAL;
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 06/11] mm/hmm: Hold on to the mmget for the lifetime of the range
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
Range functions like hmm_range_snapshot() and hmm_range_fault() call
find_vma, which requires hodling the mmget() and the mmap_sem for the mm.
Make this simpler for the callers by holding the mmget() inside the range
for the lifetime of the range. Other functions that accept a range should
only be called if the range is registered.
This has the side effect of directly preventing hmm_release() from
happening while a range is registered. That means range->dead cannot be
false during the lifetime of the range, so remove dead and
hmm_mirror_mm_is_alive() entirely.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
v2:
- Use Jerome's idea of just holding the mmget() for the range lifetime,
rework the patch to use that as as simplification to remove dead in
one step
---
include/linux/hmm.h | 26 --------------------------
mm/hmm.c | 28 ++++++++++------------------
2 files changed, 10 insertions(+), 44 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 2ab35b40992b24..0e20566802967a 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -91,7 +91,6 @@
* @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;
@@ -104,7 +103,6 @@ struct hmm {
wait_queue_head_t wq;
struct rcu_head rcu;
long notifiers;
- bool dead;
};
/*
@@ -469,30 +467,6 @@ struct hmm_mirror {
int hmm_mirror_register(struct hmm_mirror *mirror, struct mm_struct *mm);
void hmm_mirror_unregister(struct hmm_mirror *mirror);
-/*
- * hmm_mirror_mm_is_alive() - test if mm is still alive
- * @mirror: the HMM mm mirror for which we want to lock the mmap_sem
- * Return: false if the mm is dead, true otherwise
- *
- * This is an optimization, it will not always accurately return false if the
- * mm is dead; i.e., there can be false negatives (process is being killed but
- * HMM is not yet informed of that). It is only intended to be used to optimize
- * out cases where the driver is about to do something time consuming and it
- * would be better to skip it if the mm is dead.
- */
-static inline bool hmm_mirror_mm_is_alive(struct hmm_mirror *mirror)
-{
- struct mm_struct *mm;
-
- if (!mirror || !mirror->hmm)
- return false;
- mm = READ_ONCE(mirror->hmm->mm);
- if (mirror->hmm->dead || !mm)
- return false;
-
- return true;
-}
-
/*
* Please see Documentation/vm/hmm.rst for how to use the range API.
*/
diff --git a/mm/hmm.c b/mm/hmm.c
index dc30edad9a8a02..f67ba32983d9f1 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -80,7 +80,6 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
mutex_init(&hmm->lock);
kref_init(&hmm->kref);
hmm->notifiers = 0;
- hmm->dead = false;
hmm->mm = mm;
hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
@@ -124,20 +123,17 @@ static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
struct hmm_mirror *mirror;
- struct hmm_range *range;
/* hmm is in progress to free */
if (!kref_get_unless_zero(&hmm->kref))
return;
- /* Report this HMM as dying. */
- hmm->dead = true;
-
- /* Wake-up everyone waiting on any range. */
mutex_lock(&hmm->lock);
- list_for_each_entry(range, &hmm->ranges, list)
- range->valid = false;
- wake_up_all(&hmm->wq);
+ /*
+ * Since hmm_range_register() holds the mmget() lock hmm_release() is
+ * prevented as long as a range exists.
+ */
+ WARN_ON(!list_empty(&hmm->ranges));
mutex_unlock(&hmm->lock);
down_write(&hmm->mirrors_sem);
@@ -909,8 +905,8 @@ int hmm_range_register(struct hmm_range *range,
range->start = start;
range->end = end;
- /* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead)
+ /* Prevent hmm_release() from running while the range is valid */
+ if (!mmget_not_zero(hmm->mm))
return -EFAULT;
range->hmm = hmm;
@@ -955,6 +951,7 @@ void hmm_range_unregister(struct hmm_range *range)
/* Drop reference taken by hmm_range_register() */
range->valid = false;
+ mmput(hmm->mm);
hmm_put(hmm);
range->hmm = NULL;
}
@@ -982,10 +979,7 @@ long hmm_range_snapshot(struct hmm_range *range)
struct vm_area_struct *vma;
struct mm_walk mm_walk;
- /* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead)
- return -EFAULT;
-
+ lockdep_assert_held(&hmm->mm->mmap_sem);
do {
/* If range is no longer valid force retry. */
if (!range->valid)
@@ -1080,9 +1074,7 @@ long hmm_range_fault(struct hmm_range *range, bool block)
struct mm_walk mm_walk;
int ret;
- /* Check if hmm_mm_destroy() was call. */
- if (hmm->mm == NULL || hmm->dead)
- return -EFAULT;
+ lockdep_assert_held(&hmm->mm->mmap_sem);
do {
/* If range is no longer valid force retry. */
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 05/11] mm/hmm: Remove duplicate condition test before wait_event_timeout
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
The wait_event_timeout macro already tests the condition as its first
action, so there is no reason to open code another version of this, all
that does is skip the might_sleep() debugging in common cases, which is
not helpful.
Further, based on prior patches, we can no simplify the required condition
test:
- If range is valid memory then so is range->hmm
- If hmm_release() has run then range->valid is set to false
at the same time as dead, so no reason to check both.
- A valid hmm has a valid hmm->mm.
Also, add the READ_ONCE for range->valid as there is no lock held here.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
---
include/linux/hmm.h | 12 ++----------
1 file changed, 2 insertions(+), 10 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 4ee3acabe5ed22..2ab35b40992b24 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -218,17 +218,9 @@ static inline unsigned long hmm_range_page_size(const struct hmm_range *range)
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,
+ wait_event_timeout(range->hmm->wq, range->valid,
msecs_to_jiffies(timeout));
- /* Return current valid status just in case we get lucky */
- return range->valid;
+ return READ_ONCE(range->valid);
}
/*
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 04/11] mm/hmm: Simplify hmm_get_or_create and make it reliable
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
As coded this function can false-fail in various racy situations. Make it
reliable by running only under the write side of the mmap_sem and avoiding
the false-failing compare/exchange pattern.
Also make the locking very easy to understand by only ever reading or
writing mm->hmm while holding the write side of the mmap_sem.
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
---
v2:
- Fix error unwind of mmgrab (Jerome)
- Use hmm local instead of 2nd container_of (Jerome)
---
mm/hmm.c | 80 ++++++++++++++++++++------------------------------------
1 file changed, 29 insertions(+), 51 deletions(-)
diff --git a/mm/hmm.c b/mm/hmm.c
index cc7c26fda3300e..dc30edad9a8a02 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -40,16 +40,6 @@
#if IS_ENABLED(CONFIG_HMM_MIRROR)
static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
-static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
-{
- struct hmm *hmm = READ_ONCE(mm->hmm);
-
- if (hmm && kref_get_unless_zero(&hmm->kref))
- return hmm;
-
- return NULL;
-}
-
/**
* hmm_get_or_create - register HMM against an mm (HMM internal)
*
@@ -64,11 +54,20 @@ static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
*/
static struct hmm *hmm_get_or_create(struct mm_struct *mm)
{
- struct hmm *hmm = mm_get_hmm(mm);
- bool cleanup = false;
+ struct hmm *hmm;
- if (hmm)
- return hmm;
+ lockdep_assert_held_exclusive(&mm->mmap_sem);
+
+ if (mm->hmm) {
+ if (kref_get_unless_zero(&mm->hmm->kref))
+ return mm->hmm;
+ /*
+ * The hmm is being freed by some other CPU and is pending a
+ * RCU grace period, but this CPU can NULL now it since we
+ * have the mmap_sem.
+ */
+ mm->hmm = NULL;
+ }
hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
if (!hmm)
@@ -83,57 +82,36 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
hmm->notifiers = 0;
hmm->dead = false;
hmm->mm = mm;
- mmgrab(hmm->mm);
-
- spin_lock(&mm->page_table_lock);
- if (!mm->hmm)
- mm->hmm = hmm;
- else
- cleanup = true;
- spin_unlock(&mm->page_table_lock);
- if (cleanup)
- goto error;
-
- /*
- * We should only get here if hold the mmap_sem in write mode ie on
- * registration of first mirror through hmm_mirror_register()
- */
hmm->mmu_notifier.ops = &hmm_mmu_notifier_ops;
- if (__mmu_notifier_register(&hmm->mmu_notifier, mm))
- goto error_mm;
+ if (__mmu_notifier_register(&hmm->mmu_notifier, mm)) {
+ kfree(hmm);
+ return NULL;
+ }
+ mmgrab(hmm->mm);
+ mm->hmm = hmm;
return hmm;
-
-error_mm:
- spin_lock(&mm->page_table_lock);
- if (mm->hmm == hmm)
- mm->hmm = NULL;
- spin_unlock(&mm->page_table_lock);
-error:
- mmdrop(hmm->mm);
- kfree(hmm);
- return NULL;
}
static void hmm_free_rcu(struct rcu_head *rcu)
{
- kfree(container_of(rcu, struct hmm, rcu));
+ struct hmm *hmm = container_of(rcu, struct hmm, rcu);
+
+ down_write(&hmm->mm->mmap_sem);
+ if (hmm->mm->hmm == hmm)
+ hmm->mm->hmm = NULL;
+ up_write(&hmm->mm->mmap_sem);
+ mmdrop(hmm->mm);
+
+ kfree(hmm);
}
static void hmm_free(struct kref *kref)
{
struct hmm *hmm = container_of(kref, struct hmm, kref);
- struct mm_struct *mm = hmm->mm;
-
- mmu_notifier_unregister_no_release(&hmm->mmu_notifier, mm);
- spin_lock(&mm->page_table_lock);
- if (mm->hmm == hmm)
- mm->hmm = NULL;
- spin_unlock(&mm->page_table_lock);
-
- mmdrop(hmm->mm);
+ mmu_notifier_unregister_no_release(&hmm->mmu_notifier, hmm->mm);
mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
}
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related
* [PATCH v2 hmm 03/11] mm/hmm: Hold a mmgrab from hmm to mm
From: Jason Gunthorpe @ 2019-06-06 18:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard,
Felix.Kuehling-5C7GfCeVMHo
Cc: Andrea Arcangeli, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Jason Gunthorpe,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW
In-Reply-To: <20190606184438.31646-1-jgg-uk2M96/98Pc@public.gmane.org>
From: Jason Gunthorpe <jgg@mellanox.com>
So long a a struct hmm pointer exists, so should the struct mm it is
linked too. Hold the mmgrab() as soon as a hmm is created, and mmdrop() it
once the hmm refcount goes to zero.
Since mmdrop() (ie a 0 kref on struct mm) is now impossible with a !NULL
mm->hmm delete the hmm_hmm_destroy().
Signed-off-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Jérôme Glisse <jglisse@redhat.com>
---
v2:
- Fix error unwind paths in hmm_get_or_create (Jerome/Jason)
---
include/linux/hmm.h | 3 ---
kernel/fork.c | 1 -
mm/hmm.c | 22 ++++------------------
3 files changed, 4 insertions(+), 22 deletions(-)
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 2d519797cb134a..4ee3acabe5ed22 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -586,14 +586,11 @@ static inline int hmm_vma_fault(struct hmm_mirror *mirror,
}
/* Below are for HMM internal use only! Not to be used by device driver! */
-void hmm_mm_destroy(struct mm_struct *mm);
-
static inline void hmm_mm_init(struct mm_struct *mm)
{
mm->hmm = NULL;
}
#else /* IS_ENABLED(CONFIG_HMM_MIRROR) */
-static inline void hmm_mm_destroy(struct mm_struct *mm) {}
static inline void hmm_mm_init(struct mm_struct *mm) {}
#endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
diff --git a/kernel/fork.c b/kernel/fork.c
index b2b87d450b80b5..588c768ae72451 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -673,7 +673,6 @@ void __mmdrop(struct mm_struct *mm)
WARN_ON_ONCE(mm == current->active_mm);
mm_free_pgd(mm);
destroy_context(mm);
- hmm_mm_destroy(mm);
mmu_notifier_mm_destroy(mm);
check_mm(mm);
put_user_ns(mm->user_ns);
diff --git a/mm/hmm.c b/mm/hmm.c
index 8796447299023c..cc7c26fda3300e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -29,6 +29,7 @@
#include <linux/swapops.h>
#include <linux/hugetlb.h>
#include <linux/memremap.h>
+#include <linux/sched/mm.h>
#include <linux/jump_label.h>
#include <linux/dma-mapping.h>
#include <linux/mmu_notifier.h>
@@ -82,6 +83,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
hmm->notifiers = 0;
hmm->dead = false;
hmm->mm = mm;
+ mmgrab(hmm->mm);
spin_lock(&mm->page_table_lock);
if (!mm->hmm)
@@ -109,6 +111,7 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
mm->hmm = NULL;
spin_unlock(&mm->page_table_lock);
error:
+ mmdrop(hmm->mm);
kfree(hmm);
return NULL;
}
@@ -130,6 +133,7 @@ static void hmm_free(struct kref *kref)
mm->hmm = NULL;
spin_unlock(&mm->page_table_lock);
+ mmdrop(hmm->mm);
mmu_notifier_call_srcu(&hmm->rcu, hmm_free_rcu);
}
@@ -138,24 +142,6 @@ static inline void hmm_put(struct hmm *hmm)
kref_put(&hmm->kref, hmm_free);
}
-void hmm_mm_destroy(struct mm_struct *mm)
-{
- struct hmm *hmm;
-
- spin_lock(&mm->page_table_lock);
- hmm = mm_get_hmm(mm);
- mm->hmm = NULL;
- if (hmm) {
- hmm->mm = NULL;
- hmm->dead = true;
- spin_unlock(&mm->page_table_lock);
- hmm_put(hmm);
- return;
- }
-
- spin_unlock(&mm->page_table_lock);
-}
-
static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
{
struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
--
2.21.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ 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