| Message ID | 20220328151748.304551-1-yangx.jy@fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] RDMA/rxe: Generate a completion on error after getting a wqe | expand |
On Mon, Mar 28, 2022 at 11:17:48PM +0800, Xiao Yang wrote: > Current rxe_requester() doesn't generate a completion on error after > getting a wqe. Fix the issue by calling "goto err;" instead. > > Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> > --- > drivers/infiniband/sw/rxe/rxe_req.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) > > diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c > index ae5fbc79dd5c..e69fe409fbcb 100644 > --- a/drivers/infiniband/sw/rxe/rxe_req.c > +++ b/drivers/infiniband/sw/rxe/rxe_req.c > @@ -648,26 +648,30 @@ 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; > + wqe->status = IB_WC_LOC_QP_OP_ERR; 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. Thanks > + goto 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; > + wqe->status = IB_WC_LOC_QP_OP_ERR; > + goto err; > } > > opcode = next_opcode(qp, wqe, wqe->wr.opcode); > if (unlikely(opcode < 0)) { > wqe->status = IB_WC_LOC_QP_OP_ERR; > - goto exit; > + goto err; > } > > mask = rxe_opcode[opcode].mask; > if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) { > - if (check_init_depth(qp, wqe)) > - goto exit; > + if (check_init_depth(qp, wqe)) { > + wqe->status = IB_WC_LOC_QP_OP_ERR; > + goto err; > + } > } > > mtu = get_mtu(qp); > -- > 2.25.4 > > >
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() 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? Best Regards, Xiao Yang > > Thanks
On Tue, Mar 29, 2022 at 03:55:07AM +0000, yangx.jy@fujitsu.com 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
On 2022/3/30 15:44, Leon Romanovsky wrote: > 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. Hi Leon, It seems that your change is based on old code. After looking into the latest code, this change seem not simpler and clearer. do you think so? Best Regards, Xiao Yang > > Thanks
On Thu, Mar 31, 2022 at 05:55:59AM +0000, yangx.jy@fujitsu.com wrote: > On 2022/3/30 15:44, Leon Romanovsky wrote: > > 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. > > Hi Leon, > > It seems that your change is based on old code. > > After looking into the latest code, this change seem not simpler and > clearer. do you think so? No, something like that will do the trick. diff --git a/drivers/infiniband/sw/rxe/rxe_av.c b/drivers/infiniband/sw/rxe/rxe_av.c index 3b05314ca739..5938cba936d9 100644 --- a/drivers/infiniband/sw/rxe/rxe_av.c +++ b/drivers/infiniband/sw/rxe/rxe_av.c @@ -104,9 +104,6 @@ struct rxe_av *rxe_get_av(struct rxe_pkt_info *pkt, struct rxe_ah **ahp) struct rxe_ah *ah; u32 ah_num; - if (ahp) - *ahp = NULL; - if (!pkt || !pkt->qp) return NULL; diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c index ae5fbc79dd5c..9afc8cf50863 100644 --- a/drivers/infiniband/sw/rxe/rxe_req.c +++ b/drivers/infiniband/sw/rxe/rxe_req.c @@ -648,26 +648,30 @@ 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; + wqe->status = IB_WC_LOC_QP_OP_ERR; + goto 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; + wqe->status = IB_WC_LOC_QP_OP_ERR; + goto err; } opcode = next_opcode(qp, wqe, wqe->wr.opcode); if (unlikely(opcode < 0)) { wqe->status = IB_WC_LOC_QP_OP_ERR; - goto exit; + goto err; } mask = rxe_opcode[opcode].mask; if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) { - if (check_init_depth(qp, wqe)) - goto exit; + if (check_init_depth(qp, wqe)) { + wqe->status = IB_WC_LOC_QP_OP_ERR; + goto err; + } } mtu = get_mtu(qp); @@ -704,32 +708,27 @@ int rxe_requester(void *arg) pkt.wqe = wqe; av = rxe_get_av(&pkt, &ah); - if (unlikely(!av)) { - pr_err("qp#%d Failed no address vector\n", qp_num(qp)); - wqe->status = IB_WC_LOC_QP_OP_ERR; - goto err_drop_ah; - } + if (unlikely(!av)) + goto qp_err; skb = init_req_packet(qp, av, 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_drop_ah; + rxe_put(ah); + goto qp_err; } ret = finish_packet(qp, av, wqe, &pkt, skb, payload); if (unlikely(ret)) { - pr_debug("qp#%d Error during finish packet\n", qp_num(qp)); if (ret == -EFAULT) wqe->status = IB_WC_LOC_PROT_ERR; else wqe->status = IB_WC_LOC_QP_OP_ERR; kfree_skb(skb); - goto err_drop_ah; + rxe_put(ah); + goto err; } - if (ah) - rxe_put(ah); + rxe_put(ah); /* * To prevent a race on wqe access between requester and completer, @@ -751,17 +750,15 @@ int rxe_requester(void *arg) goto exit; } - wqe->status = IB_WC_LOC_QP_OP_ERR; - goto err; + goto qp_err; } update_state(qp, wqe, &pkt); goto next_wqe; -err_drop_ah: - if (ah) - rxe_put(ah); +qp_err: + wqe->status = IB_WC_LOC_QP_OP_ERR; err: wqe->state = wqe_state_error; __rxe_do_task(&qp->comp.task); > > Best Regards, > > Xiao Yang > > > > > Thanks
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c index ae5fbc79dd5c..e69fe409fbcb 100644 --- a/drivers/infiniband/sw/rxe/rxe_req.c +++ b/drivers/infiniband/sw/rxe/rxe_req.c @@ -648,26 +648,30 @@ 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; + wqe->status = IB_WC_LOC_QP_OP_ERR; + goto 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; + wqe->status = IB_WC_LOC_QP_OP_ERR; + goto err; } opcode = next_opcode(qp, wqe, wqe->wr.opcode); if (unlikely(opcode < 0)) { wqe->status = IB_WC_LOC_QP_OP_ERR; - goto exit; + goto err; } mask = rxe_opcode[opcode].mask; if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) { - if (check_init_depth(qp, wqe)) - goto exit; + if (check_init_depth(qp, wqe)) { + wqe->status = IB_WC_LOC_QP_OP_ERR; + goto err; + } } mtu = get_mtu(qp);
Current rxe_requester() doesn't generate a completion on error after getting a wqe. Fix the issue by calling "goto err;" instead. Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com> --- drivers/infiniband/sw/rxe/rxe_req.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)