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

Sagi Grimberg
2015-07-23T10:47:02Z

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?