On Mon, 16 Mar 2020 at 15:30, Marcel Apfelbaum <marcel.apfelb...@gmail.com>
wrote:
> Hi Yuval,
>
> On 3/7/20 2:56 PM, Yuval Shaia wrote:
> > The function build_host_sge_array uses two sge arrays, one for input and
> > one for output.
> > Since the size of the two arrays is the same, the function can write
> > directly to the given source array (i.e. input/output argument).
> >
> > Signed-off-by: Yuval Shaia <yuval.shaia...@gmail.com>
> > ---
> > hw/rdma/rdma_backend.c | 40 +++++++++++++++++-----------------------
> > 1 file changed, 17 insertions(+), 23 deletions(-)
> >
> > diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> > index c346407cd3..79b9cfb487 100644
> > --- a/hw/rdma/rdma_backend.c
> > +++ b/hw/rdma/rdma_backend.c
> > @@ -378,30 +378,27 @@ static void ah_cache_init(void)
> > }
> >
> > static int build_host_sge_array(RdmaDeviceResources *rdma_dev_res,
> > - struct ibv_sge *dsge, struct ibv_sge
> *ssge,
> > - uint8_t num_sge, uint64_t *total_length)
> > + struct ibv_sge *sge, uint8_t num_sge,
> > + uint64_t *total_length)
> > {
> > RdmaRmMR *mr;
> > - int ssge_idx;
> > + int idx;
> >
> > - for (ssge_idx = 0; ssge_idx < num_sge; ssge_idx++) {
> > - mr = rdma_rm_get_mr(rdma_dev_res, ssge[ssge_idx].lkey);
> > + for (idx = 0; idx < num_sge; idx++) {
> > + mr = rdma_rm_get_mr(rdma_dev_res, sge[idx].lkey);
> > if (unlikely(!mr)) {
> > - rdma_error_report("Invalid lkey 0x%x", ssge[ssge_idx].lkey);
> > - return VENDOR_ERR_INVLKEY | ssge[ssge_idx].lkey;
> > + rdma_error_report("Invalid lkey 0x%x", sge[idx].lkey);
> > + return VENDOR_ERR_INVLKEY | sge[idx].lkey;
> > }
> >
> > #ifdef LEGACY_RDMA_REG_MR
> > - dsge->addr = (uintptr_t)mr->virt + ssge[ssge_idx].addr -
> mr->start;
> > + sge[idx].addr = (uintptr_t)mr->virt + sge[idx].addr - mr->start;
> > #else
> > - dsge->addr = ssge[ssge_idx].addr;
> > + sge[idx].addr = sge[idx].addr;
>
> It seems we don't need the above line.
> Other than that it looks good to me.
>
Yeah, thanks.
It is fixed in the next patch but better be fixed here.
Will fix and post new set.
>
> Thanks,
> Marcel
>
> > #endif
> > - dsge->length = ssge[ssge_idx].length;
> > - dsge->lkey = rdma_backend_mr_lkey(&mr->backend_mr);
> > + sge[idx].lkey = rdma_backend_mr_lkey(&mr->backend_mr);
> >
> > - *total_length += dsge->length;
> > -
> > - dsge++;
> > + *total_length += sge[idx].length;
> > }
> >
> > return 0;
> > @@ -484,7 +481,6 @@ void rdma_backend_post_send(RdmaBackendDev
> *backend_dev,
> > void *ctx)
> > {
> > BackendCtx *bctx;
> > - struct ibv_sge new_sge[MAX_SGE];
> > uint32_t bctx_id;
> > int rc;
> > struct ibv_send_wr wr = {}, *bad_wr;
> > @@ -518,7 +514,7 @@ void rdma_backend_post_send(RdmaBackendDev
> *backend_dev,
> >
> > rdma_protected_gslist_append_int32(&qp->cqe_ctx_list, bctx_id);
> >
> > - rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge,
> num_sge,
> > + rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
> >
> &backend_dev->rdma_dev_res->stats.tx_len);
> > if (rc) {
> > complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
> > @@ -538,7 +534,7 @@ void rdma_backend_post_send(RdmaBackendDev
> *backend_dev,
> > wr.num_sge = num_sge;
> > wr.opcode = IBV_WR_SEND;
> > wr.send_flags = IBV_SEND_SIGNALED;
> > - wr.sg_list = new_sge;
> > + wr.sg_list = sge;
> > wr.wr_id = bctx_id;
> >
> > rc = ibv_post_send(qp->ibqp, &wr, &bad_wr);
> > @@ -601,7 +597,6 @@ void rdma_backend_post_recv(RdmaBackendDev
> *backend_dev,
> > struct ibv_sge *sge, uint32_t num_sge,
> void *ctx)
> > {
> > BackendCtx *bctx;
> > - struct ibv_sge new_sge[MAX_SGE];
> > uint32_t bctx_id;
> > int rc;
> > struct ibv_recv_wr wr = {}, *bad_wr;
> > @@ -635,7 +630,7 @@ void rdma_backend_post_recv(RdmaBackendDev
> *backend_dev,
> >
> > rdma_protected_gslist_append_int32(&qp->cqe_ctx_list, bctx_id);
> >
> > - rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge,
> num_sge,
> > + rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
> >
> &backend_dev->rdma_dev_res->stats.rx_bufs_len);
> > if (rc) {
> > complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
> > @@ -643,7 +638,7 @@ void rdma_backend_post_recv(RdmaBackendDev
> *backend_dev,
> > }
> >
> > wr.num_sge = num_sge;
> > - wr.sg_list = new_sge;
> > + wr.sg_list = sge;
> > wr.wr_id = bctx_id;
> > rc = ibv_post_recv(qp->ibqp, &wr, &bad_wr);
> > if (rc) {
> > @@ -671,7 +666,6 @@ void rdma_backend_post_srq_recv(RdmaBackendDev
> *backend_dev,
> > uint32_t num_sge, void *ctx)
> > {
> > BackendCtx *bctx;
> > - struct ibv_sge new_sge[MAX_SGE];
> > uint32_t bctx_id;
> > int rc;
> > struct ibv_recv_wr wr = {}, *bad_wr;
> > @@ -688,7 +682,7 @@ void rdma_backend_post_srq_recv(RdmaBackendDev
> *backend_dev,
> >
> > rdma_protected_gslist_append_int32(&srq->cqe_ctx_list, bctx_id);
> >
> > - rc = build_host_sge_array(backend_dev->rdma_dev_res, new_sge, sge,
> num_sge,
> > + rc = build_host_sge_array(backend_dev->rdma_dev_res, sge, num_sge,
> >
> &backend_dev->rdma_dev_res->stats.rx_bufs_len);
> > if (rc) {
> > complete_work(IBV_WC_GENERAL_ERR, rc, ctx);
> > @@ -696,7 +690,7 @@ void rdma_backend_post_srq_recv(RdmaBackendDev
> *backend_dev,
> > }
> >
> > wr.num_sge = num_sge;
> > - wr.sg_list = new_sge;
> > + wr.sg_list = sge;
> > wr.wr_id = bctx_id;
> > rc = ibv_post_srq_recv(srq->ibsrq, &wr, &bad_wr);
> > if (rc) {
>
>
