On 6/7/2021 3:16 AM, Zhu Yanjun wrote:
On Sat, Jun 5, 2021 at 7:07 AM Bob Pearson <rpearsonhpe@xxxxxxxxx> wrote:
Currently the rdma_rxe driver attempts to protect atomic responder
resources by taking a reference to the qp which is only freed when the
resource is recycled for a new read or atomic operation. This means that
in normal circumstances there is almost always an extra qp reference
once an atomic operation has been executed which prevents cleaning up
the qp and associated pd and cqs when the qp is destroyed.
This patch removes the call to rxe_add_ref() in send_atomic_ack() and the
call to rxe_drop_ref() in free_rd_atomic_resource(). If the qp is
Not sure if it is a good way to fix this problem by removing the call
to rxe_add_ref.
Because taking a reference to the qp is to protect atomic responder resources.
There is no good way to 'protect' responder resources. They are there to
recover from a lost response packet which can occur multiple times. The
peer can retry the atomic request an unpredictable number of times.
There are no acks for response packets. So the QP just has to leave it
in place until the max number of read/atomic requests has been received
and then it re-uses the responder resource. So to 'protect' the
responder resource means you have to leave the QP around potentially
forever which is the root cause of the bug. In fact you should treat a
retry of a read/atomic the same as a new request and when the QP is
destroyed it stops responding to new requests.
There is a reference to the QP taken by a received packet which lasts
until the packet is freed to prevent kernel seg faults if the QP is
destroyed while a request is in process of being responded to.
Removing rxe_add_ref is to decrease the protection of the atomic
responder resources.
Zhu Yanjun
destroyed while a peer is retrying an atomic op it will cause the
operation to fail which is acceptable.
Reported-by: Zhu Yanjun <zyjzyj2000@xxxxxxxxx>
Fixes: 86af61764151 ("IB/rxe: remove unnecessary skb_clone")
Signed-off-by: Bob Pearson <rpearsonhpe@xxxxxxxxx>
---
drivers/infiniband/sw/rxe/rxe_qp.c | 1 -
drivers/infiniband/sw/rxe/rxe_resp.c | 2 --
2 files changed, 3 deletions(-)
diff --git a/drivers/infiniband/sw/rxe/rxe_qp.c b/drivers/infiniband/sw/rxe/rxe_qp.c
index 34ae957a315c..b6d83d82e4f9 100644
--- a/drivers/infiniband/sw/rxe/rxe_qp.c
+++ b/drivers/infiniband/sw/rxe/rxe_qp.c
@@ -136,7 +136,6 @@ static void free_rd_atomic_resources(struct rxe_qp *qp)
void free_rd_atomic_resource(struct rxe_qp *qp, struct resp_res *res)
{
if (res->type == RXE_ATOMIC_MASK) {
- rxe_drop_ref(qp);
kfree_skb(res->atomic.skb);
} else if (res->type == RXE_READ_MASK) {
if (res->read.mr)
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c b/drivers/infiniband/sw/rxe/rxe_resp.c
index 2b220659bddb..39dc39be586e 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -966,8 +966,6 @@ static int send_atomic_ack(struct rxe_qp *qp, struct rxe_pkt_info *pkt,
goto out;
}
- rxe_add_ref(qp);
-
res = &qp->resp.resources[qp->resp.res_head];
free_rd_atomic_resource(qp, res);
rxe_advance_resp_resource(qp);
--
2.30.2
