Re: [PATCH V3 rdma-next 3/4] RDMA/uverbs: Hoist the common process of disassociate_ucontext into ib core

[Leon Romanovsky <leonro@mellanox.com>]

[-- Attachment #1: Type: text/plain, Size: 7050 bytes --]

On Sat, May 26, 2018 at 04:41:46PM +0800, Wei Hu (Xavier) wrote:
> This patch hoisted the common process of disassociate_ucontext
> callback function into ib core code, and these code are common
> to ervery ib_device driver.
>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
> ---
>  drivers/infiniband/core/uverbs_main.c | 45 ++++++++++++++++++++++++++++++++++-
>  drivers/infiniband/hw/mlx4/main.c     | 34 --------------------------
>  drivers/infiniband/hw/mlx5/main.c     | 34 --------------------------
>  3 files changed, 44 insertions(+), 69 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 4445d8e..a0a1c70 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -41,6 +41,8 @@
>  #include <linux/fs.h>
>  #include <linux/poll.h>
>  #include <linux/sched.h>
> +#include <linux/sched/mm.h>
> +#include <linux/sched/task.h>
>  #include <linux/file.h>
>  #include <linux/cdev.h>
>  #include <linux/anon_inodes.h>
> @@ -1090,6 +1092,47 @@ static void ib_uverbs_add_one(struct ib_device *device)
>  	return;
>  }
>
> +static void ib_uverbs_disassociate_ucontext(struct ib_ucontext *ibcontext)
> +{
> +	struct ib_device *ib_dev = ibcontext->device;
> +	struct task_struct *owning_process  = NULL;
> +	struct mm_struct   *owning_mm       = NULL;
> +
> +	owning_process = get_pid_task(ibcontext->tgid, PIDTYPE_PID);
> +	if (!owning_process)
> +		return;
> +
> +	owning_mm = get_task_mm(owning_process);
> +	if (!owning_mm) {
> +		pr_info("no mm, disassociate ucontext is pending task termination\n");
> +		while (1) {
> +			put_task_struct(owning_process);
> +			usleep_range(1000, 2000);
> +			owning_process = get_pid_task(ibcontext->tgid,
> +						      PIDTYPE_PID);
> +			if (!owning_process ||
> +			    owning_process->state == TASK_DEAD) {
> +				pr_info("disassociate ucontext done, task was terminated\n");
> +				/* in case task was dead need to release the
> +				 * task struct.
> +				 */
> +				if (owning_process)
> +					put_task_struct(owning_process);
> +				return;
> +			}
> +		}
> +	}
> +
> +	/* need to protect from a race on closing the vma as part of
> +	 * mlx5_ib_vma_close.

Except this "mlx5_ib_vma_close"

Acked-by: Leon Romanovsky <leonro@mellanox.com>

> +	 */
> +	down_write(&owning_mm->mmap_sem);
> +	ib_dev->disassociate_ucontext(ibcontext);
> +	up_write(&owning_mm->mmap_sem);
> +	mmput(owning_mm);
> +	put_task_struct(owning_process);
> +}
> +
>  static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev,
>  					struct ib_device *ib_dev)
>  {
> @@ -1130,7 +1173,7 @@ static void ib_uverbs_free_hw_resources(struct ib_uverbs_device *uverbs_dev,
>  			 * (e.g mmput).
>  			 */
>  			ib_uverbs_event_handler(&file->event_handler, &event);
> -			ib_dev->disassociate_ucontext(ucontext);
> +			ib_uverbs_disassociate_ucontext(ucontext);
>  			mutex_lock(&file->cleanup_mutex);
>  			ib_uverbs_cleanup_ucontext(file, ucontext, true);
>  			mutex_unlock(&file->cleanup_mutex);
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index bf12394..59aed45 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -1189,40 +1189,10 @@ static void mlx4_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
>  	int ret = 0;
>  	struct vm_area_struct *vma;
>  	struct mlx4_ib_ucontext *context = to_mucontext(ibcontext);
> -	struct task_struct *owning_process  = NULL;
> -	struct mm_struct   *owning_mm       = NULL;
> -
> -	owning_process = get_pid_task(ibcontext->tgid, PIDTYPE_PID);
> -	if (!owning_process)
> -		return;
> -
> -	owning_mm = get_task_mm(owning_process);
> -	if (!owning_mm) {
> -		pr_info("no mm, disassociate ucontext is pending task termination\n");
> -		while (1) {
> -			/* make sure that task is dead before returning, it may
> -			 * prevent a rare case of module down in parallel to a
> -			 * call to mlx4_ib_vma_close.
> -			 */
> -			put_task_struct(owning_process);
> -			usleep_range(1000, 2000);
> -			owning_process = get_pid_task(ibcontext->tgid,
> -						      PIDTYPE_PID);
> -			if (!owning_process ||
> -			    owning_process->state == TASK_DEAD) {
> -				pr_info("disassociate ucontext done, task was terminated\n");
> -				/* in case task was dead need to release the task struct */
> -				if (owning_process)
> -					put_task_struct(owning_process);
> -				return;
> -			}
> -		}
> -	}
>
>  	/* need to protect from a race on closing the vma as part of
>  	 * mlx4_ib_vma_close().
>  	 */
> -	down_write(&owning_mm->mmap_sem);
>  	for (i = 0; i < HW_BAR_COUNT; i++) {
>  		vma = context->hw_bar_info[i].vma;
>  		if (!vma)
> @@ -1241,10 +1211,6 @@ static void mlx4_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
>  		/* context going to be destroyed, should not access ops any more */
>  		context->hw_bar_info[i].vma->vm_ops = NULL;
>  	}
> -
> -	up_write(&owning_mm->mmap_sem);
> -	mmput(owning_mm);
> -	put_task_struct(owning_process);
>  }
>
>  static void mlx4_ib_set_vma_data(struct vm_area_struct *vma,
> diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
> index f3e7d7c..136d64f 100644
> --- a/drivers/infiniband/hw/mlx5/main.c
> +++ b/drivers/infiniband/hw/mlx5/main.c
> @@ -1965,38 +1965,7 @@ static void mlx5_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
>  	struct vm_area_struct *vma;
>  	struct mlx5_ib_vma_private_data *vma_private, *n;
>  	struct mlx5_ib_ucontext *context = to_mucontext(ibcontext);
> -	struct task_struct *owning_process  = NULL;
> -	struct mm_struct   *owning_mm       = NULL;
>
> -	owning_process = get_pid_task(ibcontext->tgid, PIDTYPE_PID);
> -	if (!owning_process)
> -		return;
> -
> -	owning_mm = get_task_mm(owning_process);
> -	if (!owning_mm) {
> -		pr_info("no mm, disassociate ucontext is pending task termination\n");
> -		while (1) {
> -			put_task_struct(owning_process);
> -			usleep_range(1000, 2000);
> -			owning_process = get_pid_task(ibcontext->tgid,
> -						      PIDTYPE_PID);
> -			if (!owning_process ||
> -			    owning_process->state == TASK_DEAD) {
> -				pr_info("disassociate ucontext done, task was terminated\n");
> -				/* in case task was dead need to release the
> -				 * task struct.
> -				 */
> -				if (owning_process)
> -					put_task_struct(owning_process);
> -				return;
> -			}
> -		}
> -	}
> -
> -	/* need to protect from a race on closing the vma as part of
> -	 * mlx5_ib_vma_close.
> -	 */
> -	down_write(&owning_mm->mmap_sem);
>  	mutex_lock(&context->vma_private_list_mutex);
>  	list_for_each_entry_safe(vma_private, n, &context->vma_private_list,
>  				 list) {
> @@ -2013,9 +1982,6 @@ static void mlx5_ib_disassociate_ucontext(struct ib_ucontext *ibcontext)
>  		kfree(vma_private);
>  	}
>  	mutex_unlock(&context->vma_private_list_mutex);
> -	up_write(&owning_mm->mmap_sem);
> -	mmput(owning_mm);
> -	put_task_struct(owning_process);
>  }
>
>  static inline char *mmap_cmd2str(enum mlx5_ib_mmap_cmd cmd)
> --
> 1.9.1
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]