Re: timeout.c in bsd_tcpip

[Motoya Kurotsu <kurotsu at allied-telesis dot co dot jp>]

Hi Andrew;

On Fri, May 09, 2003 at 09:41:25AM +0200, Andrew Lunn wrote:
> 
> I get it now. _timeout is the array which is used for storage of the
> timeouts. timeout is a pointer and is the head of the linked list.
> 
> do_timeout() currently walks the linked list starting from the first
> entry in the array. This is not necessarily the head of the linked
> list, eg the first entry may of expired. In this case, the subsequent
> entries are never looked at because the next pointer is NULL. Thats
> what the first hunk of the patch changes. It now walked from the head
> of the linked list.
> 
> timeout() is responsible for adding new timeouts to the linked
> list. It should be looking at the array for empty slots. So using
> _timeout is correct. What is wrong is how it moves from slot to
> slot. The old code tries to walk a linked list. This is wrong. There
> is no linked list of empty slots. The second hunk changes this so that
> it simply tries each entry in the array in turn until it finds one
> which is not in use, or the end of the array is reached. 
> 
> The third hunk changes untimeout(). This removes a timeout. Again the
> old code walked a linked list starting from the first entry in the
> array and not the head of the list. You changed it to look at all
> elements in the array one by one. This will work, but maybe its more
> efficient to walk the linked list, but start from the head, ie
> timeout, not _timeout. What do you think?

I don't agree. I think that the old untimeout() is not only inefficient
but also wrong. 
First, the link list headed by timeout may link many tcp timers
such as tt_rexmt, tt_persist, tt_keep, tt_2msl and tt_delack.
These timers are linked on the list by calling callout_reset() directly, 
not calling timeout() since they don't need to allocate the entry space 
for it. Actually the spaces for these timers are allocated 
(or point to the allocated space) in tcp_newtcpcb(). Likewise these 
timers are removed from the link list by calling callout_stop()
directly. On the other hand, untimeout() is used to remove the timer entry 
which was allocated by timeout() previously. 
Second, the old untimeout() change the link by calling callout_stop() 
at the same time when it is tracing the link itself.
So I think that to look at all elements in the array(_timeout) is 
not only efficient but also right.

> 
> Apart from the minor point for the last hunk, im now happy with this
> patch. I've not actually run it, but i know understand it.
> 
> Garry, what do you think? This is yours and Hugo's code. Ok to commit?

Motoya Kurotsu
Allied Telesis K.K.