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

[Santosh Shilimkar <santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>]

On 7/13/2016 10:50 PM, Yuval Shaia wrote:
> On Wed, Jul 13, 2016 at 01:46:07PM -0700, Santosh Shilimkar wrote:
>>
>> On 7/13/2016 12:50 PM, Yuval Shaia wrote:
>>> On Wed, Jul 13, 2016 at 01:25:04PM -0600, Jason Gunthorpe wrote:
>>>> On Wed, Jul 13, 2016 at 10:12:25PM +0300, Yuval Shaia wrote:
>>>>> On Wed, Jul 13, 2016 at 11:47:42AM -0600, Jason Gunthorpe wrote:
>>>>>> On Wed, Jul 13, 2016 at 02:33:56AM -0700, Yuval Shaia wrote:
>>>>>>> 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.
>>>
>>>>
>>>> I question the sanity of a poll_cq implementation that can return a
>>>> hard error...
>>>>
>>>>> 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.
>
> It is not only leak.
> This CQ serve many QP (== many connections). Destroying it seems to me
> catastrophic as reloading the driver.
>
Fair enough.
--
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