Re: [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe

[Leon Romanovsky <leonro@xxxxxxxxxx>]

On Tue, Mar 29, 2022 at 03:55:07AM +0000, yangx.jy@xxxxxxxxxxx wrote:
> On 2022/3/29 2:47, Leon Romanovsky wrote:
> > I see that you put same wqe->status for all error paths.
> > If we assume that same status needs to be for all errors, you will better
> > put this line under err label.
> >
> > diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> > index 5eb89052dd66..003a9b109eff 100644
> > --- a/drivers/infiniband/sw/rxe/rxe_req.c
> > +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> > @@ -752,6 +752,7 @@ int rxe_requester(void *arg)
> >          goto next_wqe;
> >
> >   err:
> > +       wqe->status = IB_WC_LOC_QP_OP_ERR;
> >          wqe->state = wqe_state_error;
> >          __rxe_do_task(&qp->comp.task);
> >
> >
> > BTW, I didn't review if same error status is applicable for all paths.
> 
> Hi Leon,
> 
> It's not suitable to set the same IB_WC_LOC_QP_OP_ERR for all paths 
> because other error status also will be set in some places.
> 
> For example:
> 
> IB_WC_LOC_QP_OP_ERR or IB_WC_MW_BIND_ERR will be set in rxe_do_local_ops()
> 
> IB_WC_LOC_PROT_ERR or IB_WC_LOC_QP_OP_ERR will be set by checking the 
> return value of finish_packet()

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index 5eb89052dd66..5515a095cbba 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -657,26 +657,24 @@ int rxe_requester(void *arg)
                psn_compare(qp->req.psn, (qp->comp.psn +
                                RXE_MAX_UNACKED_PSNS)) > 0)) {
                qp->req.wait_psn = 1;
-               goto exit;
+               goto qp_err;
        }
 
        /* Limit the number of inflight SKBs per QP */
        if (unlikely(atomic_read(&qp->skb_out) >
                     RXE_INFLIGHT_SKBS_PER_QP_HIGH)) {
                qp->need_req_skb = 1;
-               goto exit;
+               goto qp_err;
        }
 
        opcode = next_opcode(qp, wqe, wqe->wr.opcode);
-       if (unlikely(opcode < 0)) {
-               wqe->status = IB_WC_LOC_QP_OP_ERR;
-               goto exit;
-       }
+       if (unlikely(opcode < 0))
+               goto qp_err;
 
        mask = rxe_opcode[opcode].mask;
        if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) {
                if (check_init_depth(qp, wqe))
-                       goto exit;
+                       goto qp_err;
        }
 
        mtu = get_mtu(qp);
@@ -706,11 +704,8 @@ int rxe_requester(void *arg)
        }
 
        skb = init_req_packet(qp, wqe, opcode, payload, &pkt);
-       if (unlikely(!skb)) {
-               pr_err("qp#%d Failed allocating skb\n", qp_num(qp));
-               wqe->status = IB_WC_LOC_QP_OP_ERR;
-               goto err;
-       }
+       if (unlikely(!skb))
+               goto qp_err;
 
        ret = finish_packet(qp, wqe, &pkt, skb, payload);
        if (unlikely(ret)) {
@@ -742,15 +737,15 @@ int rxe_requester(void *arg)
                        rxe_run_task(&qp->req.task, 1);
                        goto exit;
                }
-
-               wqe->status = IB_WC_LOC_QP_OP_ERR;
-               goto err;
+               goto qp_err;
        }
 
        update_state(qp, wqe, &pkt, payload);
 
        goto next_wqe;
 
+qp_err:
+       wqe->status = IB_WC_LOC_QP_OP_ERR;
 err:
        wqe->state = wqe_state_error;
        __rxe_do_task(&qp->comp.task);

> 
> 
> Hi Leon, Bob, Yanjun
> 
> How can I know if IB_WC_LOC_QP_OP_ERR is suitable for all changes on my 
> patch?
> 
> In other words,  how can I know which error status should be used in 
> which case?

You will need to open IBTA spec and try to understand from it.
But realistically speaking, I think that it will be too hard to do it
and not sure if it is really important.

Please resubmit your patch with updated diff and commit message.

Thanks

> 
> 
> Best Regards,
> 
> Xiao Yang
> 
> >
> > Thanks