* [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe
@ 2022-03-31 12:02 Xiao Yang
2022-03-31 17:19 ` Leon Romanovsky
2022-04-07 21:39 ` Bob Pearson
0 siblings, 2 replies; 6+ messages in thread
From: Xiao Yang @ 2022-03-31 12:02 UTC (permalink / raw)
To: leon, jgg; +Cc: linux-rdma, Xiao Yang
Current rxe_requester() doesn't generate a completion on error after
getting a wqe. Fix the issue by calling rxe_completer() on error.
Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
drivers/infiniband/sw/rxe/rxe_req.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
index ae5fbc79dd5c..01ae400e5481 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -648,26 +648,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_op_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_op_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_op_err;
mask = rxe_opcode[opcode].mask;
if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) {
if (check_init_depth(qp, wqe))
- goto exit;
+ goto qp_op_err;
}
mtu = get_mtu(qp);
@@ -706,26 +704,26 @@ int rxe_requester(void *arg)
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;
}
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;
}
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 (ah)
+ rxe_put(ah);
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;
+ goto err;
}
if (ah)
@@ -751,8 +749,7 @@ int rxe_requester(void *arg)
goto exit;
}
- wqe->status = IB_WC_LOC_QP_OP_ERR;
- goto err;
+ goto qp_op_err;
}
update_state(qp, wqe, &pkt);
@@ -762,6 +759,9 @@ int rxe_requester(void *arg)
err_drop_ah:
if (ah)
rxe_put(ah);
+
+qp_op_err:
+ wqe->status = IB_WC_LOC_QP_OP_ERR;
err:
wqe->state = wqe_state_error;
__rxe_do_task(&qp->comp.task);
--
2.25.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-31 12:02 [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe Xiao Yang
@ 2022-03-31 17:19 ` Leon Romanovsky
[not found] ` <8b90851c-72ba-1eb2-7e94-ee7d6a178fc7@fujitsu.com>
2022-04-07 21:39 ` Bob Pearson
1 sibling, 1 reply; 6+ messages in thread
From: Leon Romanovsky @ 2022-03-31 17:19 UTC (permalink / raw)
To: Xiao Yang; +Cc: jgg, linux-rdma
On Thu, Mar 31, 2022 at 08:02:45PM +0800, Xiao Yang wrote:
> Current rxe_requester() doesn't generate a completion on error after
> getting a wqe. Fix the issue by calling rxe_completer() on error.
>
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
> drivers/infiniband/sw/rxe/rxe_req.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index ae5fbc79dd5c..01ae400e5481 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -648,26 +648,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_op_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_op_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_op_err;
>
> mask = rxe_opcode[opcode].mask;
> if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) {
> if (check_init_depth(qp, wqe))
> - goto exit;
> + goto qp_op_err;
> }
>
> mtu = get_mtu(qp);
> @@ -706,26 +704,26 @@ int rxe_requester(void *arg)
> 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;
> }
>
> 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;
> }
>
> 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 (ah)
No, ah can't be NULL. This is why I proposed to clean rxe_get_av() too.
**ahp is not NULL, as an input to that function and in all flows it is
updated with new ah pointer. If it can't update, the NULL will be
returned and rxe_requester() will exit.
Thanks
> + rxe_put(ah);
> 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;
> + goto err;
> }
>
> if (ah)
> @@ -751,8 +749,7 @@ int rxe_requester(void *arg)
> goto exit;
> }
>
> - wqe->status = IB_WC_LOC_QP_OP_ERR;
> - goto err;
> + goto qp_op_err;
> }
>
> update_state(qp, wqe, &pkt);
> @@ -762,6 +759,9 @@ int rxe_requester(void *arg)
> err_drop_ah:
> if (ah)
> rxe_put(ah);
> +
> +qp_op_err:
> + wqe->status = IB_WC_LOC_QP_OP_ERR;
> err:
> wqe->state = wqe_state_error;
> __rxe_do_task(&qp->comp.task);
> --
> 2.25.4
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe
2022-03-31 12:02 [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe Xiao Yang
2022-03-31 17:19 ` Leon Romanovsky
@ 2022-04-07 21:39 ` Bob Pearson
2022-04-08 3:14 ` yangx.jy
1 sibling, 1 reply; 6+ messages in thread
From: Bob Pearson @ 2022-04-07 21:39 UTC (permalink / raw)
To: Xiao Yang, leon, jgg; +Cc: linux-rdma
On 3/31/22 07:02, Xiao Yang wrote:
> Current rxe_requester() doesn't generate a completion on error after
> getting a wqe. Fix the issue by calling rxe_completer() on error.
>
> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
> ---
> drivers/infiniband/sw/rxe/rxe_req.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
> index ae5fbc79dd5c..01ae400e5481 100644
> --- a/drivers/infiniband/sw/rxe/rxe_req.c
> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
> @@ -648,26 +648,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_op_err;
> }
This isn't an error. What is happening is the requester has advanced too far into the
work request queue compared to what has been completed. So it sets a flag and exits the
loop. When the completer finishes another completion it will reschedule the requester.
RXE_MAX_UNACKED_PSNS is 128 so only 128 packets are allowed in the network without
seeing an ack.
>
> /* 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_op_err;
> }
This also is not an error. Here there is a limit on the number SKBs in flight.
The SKBs are consumed by the ethernet driver when the packet is put on the wire.
This prevents the driver from using too much memory when the NIC is paused.
>
> opcode = next_opcode(qp, wqe, wqe->wr.opcode);
> - if (unlikely(opcode < 0)) {
> - wqe->status = IB_WC_LOC_QP_OP_ERR;
> - goto exit;
> - }
This really is an error, but one that shouldn't happen under normal
circumstances since it implies an illegal operation. Like attempting
a write on a UD QP. Or causing an invalid opcode sequence.
> + if (unlikely(opcode < 0))
> + goto qp_op_err;
>
> mask = rxe_opcode[opcode].mask;
> if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) {
> if (check_init_depth(qp, wqe))
> - goto exit;
> + goto qp_op_err;
> }
This isn't an error. It means that someone posted a read/atomic operation
and there are too many pending read/atomic operations pending. You just need
to wait for one of them to complete so it is another pause.
>
> mtu = get_mtu(qp);
> @@ -706,26 +704,26 @@ int rxe_requester(void *arg)
> 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;
> }
This is an error. It could happen if the address handle referred to by
the WR is not longer valid. There should always be an address vector but
there may or may not be an address handle if the AV comes from a connected
QP or not.
>
> 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;
> }
>
> 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 (ah)
> + rxe_put(ah);
> 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;
> + goto err;
> }
Not sure the point of this.
>
> if (ah)
> @@ -751,8 +749,7 @@ int rxe_requester(void *arg)
> goto exit;
> }
>
> - wqe->status = IB_WC_LOC_QP_OP_ERR;
> - goto err;
> + goto qp_op_err;
> }
>
> update_state(qp, wqe, &pkt);
> @@ -762,6 +759,9 @@ int rxe_requester(void *arg)
> err_drop_ah:
> if (ah)
> rxe_put(ah);
> +
> +qp_op_err:
> + wqe->status = IB_WC_LOC_QP_OP_ERR;
> err:
> wqe->state = wqe_state_error;
> __rxe_do_task(&qp->comp.task);
Near as I can tell all you did was turn a few pauses into errors which is wrong
and otherwise move things around. What were you trying to solve here?
Bob
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe
2022-04-07 21:39 ` Bob Pearson
@ 2022-04-08 3:14 ` yangx.jy
0 siblings, 0 replies; 6+ messages in thread
From: yangx.jy @ 2022-04-08 3:14 UTC (permalink / raw)
To: Bob Pearson, leon, jgg; +Cc: linux-rdma
Hi Bob,
Thanks a lot for your detailed comments.
On 2022/4/8 5:39, Bob Pearson wrote:
> On 3/31/22 07:02, Xiao Yang wrote:
>> Current rxe_requester() doesn't generate a completion on error after
>> getting a wqe. Fix the issue by calling rxe_completer() on error.
>>
>> Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
>> ---
>> drivers/infiniband/sw/rxe/rxe_req.c | 24 ++++++++++++------------
>> 1 file changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/infiniband/sw/rxe/rxe_req.c b/drivers/infiniband/sw/rxe/rxe_req.c
>> index ae5fbc79dd5c..01ae400e5481 100644
>> --- a/drivers/infiniband/sw/rxe/rxe_req.c
>> +++ b/drivers/infiniband/sw/rxe/rxe_req.c
>> @@ -648,26 +648,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_op_err;
>> }
> This isn't an error. What is happening is the requester has advanced too far into the
> work request queue compared to what has been completed. So it sets a flag and exits the
> loop. When the completer finishes another completion it will reschedule the requester.
> RXE_MAX_UNACKED_PSNS is 128 so only 128 packets are allowed in the network without
> seeing an ack.
Got it.
>>
>> /* 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_op_err;
>> }
> This also is not an error. Here there is a limit on the number SKBs in flight.
> The SKBs are consumed by the ethernet driver when the packet is put on the wire.
> This prevents the driver from using too much memory when the NIC is paused.
Got it.
>>
>> opcode = next_opcode(qp, wqe, wqe->wr.opcode);
>> - if (unlikely(opcode < 0)) {
>> - wqe->status = IB_WC_LOC_QP_OP_ERR;
>> - goto exit;
>> - }
> This really is an error, but one that shouldn't happen under normal
> circumstances since it implies an illegal operation. Like attempting
> a write on a UD QP. Or causing an invalid opcode sequence.
Yes, this is the issue I try to resolve. Current rxe_requester() doesn't
generate a completion when processing an unsupported/invalid opcode.
I am trying to introduce new RDMA Atomic Write opcode recently. If rxe
driver doesn't support a new opcode (e.g. RDMA Atomic Write) and RDMA
library supports it, an application using the new opcode can reproduce
the issue.
>> + if (unlikely(opcode < 0))
>> + goto qp_op_err;
>>
>> mask = rxe_opcode[opcode].mask;
>> if (unlikely(mask & RXE_READ_OR_ATOMIC_MASK)) {
>> if (check_init_depth(qp, wqe))
>> - goto exit;
>> + goto qp_op_err;
>> }
> This isn't an error. It means that someone posted a read/atomic operation
> and there are too many pending read/atomic operations pending. You just need
> to wait for one of them to complete so it is another pause.
Got it.
>>
>> mtu = get_mtu(qp);
>> @@ -706,26 +704,26 @@ int rxe_requester(void *arg)
>> 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;
>> }
> This is an error. It could happen if the address handle referred to by
> the WR is not longer valid. There should always be an address vector but
> there may or may not be an address handle if the AV comes from a connected
> QP or not.
Yes, I think the original logic is enough.
>>
>> 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;
>> }
>>
>> 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 (ah)
>> + rxe_put(ah);
>> 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;
>> + goto err;
>> }
> Not sure the point of this.
Just keep the original logic.
>>
>> if (ah)
>> @@ -751,8 +749,7 @@ int rxe_requester(void *arg)
>> goto exit;
>> }
>>
>> - wqe->status = IB_WC_LOC_QP_OP_ERR;
>> - goto err;
>> + goto qp_op_err;
>> }
>>
>> update_state(qp, wqe, &pkt);
>> @@ -762,6 +759,9 @@ int rxe_requester(void *arg)
>> err_drop_ah:
>> if (ah)
>> rxe_put(ah);
>> +
>> +qp_op_err:
>> + wqe->status = IB_WC_LOC_QP_OP_ERR;
>> err:
>> wqe->state = wqe_state_error;
>> __rxe_do_task(&qp->comp.task);
>
> Near as I can tell all you did was turn a few pauses into errors which is wrong
> and otherwise move things around. What were you trying to solve here?
I will resend a new patch to only fix the unsupported/invalid issue.
Best Regards,
Xiao Yang
>
> Bob
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-04-08 3:15 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-31 12:02 [PATCH v3] RDMA/rxe: Generate a completion on error after getting a wqe Xiao Yang
2022-03-31 17:19 ` Leon Romanovsky
[not found] ` <8b90851c-72ba-1eb2-7e94-ee7d6a178fc7@fujitsu.com>
2022-04-01 4:50 ` Leon Romanovsky
2022-04-01 5:09 ` yangx.jy
2022-04-07 21:39 ` Bob Pearson
2022-04-08 3:14 ` yangx.jy
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.