Re: [Qemu-devel] [PATCH 07/13] hw/rdma: Do not allocate memory for non-dma MR

[Yuval Shaia]

On Tue, Jul 24, 2018 at 03:19:52PM +0300, Marcel Apfelbaum wrote:
> 
> Hi Yuval,
> 
> On 07/16/2018 10:40 AM, Yuval Shaia wrote:
> > There is no use in the memory allocated for non-dma MR (one with
> > host_virt equals to NULL).
> 
> No need for the (one with...)

Will remove.

> 
> > Delete the code that allocates it.
> > 
> > Signed-off-by: Yuval Shaia <address@hidden>
> > ---
> >   hw/rdma/rdma_rm.c | 52 +++++++++++++++++++----------------------------
> >   1 file changed, 21 insertions(+), 31 deletions(-)
> > 
> > diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> > index 7403d24674..bf4a5c71b4 100644
> > --- a/hw/rdma/rdma_rm.c
> > +++ b/hw/rdma/rdma_rm.c
> > @@ -144,8 +144,6 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, 
> > uint32_t pd_handle,
> >       RdmaRmMR *mr;
> >       int ret = 0;
> >       RdmaRmPD *pd;
> > -    void *addr;
> > -    size_t length;
> >       pd = rdma_rm_get_pd(dev_res, pd_handle);
> >       if (!pd) {
> > @@ -158,40 +156,29 @@ int rdma_rm_alloc_mr(RdmaDeviceResources *dev_res, 
> > uint32_t pd_handle,
> >           pr_dbg("Failed to allocate obj in table\n");
> >           return -ENOMEM;
> >       }
> > +    pr_dbg("mr_handle=%d\n", *mr_handle);
> > -    if (!host_virt) {
> > -        /* TODO: This is my guess but not so sure that this needs to be
> > -         * done */
> > -        length = TARGET_PAGE_SIZE;
> > -        addr = g_malloc(length);
> > -    } else {
> > +    pr_dbg("host_virt=0x%p\n", host_virt);
> > +    pr_dbg("guest_start=0x%" PRIx64 "\n", guest_start);
> > +    pr_dbg("length=%zu\n", guest_length);
> > +
> > +    if (host_virt) {
> >           mr->virt = host_virt;
> > -        pr_dbg("host_virt=0x%p\n", mr->virt);
> > -        mr->length = guest_length;
> > -        pr_dbg("length=%zu\n", guest_length);
> >           mr->start = guest_start;

[1]

> > -        pr_dbg("guest_start=0x%" PRIx64 "\n", mr->start);
> > -
> > -        length = mr->length;
> > -        addr = mr->virt;
> > -    }
> > +        mr->length = guest_length;
> > -    ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, addr, 
> > length,
> > -                                 access_flags);
> > -    if (ret) {
> > -        pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
> > -        ret = -EIO;
> > -        goto out_dealloc_mr;
> > +        ret = rdma_backend_create_mr(&mr->backend_mr, &pd->backend_pd, 
> > mr->virt,
> > +                                     mr->length, access_flags);
> > +        if (ret) {
> > +            pr_dbg("Fail in rdma_backend_create_mr, err=%d\n", ret);
> > +            ret = -EIO;
> > +            goto out_dealloc_mr;
> > +        }
> >       }
> > -    if (!host_virt) {
> > -        *lkey = mr->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
> > -        *rkey = mr->rkey = rdma_backend_mr_rkey(&mr->backend_mr);
> > -    } else {
> > -        /* We keep mr_handle in lkey so send and recv get get mr ptr */
> > -        *lkey = *mr_handle;
> > -        *rkey = -1;
> > -    }
> > +    /* We keep mr_handle in lkey so send and recv get get mr ptr */
> > +    *lkey = *mr_handle;
> > +    *rkey = -1;
> 
> Before this change rkey whould get a value when !host_virt.
> But I suppose is OK since Remote DMA operations are not implemented yet.

The entire code that handled the case where host_virt is NULL was wrong
thus removed.
And yes, RDMA verb is not yet implemented so rkey is anyway not needed.

> 
> >       mr->pd_handle = pd_handle;
> > @@ -214,7 +201,10 @@ void rdma_rm_dealloc_mr(RdmaDeviceResources *dev_res, 
> > uint32_t mr_handle)
> >       if (mr) {
> >           rdma_backend_destroy_mr(&mr->backend_mr);
> > -        munmap(mr->virt, mr->length);
> > +        pr_dbg("start=0x%" PRIx64 "\n", mr->start);
> > +        if (mr->start) {
> 
> When is the mr->start inited?

res_tbl_alloc cleans the MR before giving it to caller so we expect
mr->start to be NULL.
Then if host_virt is given then mr->start is set to guest virtual address
[1].

> 
> Thanks,
> Marcel
> 
> > +            munmap(mr->virt, mr->length);
> > +        }
> >           res_tbl_dealloc(&dev_res->mr_tbl, mr_handle);
> >       }
> >   }
>