Re: [PATCH] IB/ipoib: Skip napi_schedule if ib_poll_cq fails

On 7/13/2016 12:50 PM, Yuval Shaia wrote:

On Wed, Jul 13, 2016 at 01:25:04PM -0600, Jason Gunthorpe wrote:

quoted

On Wed, Jul 13, 2016 at 10:12:25PM +0300, Yuval Shaia wrote:

quoted

On Wed, Jul 13, 2016 at 11:47:42AM -0600, Jason Gunthorpe wrote:

quoted

On Wed, Jul 13, 2016 at 02:33:56AM -0700, Yuval Shaia wrote:

quoted

To avoid entering into endless loop when device can't poll CQE from CQ
driver should not reschedule if error is not -EAGAIN.

?? what causes ib_poll_cq to return an error?

You need to describe the motivation here.

EAGAIN is fine - HW driver returns this to indicates temporary error and
caller should retry again.
However, other errors (such as EINVAL) may refer to some fatal error where
HW driver is unable to recover from.

So you've never seen this?

Waiting for real use case might take some time.

quoted

I question the sanity of a poll_cq implementation that can return a
hard error...

quoted

Two examples:
- Mellanox folks may comment for example if the case where
  __mlx4_qp_lookup() returns NULL in function mlx4_ib_poll_one() means
  fatal or not.
- At least by reading the of c4iw_poll_cq_one() it is clear that it may
  return fatal error.

If EAGAIN should be ignored, then all other errors indicate the CQ is
dead and needs to be reconstructed. So the approach in this patch to
add a 'recv_conseq_cq_errs' is nonsense. You need to trigger some kind
of restart of the QP instead.

The idea behind consec counter is not a recovery mechanism it is just some
why to "retry" for a while just before declaring game over. I do not have
strong opinion on that, actually my first try was w/o it, i.e. 'kill' on
the first error.

Patch does not offer any recovery mechanism it simply print fatal error to
console and exit NAPI. This fatal error will suggest admin to reload the
driver or something like that.
This takes us to "recovery-mechanism" :)
I'm not sure that restarting the QP will help as the error is while reading
the CQ and restarting the CQ is more or less like restarting the driver.

Probably Jason mean destroy the problematic CQ and create a new one. This is what Haakon suggested as well but it will lead to the leak
and also possible issue with outstanding WC's getting lost without
being flushed on that CQ.


--
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