Re: [PATCHv6 2/3] IB/core: added support to use rdma cgroup controller

[Haggai Eran <haggaie@mellanox.com>]

On 20/02/2016 13:00, Parav Pandit wrote:
> +/**
> + * ib_device_unregister_rdmacg - unregister with rdma cgroup.
> + * @device: device to unregister.
> + *
> + * Unregister with the rdma cgroup. Should be called after
> + * all the resources are deallocated, and after a stage when any
> + * other resource allocation of user application cannot be done
> + * for this device to avoid any leak in accounting.
> + * HCA drivers should clear resource pool ops after ib stack
> + * unregisters with rdma cgroup.
HCA drivers don't supply their own ops in this version, right?
If so, you can update the comment.

> + */
> +void ib_device_unregister_rdmacg(struct ib_device *device)
> +{
> +	rdmacg_unregister_device(&device->cg_device);
> +}

> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index 179e813..c3bd24c 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -352,10 +352,22 @@ int ib_register_device(struct ib_device *device,
>  		goto out;
>  	}
>  
> +#ifdef CONFIG_CGROUP_RDMA
> +	ret = ib_device_register_rdmacg(device);
> +	if (ret) {
> +		printk(KERN_WARNING "Couldn't set up InfiniBand P_Key/GID cache\n");
You should update the error string, and I think checkpatch recommends
using pr_warn().

> +		ib_cache_cleanup_one(device);
> +		goto out;
> +	}
> +#endif
> +
>  	ret = ib_device_register_sysfs(device, port_callback);
>  	if (ret) {
>  		printk(KERN_WARNING "Couldn't register device %s with driver model\n",
>  		       device->name);
> +#ifdef CONFIG_CGROUP_RDMA
> +		ib_device_unregister_rdmacg(device);
> +#endif
>  		ib_cache_cleanup_one(device);
>  		goto out;
>  	}
> @@ -405,6 +417,10 @@ void ib_unregister_device(struct ib_device *device)
>  
>  	mutex_unlock(&device_mutex);
>  
> +#ifdef CONFIG_CGROUP_RDMA
> +	ib_device_unregister_rdmacg(device);
> +#endif
> +
>  	ib_device_unregister_sysfs(device);
>  	ib_cache_cleanup_one(device);
>  
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 1c02dea..7c51e8a 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c

> @@ -1777,6 +1851,12 @@ static int create_qp(struct ib_uverbs_file *file,
>  		  &qp_lock_class);
>  	down_write(&obj->uevent.uobject.mutex);
>  
> +	pd  = idr_read_pd(cmd->pd_handle, file->ucontext);
> +	if (!pd) {
> +		ret = -EINVAL;
> +		goto err_put;
> +	}
> +
I'm not sure I understand why you need to keep the PD here. Why 
don't you use the same ib_device that is used to create the QP? 
The same applies comment also applies to other uverbs commands.

>  	if (cmd->qp_type == IB_QPT_XRC_TGT) {
>  		xrcd = idr_read_xrcd(cmd->pd_handle, file->ucontext,
>  				     &xrcd_uobj);
> @@ -1811,8 +1891,7 @@ static int create_qp(struct ib_uverbs_file *file,
>  
>  		scq = idr_read_cq(cmd->send_cq_handle, file->ucontext, !!rcq);
>  		rcq = rcq ?: scq;
> -		pd  = idr_read_pd(cmd->pd_handle, file->ucontext);
> -		if (!pd || !scq) {
> +		if (!scq) {
>  			ret = -EINVAL;
>  			goto err_put;
>  		}
> @@ -1858,6 +1937,11 @@ static int create_qp(struct ib_uverbs_file *file,
>  			goto err_put;
>  		}
>  
> +	ret = ib_rdmacg_try_charge(&obj->uevent.uobject.cg_obj, pd->device,
> +				   RDMA_VERB_RESOURCE_QP, 1);
> +	if (ret)
> +		goto err_put;
> +
>  	if (cmd->qp_type == IB_QPT_XRC_TGT)
>  		qp = ib_create_qp(pd, &attr);
>  	else
> @@ -1865,7 +1949,7 @@ static int create_qp(struct ib_uverbs_file *file,
>  
>  	if (IS_ERR(qp)) {
>  		ret = PTR_ERR(qp);
> -		goto err_put;
> +		goto err_create;
>  	}
>  
>  	if (cmd->qp_type != IB_QPT_XRC_TGT) {
> @@ -1940,6 +2024,10 @@ err_cb:
>  err_destroy:
>  	ib_destroy_qp(qp);
>  
> +err_create:
> +	ib_rdmacg_uncharge(&obj->uevent.uobject.cg_obj, device,
> +			   RDMA_VERB_RESOURCE_QP, 1);
> +
>  err_put:
>  	if (xrcd)
>  		put_xrcd_read(xrcd_uobj);

> @@ -3323,6 +3441,11 @@ static int __uverbs_create_xsrq(struct ib_uverbs_file *file,
>  	obj->uevent.events_reported = 0;
>  	INIT_LIST_HEAD(&obj->uevent.event_list);
>  
> +	ret = ib_rdmacg_try_charge(&obj->uevent.uobject.cg_obj, pd->device,
> +				   RDMA_VERB_RESOURCE_SRQ, 1);
> +	if (ret)
> +		goto err_put_cq;
> +
I think you need a new error label to release the PD IDR but not
uncharge.

>  	srq = pd->device->create_srq(pd, &attr, udata);
>  	if (IS_ERR(srq)) {
>  		ret = PTR_ERR(srq);
> @@ -3387,6 +3510,8 @@ err_destroy:
>  	ib_destroy_srq(srq);
>  
>  err_put:
> +	ib_rdmacg_uncharge(&obj->uevent.uobject.cg_obj, pd->device,
> +			   RDMA_VERB_RESOURCE_SRQ, 1);
>  	put_pd_read(pd);
>  
>  err_put_cq:

Regards,
Haggai