* [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe
@ 2022-03-28 15:17 Xiao Yang
2022-03-28 18:47 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: Xiao Yang @ 2022-03-28 15:17 UTC (permalink / raw)
To: linux-rdma; +Cc: yanjun.zhu, leonro, jgg, rpearsonhpe, Xiao Yang
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;
+ 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-28 15:17 [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe Xiao Yang
@ 2022-03-28 18:47 ` Leon Romanovsky
2022-03-29 3:55 ` yangx.jy
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2022-03-28 18:47 UTC (permalink / raw)
To: Xiao Yang; +Cc: linux-rdma, yanjun.zhu, jgg, rpearsonhpe
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
>
>
>
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-28 18:47 ` Leon Romanovsky
@ 2022-03-29 3:55 ` yangx.jy
2022-03-30 7:44 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: yangx.jy @ 2022-03-29 3:55 UTC (permalink / raw)
To: Leon Romanovsky, yanjun.zhu, rpearsonhpe; +Cc: linux-rdma, jgg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-29 3:55 ` yangx.jy
@ 2022-03-30 7:44 ` Leon Romanovsky
2022-03-31 5:55 ` yangx.jy
0 siblings, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2022-03-30 7:44 UTC (permalink / raw)
To: yangx.jy; +Cc: yanjun.zhu, rpearsonhpe, linux-rdma, jgg
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-30 7:44 ` Leon Romanovsky
@ 2022-03-31 5:55 ` yangx.jy
2022-03-31 8:51 ` Leon Romanovsky
0 siblings, 1 reply; 6+ messages in thread
From: yangx.jy @ 2022-03-31 5:55 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: yanjun.zhu, rpearsonhpe, linux-rdma, jgg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-31 5:55 ` yangx.jy
@ 2022-03-31 8:51 ` Leon Romanovsky
0 siblings, 0 replies; 6+ messages in thread
From: Leon Romanovsky @ 2022-03-31 8:51 UTC (permalink / raw)
To: yangx.jy; +Cc: yanjun.zhu, rpearsonhpe, linux-rdma, jgg
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-31 9:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-28 15:17 [PATCH v2] RDMA/rxe: Generate a completion on error after getting a wqe Xiao Yang
2022-03-28 18:47 ` Leon Romanovsky
2022-03-29 3:55 ` yangx.jy
2022-03-30 7:44 ` Leon Romanovsky
2022-03-31 5:55 ` yangx.jy
2022-03-31 8:51 ` Leon Romanovsky
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.