[PATCH rdma-core 2/2] srp_daemon: fix CQ handling

[PATCH rdma-core 2/2] srp_daemon: fix CQ handling

[Nicolas Morey-Chaisemartin]

SM traps are polled through poll_cq which waited for a CQ event
before polling the CQ itself.
However it may happens that multiple completions are attached
to a single event. As stated by the ibv_get_cq_event man page,
it is required to poll the the CQ to get those completions
after the call to ibv_req_notify_cq.

As completions need to be handled one by one in an outer function,
start by polling the CQ and return the completion (if any) before
waiting for the next completion event.
This will allow emptying all pending completions, through multiple calls
to poll_cq,  before waiting for a new event.

The buggy use case seems to appear when the master SM is switched multiple
times between two nodes. As the number of ping-pong between the SMs increases,
the number of traps sent to notify that the SM just became master increases
too. This causes burst of completions linked to a single event.
Note that the race condition is also possible in other scenario.

Signed-off-by: Nicolas Morey-Chaisemartin <NMoreyChaisemartin-IBi9RG/b67k@public.gmane.org>
Cc: stable-Xl5UnYtxxKxKUA01WzcqbQ@public.gmane.org # v14, v15, v16
---
 srp_daemon/srp_handle_traps.c | 42 ++++++++++++++++++++++++++++++------------
 1 file changed, 30 insertions(+), 12 deletions(-)

diff --git a/srp_daemon/srp_handle_traps.c b/srp_daemon/srp_handle_traps.c
index 25f2b9ab..647e6a5e 100644
--- a/srp_daemon/srp_handle_traps.c
+++ b/srp_daemon/srp_handle_traps.c
@@ -496,6 +496,27 @@ static int stop_threads(struct sync_resources *sync_res)
 	return result;
 }
 
+static int poll_cq_once(struct sync_resources *sync_res, struct ibv_cq *cq,
+			struct ibv_wc *wc)
+{
+	int ret;
+	ret = ibv_poll_cq(cq, 1, wc);
+	if (ret < 0) {
+		pr_err("poll CQ failed\n");
+		return ret;
+	}
+
+	if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
+		if (!stop_threads(sync_res))
+			pr_err("got bad completion with status: 0x%x\n",
+			       wc->status);
+		return -ret;
+	}
+
+	return ret;
+}
+
+
 static int poll_cq(struct sync_resources *sync_res, struct ibv_cq *cq,
 		   struct ibv_wc *wc, struct ibv_comp_channel *channel)
 {
@@ -504,6 +525,14 @@ static int poll_cq(struct sync_resources *sync_res, struct ibv_cq *cq,
 	void          *ev_ctx;
 
 	if (channel) {
+		/* There may be extra completions that
+		 * were associated to the previous event.
+		 * Only poll for the first one. If there are more than one,
+		 * they will be handled by later call to poll_cq */
+		ret = poll_cq_once(sync_res, cq, wc);
+		if (ret)
+			return ret;
+
 		if (ibv_get_cq_event(channel, &ev_cq, &ev_ctx)) {
 			pr_err("Failed to get cq_event\n");
 			return -1;
@@ -524,18 +553,7 @@ static int poll_cq(struct sync_resources *sync_res, struct ibv_cq *cq,
 	}
 
 	do {
-		ret = ibv_poll_cq(cq, 1, wc);
-		if (ret < 0) {
-			pr_err("poll CQ failed\n");
-			return ret;
-		}
-
-		if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
-			if (!stop_threads(sync_res))
-				pr_err("got bad completion with status: 0x%x\n",
-				       wc->status);
-			return -ret;
-		}
+		ret = poll_cq_once(sync_res, cq, wc);
 
 		if (ret == 0 && channel) {
 			pr_err("Weird poll returned no cqe after CQ event\n");
-- 
2.15.1.272.g8e603414b

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 2/2] srp_daemon: fix CQ handling

[Bart Van Assche]

On Tue, 2017-12-12 at 15:08 +0100, Nicolas Morey-Chaisemartin wrote:
> +static int poll_cq_once(struct sync_resources *sync_res, struct ibv_cq *cq,
> +			struct ibv_wc *wc)
> +{
> +	int ret;
> +	ret = ibv_poll_cq(cq, 1, wc);

Please leave one blank line between declarations and statements.

> +	if (ret < 0) {
> +		pr_err("poll CQ failed\n");
> +		return ret;
> +	}
> +
> +	if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
> +		if (!stop_threads(sync_res))
> +			pr_err("got bad completion with status: 0x%x\n",
> +			       wc->status);
> +		return -ret;
> +	}
> +
> +	return ret;
> +}

This function can return negative values, positive values or zero. Please
modify it such that either 0 or a negative Unix error code is returned. This
will make it easier to read this function and also the code that calls this
function.

Thanks,

Bart.

Re: [PATCH rdma-core 2/2] srp_daemon: fix CQ handling

[Nicolas Morey-Chaisemartin]

Le 12/12/2017 à 18:04, Bart Van Assche a écrit :
> On Tue, 2017-12-12 at 15:08 +0100, Nicolas Morey-Chaisemartin wrote:
>> +static int poll_cq_once(struct sync_resources *sync_res, struct ibv_cq *cq,
>> +			struct ibv_wc *wc)
>> +{
>> +	int ret;
>> +	ret = ibv_poll_cq(cq, 1, wc);
> Please leave one blank line between declarations and statements.

OK

>
>> +	if (ret < 0) {
>> +		pr_err("poll CQ failed\n");
>> +		return ret;
>> +	}
>> +
>> +	if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
>> +		if (!stop_threads(sync_res))
>> +			pr_err("got bad completion with status: 0x%x\n",
>> +			       wc->status);
>> +		return -ret;
>> +	}
>> +
>> +	return ret;
>> +}
> This function can return negative values, positive values or zero. Please
> modify it such that either 0 or a negative Unix error code is returned. This
> will make it easier to read this function and also the code that calls this
> function.

We still need to differenciate between OK with 0 completions polled, and OK with 1 completion.
I did it this way to limit the side effect of factoring the code.
Would you want poll_cq_once to return -EAGAIN  when no completion was found?

I just realized the code is broken anyway.
if poll_cq_once returns an error in the second poll_cq loop, it is not returned by poll_cq itself (simply return 0).


Nicolas
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-core 2/2] srp_daemon: fix CQ handling

[Bart Van Assche]

On Tue, 2017-12-12 at 18:28 +0100, Nicolas Morey-Chaisemartin wrote:
> Le 12/12/2017 à 18:04, Bart Van Assche a écrit :
> > On Tue, 2017-12-12 at 15:08 +0100, Nicolas Morey-Chaisemartin wrote:
> > > +	if (ret < 0) {
> > > +		pr_err("poll CQ failed\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	if (ret > 0 && wc->status != IBV_WC_SUCCESS) {
> > > +		if (!stop_threads(sync_res))
> > > +			pr_err("got bad completion with status: 0x%x\n",
> > > +			       wc->status);
> > > +		return -ret;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > 
> > This function can return negative values, positive values or zero. Please
> > modify it such that either 0 or a negative Unix error code is returned. This
> > will make it easier to read this function and also the code that calls this
> > function.
> 
> We still need to differenciate between OK with 0 completions polled, and OK with 1 completion.
> I did it this way to limit the side effect of factoring the code.
> Would you want poll_cq_once to return -EAGAIN  when no completion was found?

Hello Nicolas,

If you think another approach would work better for the return values than
what I proposed it's fine to use that approach. My primary concern is that
the meaning of the return values gets documented and also that the meaning
of the return values is clear without having to read the implementation of
this function.

Bart.