Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries

From: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
To: Erez Shitrit <erezsh-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
Cc: Or Gerlitz <gerlitz.or-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>,
	Erez Shitrit <erezsh-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>,
	"linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org"
	<linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
Subject: Re: [PATCH rdma-rc 2/7] IB/IPoIB: Don't update neigh validity for unresolved entries
Date: Tue, 7 Jun 2016 10:50:54 -0400	[thread overview]
Message-ID: <99eb8d42-95f2-6899-b427-b6258db5e44b@redhat.com> (raw)
In-Reply-To: <5fc6bf69-ff7e-d94f-fbfe-46a42ee1e22d-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

[-- Attachment #1.1: Type: text/plain, Size: 2755 bytes --]

On 6/7/2016 10:43 AM, Doug Ledford wrote:
> On 6/7/2016 10:32 AM, Erez Shitrit wrote:
>> On Tue, Jun 7, 2016 at 4:48 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>> On 6/7/2016 2:01 AM, Erez Shitrit wrote:
>>>> On Tue, Jun 7, 2016 at 2:59 AM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On 6/5/2016 5:10 PM, Or Gerlitz wrote:
>>>>>
>>>>>>> -                       neigh->alive = jiffies;
>>>>>>> +
>>>>>>> +                       if (likely(skb_queue_len(&neigh->queue) < IPOIB_MAX_PATH_REC_QUEUE))
>>>>>>> +                               neigh->alive = jiffies;
>>>>>>
>>>>>> why testing the queue len makes things right?
>>>>>
>>>>> I'm with Or on this one.  This test doesn't make sense unless you assume
>>>>> that the neighbor is being hit with a steady stream of packets.  If
>>>>> someone just runs ping -c 1 to a dead neighbor, this test will fail.
>>>>> The idea of the patch is OK, but the test needs to be reworked for
>>>>> situations other than a blasting stream of data.
>>>>>
>>>>
>>>> I will try to explain the idea behind that test, and why it works (I
>>>> hope I'll make it clear this time :))
>>>>
>>>> there are 2 objects that are taking place here, the garbage-collector
>>>> that removes neigh if was not used for defined time, and the
>>>> path-query for each neigh.
>>>> if the path-query failed the packets for specific neigh are kept in
>>>> the neigh queue, but only if there are no more than
>>>> IPOIB_MAX_PATH_REC_QUEUE, and the neigh is still kept for ever (let's
>>>> assume CM connected that was stopped without any notification with
>>>> DREQ etc.)
>>>> The only way to know that there is an unresolved neigh is by checking
>>>> its queue, if it is full we can assume that this is an unresolved
>>>> neigh otherwise it is resolved.
>>>
>>> That's not true.  Reread what I wrote above (I was specific when I
>>> picked that command).  Ping -c 1 will only send one ping.  It will not
>>> trip this test.  As I said, your test relies on a stream of packets, a
>>> single packet to a gone neighbor will never cause it to get deleted.
>>
>> If you ping one time (ping -c 1) there is no problem at all, the neigh
>> will be deleted by the GC anyway, because no (unresolved) packet
>> updates the neigh validity and the GC will check the last time it was
>> refreshed and since only one ping refreshed it long ago, it will be
>> deleted. (the GC always running)
>>
>> the problem is when there are non stop traffic to that neigh. IMHO
>> that test works for that.
> 
> Ok, that makes sense.

Sorry, in case it wasn't clear, I grabbed this patch now (although I
reworded the commit message significantly).

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]