Re: [PATCH 03/15] mm/hmm: HMM should have a callback before MM is destroyed v3

[John Hubbard]

On 03/21/2018 06:28 PM, jglisse@xxxxxxxxxx wrote:
> From: Ralph Campbell <rcampbell@xxxxxxxxxx>
> 
> The hmm_mirror_register() function registers a callback for when
> the CPU pagetable is modified. Normally, the device driver will
> call hmm_mirror_unregister() when the process using the device is
> finished. However, if the process exits uncleanly, the struct_mm
> can be destroyed with no warning to the device driver.
> 
> Changed since v1:
>   - dropped VM_BUG_ON()
>   - cc stable
> Changed since v2:
>   - drop stable
>   - Split list removale and call to driver release callback. This
>     allow the release callback to wait on any pending fault handler
>     without deadlock.
> 
> Signed-off-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
> Signed-off-by: JÃrÃme Glisse <jglisse@xxxxxxxxxx>
> Cc: Evgeny Baskakov <ebaskakov@xxxxxxxxxx>
> Cc: Mark Hairgrove <mhairgrove@xxxxxxxxxx>
> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> ---
>  include/linux/hmm.h | 10 ++++++++++
>  mm/hmm.c            | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 36dd21fe5caf..fa7b51f65905 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -218,6 +218,16 @@ enum hmm_update_type {
>   * @update: callback to update range on a device
>   */
>  struct hmm_mirror_ops {
> +	/* release() - release hmm_mirror
> +	 *
> +	 * @mirror: pointer to struct hmm_mirror
> +	 *
> +	 * This is called when the mm_struct is being released.
> +	 * The callback should make sure no references to the mirror occur
> +	 * after the callback returns.
> +	 */
> +	void (*release)(struct hmm_mirror *mirror);
> +
>  	/* sync_cpu_device_pagetables() - synchronize page tables
>  	 *
>  	 * @mirror: pointer to struct hmm_mirror
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 320545b98ff5..34c16297f65e 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -160,6 +160,32 @@ static void hmm_invalidate_range(struct hmm *hmm,
>  	up_read(&hmm->mirrors_sem);
>  }
>  
> +static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
> +{
> +	struct hmm_mirror *mirror;
> +	struct hmm *hmm = mm->hmm;
> +
> +	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 callback can wait on any pending
> +			 * work that might itself trigger mmu_notifier callback
> +			 * and thus would deadlock with us.
> +			 */
> +			up_write(&hmm->mirrors_sem);
> +			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);
> +}
> +

Hi Jerome,

This looks good (and the list handling is way better than my horrible 
copy-the-list idea)!

Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx>

thanks,
-- 
John Hubbard
NVIDIA

>  static void hmm_invalidate_range_start(struct mmu_notifier *mn,
>  				       struct mm_struct *mm,
>  				       unsigned long start,
> @@ -185,6 +211,7 @@ static void hmm_invalidate_range_end(struct mmu_notifier *mn,
>  }
>  
>  static const struct mmu_notifier_ops hmm_mmu_notifier_ops = {
> +	.release		= hmm_release,
>  	.invalidate_range_start	= hmm_invalidate_range_start,
>  	.invalidate_range_end	= hmm_invalidate_range_end,
>  };
> @@ -230,7 +257,7 @@ void hmm_mirror_unregister(struct hmm_mirror *mirror)
>  	struct hmm *hmm = mirror->hmm;
>  
>  	down_write(&hmm->mirrors_sem);
> -	list_del(&mirror->list);
> +	list_del_init(&mirror->list);
>  	up_write(&hmm->mirrors_sem);
>  }
>  EXPORT_SYMBOL(hmm_mirror_unregister);
>