Re: [PATCH 01/10] IB/core: Guarantee that a local_dma_lkey is available

[Sagi Grimberg <sagig@dev.mellanox.co.il>]

On 7/23/2015 2:34 AM, Jason Gunthorpe wrote:
> Every single ULP requires a local_dma_lkey to do anything with
> a QP, so let us ensure one exists for every PD created.
>
> If the driver can supply a global local_dma_lkey then use that, otherwise
> ask the driver to create a local use all physical memory MR associated
> with the new PD.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Reviewed-by: Sagi Grimberg <sagig@dev.mellanox.co.il>
> Acked-by: Christoph Hellwig <hch@infradead.org>
> ---
>   drivers/infiniband/core/verbs.c | 40 +++++++++++++++++++++++++++++++++++-----
>   include/rdma/ib_verbs.h         |  2 ++
>   2 files changed, 37 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
> index bac3fb406a74..1ddf06314f36 100644
> --- a/drivers/infiniband/core/verbs.c
> +++ b/drivers/infiniband/core/verbs.c
> @@ -213,24 +213,54 @@ EXPORT_SYMBOL(rdma_port_get_link_layer);
>
>   /* Protection domains */
>
> +/* Return a pd for in-kernel use that has a local_dma_lkey which provides
> +   local access to all physical memory. */

Why not kdoc style? we need to move the ib_verbs.h kdocs here anyway.
Might be a good chance to do that for ib_alloc_pd().

>   struct ib_pd *ib_alloc_pd(struct ib_device *device)
>   {
>   	struct ib_pd *pd;
> +	struct ib_device_attr devattr;
> +	int rc;
> +
> +	rc = ib_query_device(device, &devattr);
> +	if (rc)
> +		return ERR_PTR(rc);
>
>   	pd = device->alloc_pd(device, NULL, NULL);
> +	if (IS_ERR(pd))
> +		return pd;
> +
> +	pd->device = device;
> +	pd->uobject = NULL;
> +	pd->local_mr = NULL;
> +	atomic_set(&pd->usecnt, 0);
> +
> +	if (devattr.device_cap_flags & IB_DEVICE_LOCAL_DMA_LKEY)
> +		pd->local_dma_lkey = device->local_dma_lkey;
> +	else {
> +		struct ib_mr *mr;
> +
> +		mr = ib_get_dma_mr(pd, IB_ACCESS_LOCAL_WRITE);
> +		if (IS_ERR(mr)) {
> +			ib_dealloc_pd(pd);
> +			return (struct ib_pd *)mr;
> +		}
>
> -	if (!IS_ERR(pd)) {
> -		pd->device  = device;
> -		pd->uobject = NULL;
> -		atomic_set(&pd->usecnt, 0);
> +		pd->local_mr = mr;
> +		pd->local_dma_lkey = pd->local_mr->lkey;
>   	}
> -
>   	return pd;
>   }
> +
>   EXPORT_SYMBOL(ib_alloc_pd);

You have an extra newline here.

>
>   int ib_dealloc_pd(struct ib_pd *pd)
>   {
> +	if (pd->local_mr) {
> +		if (ib_dereg_mr(pd->local_mr))
> +			return -EBUSY;
> +		pd->local_mr = NULL;
> +	}
> +
>   	if (atomic_read(&pd->usecnt))
>   		return -EBUSY;
>
> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index 986fddb08579..cfda95d7b067 100644
> --- a/include/rdma/ib_verbs.h
> +++ b/include/rdma/ib_verbs.h
> @@ -1255,6 +1255,8 @@ struct ib_pd {
>   	struct ib_device       *device;
>   	struct ib_uobject      *uobject;
>   	atomic_t          	usecnt; /* count all resources */
> +	struct ib_mr	       *local_mr;
> +	u32			local_dma_lkey;

Maybe its better to place the local_dma_lkey in the first cacheline as
it is normally accessed in the hot path?