Re: [PATCH 2/2] hw/rdma: Skip data-path mr_id translation

[Marcel Apfelbaum <marcel.apfelbaum@gmail.com>]

Hi Yuval,

On 3/7/20 2:56 PM, Yuval Shaia wrote:
> With the change made in commit 68b89aee71 ("Utilize ibv_reg_mr_iova for
> memory registration") the MR emulation is no longer needed in order to
> translate the guest addresses into host addresses.
> With that, the next obvious step is to skip entirely the processing in
> data-path.
> To accomplish this, return the backend's lkey to driver so we will not
> need to do the emulated mr_id to backend mr_id translation in data-path.
>
> The function build_host_sge_array is still called in data-path but only
> for backward computability with statistics collection.
>
> While there, as a cosmetic change to make the code cleaner - make one
> copy of the function rdma_backend_create_mr and leave the redundant
> guest_start argument in the legacy code.
>
> Signed-off-by: Yuval Shaia <yuval.shaia.ml@gmail.com>
> ---
>   hw/rdma/rdma_backend.c | 23 ++++++++++++++---------
>   hw/rdma/rdma_backend.h |  5 -----
>   hw/rdma/rdma_rm.c      | 13 ++++++-------
>   3 files changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index 79b9cfb487..3dd39fe1a7 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c
> @@ -377,6 +377,7 @@ static void ah_cache_init(void)
>                                       destroy_ah_hash_key, destroy_ah_hast_data);
>   }
>   
> +#ifdef LEGACY_RDMA_REG_MR
>   static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
>                                   struct ibv_sge *sge, uint8_t num_sge,
>                                   uint64_t *total_length)
> @@ -391,11 +392,7 @@ static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
>               return VENDOR_ERR_INVLKEY | sge[idx].lkey;
>           }
>   
> -#ifdef LEGACY_RDMA_REG_MR
>           sge[idx].addr = (uintptr_t)mr->virt + sge[idx].addr - mr->start;
> -#else
> -        sge[idx].addr = sge[idx].addr;
> -#endif
>           sge[idx].lkey = rdma_backend_mr_lkey(&mr->backend_mr);
>   
>           *total_length += sge[idx].length;
> @@ -403,6 +400,19 @@ static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
>   
>       return 0;
>   }
> +#else
> +static inline int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
> +                                       struct ibv_sge *sge, uint8_t num_sge,
> +                                       uint64_t *total_length)
> +{
> +    int idx;
> +
> +    for (idx = 0; idx < num_sge; idx++) {
> +        *total_length += sge[idx].length;
> +    }
> +    return 0;
> +}
> +#endif
>   
>   static void trace_mad_message(const char *title, char *buf, int len)
>   {
> @@ -733,13 +743,8 @@ void rdma_backend_destroy_pd(RdmaBackendPD *pd)
>       }
>   }
>   
> -#ifdef LEGACY_RDMA_REG_MR
> -int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
> -                           size_t length, int access)
> -#else
>   int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
>                              size_t length, uint64_t guest_start, int access)
> -#endif
>   {
>   #ifdef LEGACY_RDMA_REG_MR
>       mr->ibmr = ibv_reg_mr(pd->ibpd, addr, length, access);
> diff --git a/hw/rdma/rdma_backend.h b/hw/rdma/rdma_backend.h
> index 127f96e2d5..225af481e0 100644
> --- a/hw/rdma/rdma_backend.h
> +++ b/hw/rdma/rdma_backend.h
> @@ -78,13 +78,8 @@ int rdma_backend_query_port(RdmaBackendDev *backend_dev,
>   int rdma_backend_create_pd(RdmaBackendDev *backend_dev, RdmaBackendPD *pd);
>   void rdma_backend_destroy_pd(RdmaBackendPD *pd);
>   
> -#ifdef LEGACY_RDMA_REG_MR
> -int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
> -                           size_t length, int access);
> -#else
>   int rdma_backend_create_mr(RdmaBackendMR *mr, RdmaBackendPD *pd, void *addr,
>                              size_t length, uint64_t guest_start, int access);
> -#endif
>   void rdma_backend_destroy_mr(RdmaBackendMR *mr);
>   
>   int rdma_backend_create_cq(RdmaBackendDev *backend_dev, RdmaBackendCQ *cq,
> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index 1524dfaeaa..7e9ea283c9 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -227,21 +227,20 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, uint32_t pd_handle,
>           mr->length = guest_length;
>           mr->virt += (mr->start & (TARGET_PAGE_SIZE - 1));
>   
> -#ifdef LEGACY_RDMA_REG_MR
> -        ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
> -                                     mr->length, access_flags);
> -#else
>           ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, mr->virt,
>                                        mr->length, guest_start, access_flags);
> -#endif
>           if (ret) {
>               ret = -EIO;
>               goto out_dealloc_mr;
>           }
> +#ifdef LEGACY_RDMA_REG_MR
> +        /* We keep mr_handle in lkey so send and recv get get mr ptr */
> +        *lkey = *mr_handle;
> +#else
> +        *lkey = rdma_backend_mr_lkey(&mr->backend_mr);
> +#endif
>       }
>   
> -    /* We keep mr_handle in lkey so send and recv get get mr ptr */
> -    *lkey = *mr_handle;
>       *rkey = -1;
>   
>       mr->pd_handle = pd_handle;

Reviewed-by: Marcel Apfelbaum<marcel.apfelbaum@gmail.com>

Thanks,
Marcel