Re: [PATCH v4 09/16] IB/pvrdma: Add support for Completion Queues

On Sun, Sep 18, 2016 at 08:36:55PM +0000, Adit Ranadive wrote:

On Sun, Sep 18, 2016 at 10:07:18 -0700, Leon Romanovsky wrote:

quoted

On Thu, Sep 15, 2016 at 10:36:12AM +0300, Yuval Shaia wrote:

quoted

Hi Adit,
Please see my comments inline.

Besides that I have no more comment for this patch.

Reviewed-by: Yuval Shaia <redacted>

Yuval

On Thu, Sep 15, 2016 at 12:07:29AM +0000, Adit Ranadive wrote:

quoted

On Wed, Sep 14, 2016 at 05:43:37 -0700, Yuval Shaia wrote:

quoted

On Sun, Sep 11, 2016 at 09:49:19PM -0700, Adit Ranadive wrote:

quoted

+
+static int pvrdma_poll_one(struct pvrdma_cq *cq, struct pvrdma_qp

**cur_qp,

quoted

+			   struct ib_wc *wc)
+{
+	struct pvrdma_dev *dev = to_vdev(cq->ibcq.device);
+	int has_data;
+	unsigned int head;
+	bool tried = false;
+	struct pvrdma_cqe *cqe;
+
+retry:
+	has_data = pvrdma_idx_ring_has_data(&cq->ring_state->rx,
+					    cq->ibcq.cqe, &head);
+	if (has_data == 0) {
+		if (tried)
+			return -EAGAIN;
+
+		/* Pass down POLL to give physical HCA a chance to poll. */
+		pvrdma_write_uar_cq(dev, cq->cq_handle |

PVRDMA_UAR_CQ_POLL);

quoted

+
+		tried = true;
+		goto retry;
+	} else if (has_data == PVRDMA_INVALID_IDX) {

I didn't went throw the entire life cycle of RX-ring's head and tail but you
need to make sure that PVRDMA_INVALID_IDX error is recoverable one, i.e
there is probability that in the next call to pvrdma_poll_one it will be fine.
Otherwise it is an endless loop.

We have never run into this issue internally but I don't think we can recover here

I briefly reviewed the life cycle of RX-ring's head and tail and didn't
caught any suspicious place that might corrupt it.
So glad to see that you never encountered this case.

quoted

in the driver. The only way to recover would be to destroy and recreate the CQ
which we shouldn't do since it could be used by multiple QPs.

Agree.
But don't they hit the same problem too?

quoted

We don't have a way yet to recover in the device. Once we add that this check
should go away.

To be honest i have no idea how to do that - i was expecting driver's vendors
to come up with an ideas :)
I once came up with an idea to force restart of the driver but it was
rejected.

quoted

The reason I returned an error value from poll_cq in v3 was to break the possible
loop so that it might give clients a chance to recover. But since poll_cq is not expected
to fail I just log the device error here. I can revert to that version if you want to break
the possible loop.

Clients (ULPs) cannot recover from this case. They even do not check the
reason of the error and treats any error as -EAGAIN.

It is because poll_one is not expected to fall.

Poll_one is an internal function in our driver. ULPs should still be okay I think as long as poll_cq
does not fail, no?

Yes, I think so.

Thanks