Code review comment for lp:~martin-lp/hipl/hipl_retransmissions

Hi,

On 12/29/2011 01:01 PM, David Martin wrote:
> Hi,
>
> On Thu, Dec 22, 2011 at 11:45 AM, Miika Komu<email address hidden>
> wrote:
>> On 21/12/11 12:46, David Martin wrote:
>>> On Tue, Dec 20, 2011 at 6:50 PM, Miika Komu<email address hidden>
>>> wrote:
>
>>>> 3. Why to buffer to three messages?
>>>
>>>
>>> It's more than one to retransmit the locators during UPDATEs.
>>> And three is more or less arbitrarily chosen by me. Two did not
>>> seem worth the effort of looping through it and four seems not
>>> needed? How many locators are common when using HIPL? On my test
>>> setup it's always only two... This can easily be changed by
>>> changing HIP_RETRANSMIT_QUEUE_SIZE though.
>>
>>
>> maybe I failed to understand something or should dig into the code
>> better. Are you saying that a buffered packet is a) retransmitted
>> three times or b) three packets can be queued?
>>
>> Option a) makes sense, option b) doesn't make sense because the
>> retransmitted packets are identical (in most cases, see below).
>
> We are doing option b) in this branch. Well, are they identical? The
> destination of the buffered U2 packets differ because of the locators
> so this is not true, no?

the retransmitted packages are different only in exceptional
circumstances. Similarly, the destination is usually the same because
there is one retransmission queue per host association - right?

Note: the destination can actually change with the shotgun extension
because it sends an UPDATE to all known peer addresses.

>>>> Regarding to buffering, the state may change during
>>>> retransmission and this causes the implementation to resend old
>>>> messages in addition to new ones? If we're lucky, the peer will
>>>> ignore UPDATE packets with old SPI numbers. However, the
>>>> situation may be different in the BEX because there's no
>>>> state.
>>>
>>>
>>> I'm not sure whether I fully get your question. You mean for
>>> example when you trigger three UPDATEs in a row you would have
>>> three retransmissions? Once one of the UPDATEs has been processed
>>> the peer would ignore the older retransmissions
>>> (check_update_freshness() would fail) and on our side the
>>> buffered retransmissions would be removed after receiving the U2
>>> from the peer.
>>
>>
>> Regarding to option b), the packets can be different e.g. with
>> UPDATEs. It occurs that a host obtains new locators every now and
>> then. If the host has a queue of length of more than two, then it
>> keeps sending an old buffered set of locators in addition to the
>> new set. While this is not a big issue, it's completely unnecessary
>> and should be avoided IMHO. The easiest way to avoid this is to
>> have queue of size one.
>
> Setting the queue to size to one would completely defeat its point
> though.

What is the point of having a queue size of more than one actually? Or
is that that the buffer size determines the number of retransmissions?

> How often does such a locator change happen?

Seldom by default because hipd reacts to locator changes with an
intended delay to accumulate the changes.

> I guess in
> theory it may happen that an UPDATE is triggered and lost, the host
> obtains a new locator and another UPDATE is triggered which is lost
> again. Then you would actually retransmit both UPDATEs with different
> locators. A solution may be to remove older buffered U1 packets when
> a new UPDATE is triggered. I may change that if necessary or
> desired.
 >
>>> Can you trigger another BEX during BEX? Can I force hipd to send
>>> another I1 when I'm in state R1_SENT for example? I did not test
>>> this.
>>
>>
>> No.
>
> Ok, so we do not have a problem here.
>
>
>>>> A special (and supported) use case from base exchange: try to
>>>> trigger the BEX simultaneously from both sides (i.e. break
>>>> point both sides with gdb just after sending I1). I know the
>>>> state machine can handle this but what happens to the
>>>> retransmissions?
>>>
>>>
>>> Actually I did try this and when I let two machines ping each
>>> other virtually at the same time the BEX sort of gets stuck and
>>> neither side can get any traffic through (this is not specific to
>>> this branch as it is the same on trunk).
>>
>>
>> Please open a bug report on this and describe the state on both
>> sides. It appears somebody has been removing code that shouldn't
>> have been removed. The general idea is that the host with the
>> "larger" HIT (see hip_hit_is_bigger()) should transmit an R1. See
>> table 3 in RFC5201.
>
> It's on my todo list now. :)