Test tree patch inventory - update

Em Sat, Nov 17, 2007 at 03:42:52PM +0000, Gerrit Renker escreveu:

Applied.

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.

Ah, and there is more space to save, after this patch the layout is this, on x86_64:

struct ccid3_hc_rx_sock {
struct tfrc_rx_info ccid3hcrx_tfrc; /* 0 12 */

/* XXX 4 bytes hole, try to pack */

u64 ccid3hcrx_seqno_nonloss:48; /* 16 8 */
u64 ccid3hcrx_ccval_nonloss:4; /* 16 8 */
u64 ccid3hcrx_ccval_last_counter:4; /* 16 8 */
enum ccid3_hc_rx_states ccid3hcrx_state:8; /* 20 4 */
u32 ccid3hcrx_bytes_recv; /* 24 4 */

/* XXX 4 bytes hole, try to pack */

ktime_t ccid3hcrx_tstamp_last_feedback; /* 32 8 */
ktime_t ccid3hcrx_tstamp_last_ack; /* 40 8 */
struct list_head ccid3hcrx_hist; /* 48 16 */
/* --- cacheline 1 boundary (64 bytes) --- */
struct list_head ccid3hcrx_li_hist; /* 64 16 */
u16 ccid3hcrx_s; /* 80 2 */

/* XXX 2 bytes hole, try to pack */

u32 ccid3hcrx_pinv; /* 84 4 */

/* size: 88, cachelines: 2 */
/* sum members: 78, holes: 3, sum holes: 10 */ /* last cacheline: 24 bytes */
};

Just moving ccid3hcrx_bytes_recv to after ccid3hcrx_tfrc we can save 8 bytes.

Applied.

Applied

Why do we need DECLARE_TFRC_TX_CACHE?

+static struct kmem_cache *tfrcxh;

Why "xh"?

Will try to continue later today.

- Arnaldo

Em Sat, Nov 17, 2007 at 03:42:52PM +0000, Gerrit Renker escreveu:

I dropped this one, see next explanation.

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.

I'll combine this with the next one.

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

| >                         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

Em Wed, Nov 21, 2007 at 11:52:55AM +0000, Gerrit Renker escreveu:

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.

tfrc_rx_hist would be better, its not overly long and for the casual reader provides more info.

| > | > 	[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.

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