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

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?

>>> 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. How often does such a locator change happen? 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. :)