* Re: Test tree patch inventory - update
2007-11-20 20:10 Test tree patch inventory - update Arnaldo Carvalho de Melo
@ 2007-11-21 0:21 ` Arnaldo Carvalho de Melo
2007-11-21 11:52 ` Gerrit Renker
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2007-11-21 0:21 UTC (permalink / raw)
To: dccp
Em Sat, Nov 17, 2007 at 03:42:52PM +0000, Gerrit Renker escreveu:
> Test Tree Inventory
> =========
> 4. Support for passive-close without flushing unread data
> ---------------------------------------------------------
> This set actually involves only 5 patches, the last one is according to your suggestion.
>
> [DCCP]: Separate protocol states into general/specific
I dropped this one, see next explanation.
> [DCCP]: Make PARTOPEN an autonomous state
Here I made DCCP_PARTOPEN = TCP_MAX_STATES, that DCCP_INTRINSECS was not
used anywhere and this just means that states >= TCP_MAX_STATES are
DCCP only states.
> [DCCP]: Dedicated auxiliary states to support passive-close
I'll combine this with the next one.
> [DCCP]: Basic support for passive-close
What happens if we receive a second CloseReq or a second Close packet or
if the server performed an active close?
In all these cases we're leaking an skb, no? As dccp_rcv_close{req}
before always used dccp_fin there wasn't a need for returning a value
from these functions, but now we better return if we used the skb or
not, so that when back to __dccp_rcv_established and
dccp_rcv_state_process we can discard the packet.
- Arnaldo
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Test tree patch inventory - update
2007-11-20 20:10 Test tree patch inventory - update Arnaldo Carvalho de Melo
2007-11-21 0:21 ` Arnaldo Carvalho de Melo
@ 2007-11-21 11:52 ` Gerrit Renker
2007-11-21 12:46 ` Arnaldo Carvalho de Melo
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Gerrit Renker @ 2007-11-21 11:52 UTC (permalink / raw)
To: dccp
| > Test Tree Inventory
| > =========| >
| > [CCID3]: Ignore trivial amounts of elapsed time
|
| Applied, but I think we can revisit elapsed time later by taking a
| timestamp earlier, at dccp_v4_rcv, because then there can be time spent
| in sk_backlog, etc. We could then store a timestamp on skb->cb and the
| ccids would set a flag on dccp_sock to tell that the timestamp is
| wanted.
|
The elapsed time is needed for RTT measurements and you are right, the
hole RTT measurement issue needs more work. And this includes CCID2
which goes into the other extreme and uses a coarse-grained HZ
resolution of RTTs. But ever since there was the issue with interface
timestamps (skb_get_timestamp) I got convinced that it is not useful
to increase the resolution below, say, 1 millisecond. (There was a
discussion with Ian about above patch where this came up.) The reason
is that when we do such fine-grained resolution, we also get a lot of
noise: much of the TCP code works very well with just the jiffies
resolution.
With regard to your point, I think it would be ideal to use timestamps
in the TCP High-Speed way (RFC 1323), where we could use a resolution of
0.00001 seconds (dccp_timestamp() and RFC 4340, 13.). That is still
very high-resolution when considering that the RFC2988 remarks regarding
clock granularity in part referred back to that old BSD timer which had
a 0.5 second granularity.
I agree fully with your suggestion: all CCID-specific RTT measurement
code can be eliminated once there is a working alternative. The
challenge that needs to be overcome is to make the timestamp usage robust
against packet duplication and re-ordering.
| Just moving ccid3hcrx_bytes_recv to after ccid3hcrx_tfrc we can save 8
| bytes.
This is good news - with regard to the above I still have in the back of
my mind that there are unused icsk fields which may also be used for
storing timestamps; this could help (in a future effort) to reduce further.
| > 2. Main CCID3 patch set
| > -----------------------
| > This is the original CCID3 patch set, developed in Feb/Mar, significantly reordered to
| > make the test tree fully bisectable.
| > The patch set does three things (reflected in the order of batches):
| >
| > (1) new TX history for TFRC (packet_history.{c,h}),
|
| Why do we need DECLARE_TFRC_TX_CACHE?
|
It is not needed for compilation. When I first did this patch set there were three different
kmem_caches defined in ccid3.c:
* one for the TX history (packet_history.c; for which the above macro was declared),
* one for the RX history (packet_history.c)
* one for the Loss History (loss_interval.c)
The loss interval history has since gone, this was done when you worked on Ian's patch set
around March/April. When the patch set is fully applied, the RX history cache also goes,
since the new patch set uses at most 4 entries per socket which are supplied by a static
kmem_cache in packet_history.c.
Long story, short conclusion: if you think this is an eyesore, feel free to take it out. I
left it in since I thought it would make the purpose clearer (since ctags then automatically
jumps to packet_history.h).
| > [TFRC]: New RX history implementation
|
|
| +static struct kmem_cache *tfrcxh;
|
| Why "xh"?
|
This is a typo, my mistake. I had meant to use the suffix `rxh' to indicate that
this static cache is for the RX history, but it somehow merged with `r'
from tfrc. Are you okay with tfrc_rxh? I will uploaded a revised patch
which uses this identifier instead. Thank you for pointing this out.
Gerrit
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Test tree patch inventory - update
2007-11-20 20:10 Test tree patch inventory - update Arnaldo Carvalho de Melo
2007-11-21 0:21 ` Arnaldo Carvalho de Melo
2007-11-21 11:52 ` Gerrit Renker
@ 2007-11-21 12:46 ` Arnaldo Carvalho de Melo
2007-11-21 13:18 ` Gerrit Renker
2007-11-22 10:29 ` Gerrit Renker
4 siblings, 0 replies; 6+ messages in thread
From: Arnaldo Carvalho de Melo @ 2007-11-21 12:46 UTC (permalink / raw)
To: dccp
Em Wed, Nov 21, 2007 at 11:52:55AM +0000, Gerrit Renker escreveu:
> | > Test Tree Inventory
> | > =========> | >
> | > [CCID3]: Ignore trivial amounts of elapsed time
> |
> | Applied, but I think we can revisit elapsed time later by taking a
> | timestamp earlier, at dccp_v4_rcv, because then there can be time spent
> | in sk_backlog, etc. We could then store a timestamp on skb->cb and the
> | ccids would set a flag on dccp_sock to tell that the timestamp is
> | wanted.
> |
> The elapsed time is needed for RTT measurements and you are right, the
> hole RTT measurement issue needs more work. And this includes CCID2
> which goes into the other extreme and uses a coarse-grained HZ
> resolution of RTTs. But ever since there was the issue with interface
> timestamps (skb_get_timestamp) I got convinced that it is not useful
> to increase the resolution below, say, 1 millisecond. (There was a
> discussion with Ian about above patch where this came up.) The reason
> is that when we do such fine-grained resolution, we also get a lot of
> noise: much of the TCP code works very well with just the jiffies
> resolution.
> With regard to your point, I think it would be ideal to use timestamps
> in the TCP High-Speed way (RFC 1323), where we could use a resolution of
> 0.00001 seconds (dccp_timestamp() and RFC 4340, 13.). That is still
> very high-resolution when considering that the RFC2988 remarks regarding
> clock granularity in part referred back to that old BSD timer which had
> a 0.5 second granularity.
> I agree fully with your suggestion: all CCID-specific RTT measurement
> code can be eliminated once there is a working alternative. The
> challenge that needs to be overcome is to make the timestamp usage robust
> against packet duplication and re-ordering.
I'll try at some point to measure the maximum time a packet stays on
sk_backlog on a heavily loaded server. But agreed, this is not urgent,
just to keep on the back of our minds for later.
> | Just moving ccid3hcrx_bytes_recv to after ccid3hcrx_tfrc we can save 8
> | bytes.
> This is good news - with regard to the above I still have in the back of
> my mind that there are unused icsk fields which may also be used for
> storing timestamps; this could help (in a future effort) to reduce further.
>
>
> | > 2. Main CCID3 patch set
> | > -----------------------
> | > This is the original CCID3 patch set, developed in Feb/Mar, significantly reordered to
> | > make the test tree fully bisectable.
> | > The patch set does three things (reflected in the order of batches):
> | >
> | > (1) new TX history for TFRC (packet_history.{c,h}),
> |
> | Why do we need DECLARE_TFRC_TX_CACHE?
> |
> It is not needed for compilation. When I first did this patch set there were three different
> kmem_caches defined in ccid3.c:
> * one for the TX history (packet_history.c; for which the above macro was declared),
> * one for the RX history (packet_history.c)
> * one for the Loss History (loss_interval.c)
> The loss interval history has since gone, this was done when you worked on Ian's patch set
> around March/April. When the patch set is fully applied, the RX history cache also goes,
> since the new patch set uses at most 4 entries per socket which are supplied by a static
> kmem_cache in packet_history.c.
> Long story, short conclusion: if you think this is an eyesore, feel free to take it out. I
> left it in since I thought it would make the purpose clearer (since ctags then automatically
> jumps to packet_history.h).
>
>
> | > [TFRC]: New RX history implementation
> |
> |
> | +static struct kmem_cache *tfrcxh;
> |
> | Why "xh"?
> |
> This is a typo, my mistake. I had meant to use the suffix `rxh' to indicate that
> this static cache is for the RX history, but it somehow merged with `r'
> from tfrc. Are you okay with tfrc_rxh? I will uploaded a revised patch
tfrc_rx_hist would be better, its not overly long and for the casual
reader provides more info.
> which uses this identifier instead. Thank you for pointing this out.
>
> Gerrit
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Test tree patch inventory - update
2007-11-20 20:10 Test tree patch inventory - update Arnaldo Carvalho de Melo
` (2 preceding siblings ...)
2007-11-21 12:46 ` Arnaldo Carvalho de Melo
@ 2007-11-21 13:18 ` Gerrit Renker
2007-11-22 10:29 ` Gerrit Renker
4 siblings, 0 replies; 6+ messages in thread
From: Gerrit Renker @ 2007-11-21 13:18 UTC (permalink / raw)
To: dccp
| > | > [TFRC]: New RX history implementation
| > |
| > |
| > | +static struct kmem_cache *tfrcxh;
| > |
| > | Why "xh"?
| > |
| > This is a typo, my mistake. I had meant to use the suffix `rxh' to indicate that
| > this static cache is for the RX history, but it somehow merged with `r'
| > from tfrc. Are you okay with tfrc_rxh? I will uploaded a revised patch
|
| tfrc_rx_hist would be better, its not overly long and for the casual
| reader provides more info.
|
I just tried this, the problem is that it clashes with
struct tfrc_rx_hist {
struct tfrc_rx_hist_entry *ring[NDUPACK + 1];
/* ... */
};
Thus changed it to
static struct kmem_cache *tfrc_rxh_cache;
and uploaded a revised patch. If you are not ok with the naming
scheme, let me know and I'll revise it.
I am working on the leaked skbs issue, either there will be a revised
patch later or I'll fix this after work.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: Test tree patch inventory - update
2007-11-20 20:10 Test tree patch inventory - update Arnaldo Carvalho de Melo
` (3 preceding siblings ...)
2007-11-21 13:18 ` Gerrit Renker
@ 2007-11-22 10:29 ` Gerrit Renker
4 siblings, 0 replies; 6+ messages in thread
From: Gerrit Renker @ 2007-11-22 10:29 UTC (permalink / raw)
To: dccp
Arnaldo,
| > [DCCP]: Basic support for passive-close
|
| What happens if we receive a second CloseReq or a second Close packet or
| if the server performed an active close?
|
Just to say I am still working on this. First, thanks a lot for pointing
out the bug. This forced me to look again at this patch and ... ecce,
there is another bug: PASSIVE_1/2 may only be entered from OPEN/PARTOPEN,
the code allowed to enter these states from other states, too. This is
clearly wrong.
Will send the revised patch once I am clearer what is going on, but this
may not be today.
Gerrit
^ permalink raw reply [flat|nested] 6+ messages in thread