Date:	Tue, 10 Nov 2009 08:41:05 -0500
From:	William Allen Simpson <william.allen.simpson@...il.com>
To:	Ilpo Järvinen <ilpo.jarvinen@...sinki.fi>
CC:	Linux Kernel Network Developers <netdev@...r.kernel.org>,
	Eric Dumazet <eric.dumazet@...il.com>,
	Joe Perches <joe@...ches.com>
Subject: Re: [net-next-2.6 PATCH v5 5/5 RFC] TCPCT part1e: initial SYN exchange
 with SYNACK data

Ilpo Järvinen wrote:
> On Mon, 9 Nov 2009, William Allen Simpson wrote:
>>...
>> Data structures are carefully composed to require minimal additions.
>> For example, the struct tcp_options_received cookie_plus variable fits
>> between existing 16-bit and 8-bit variables, requiring no additional
>> space (taking alignment into consideration).  There are no additions to
>> tcp_request_sock, and only 1 pointer in tcp_sock.
>>...
> 
> One general comment. ...This particular patch still has lots of noise 
> which does not belong to the context of this change. ...Please try to 
> minimize. Eg., if you don't like sizeof(struct tcphdr) but prefer 
> sizeof(*th), you certainly don't have to do it in this particular patch!

This *is* actually in CodingStyle (line 679), and I'm trying to conform:

<blockquote>
The preferred form for passing a size of a struct is the following:

	p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.
</blockquote>

Maybe some of these anticipate part 2, so I can defer them to later.  I've
already coded much of part 2, so things are bleeding back and forth.


> ...Also some comment changes which certainly are not mandatory nor even 
> related.
> 
Hmmm....

1) The first is a hole left by the removal of the data fields some time
ago, but they left the old (now incorrect) comment:

-/*	SACKs data	*/
+	u8	cookie_plus:6,	/* bytes in authenticator/cookie option	*/
+		cookie_out_never:1,
+		cookie_in_always:1;
  	u8	num_sacks;	/* Number of SACK blocks		*/

Although it's not evident from the diff -u, I'm *filling* that hole,
putting everything on a nice 32-bit boundary, thus taking *NO* space
(already mentioned in my patch description above):

	u16 	saw_tstamp : 1,	/* Saw TIMESTAMP on last packet		*/
		tstamp_ok : 1,	/* TIMESTAMP seen on SYN packet		*/
		dsack : 1,	/* D-SACK is scheduled			*/
		wscale_ok : 1,	/* Wscale seen on SYN packet		*/
		sack_ok : 4,	/* SACK seen on SYN packet		*/
		snd_wscale : 4,	/* Window scaling received from sender	*/
		rcv_wscale : 4;	/* Window scaling to send to receiver	*/
	u8	cookie_plus:6,	/* bytes in authenticator/cookie option	*/
		cookie_out_never:1,
		cookie_in_always:1;
	u8	num_sacks;	/* Number of SACK blocks		*/
	u16	user_mss;	/* mss requested by user in ioctl	*/
	u16	mss_clamp;	/* Maximal mss, negotiated at connection setup */

The only reason that it's done this way was Miller's imperative that
required cramming this into as small a space as possible.

   "Store the data either somewhere else or in an extremely compact form."

   "Make your state take up less space in tcp_sock without making it cost
   more in some other form."

Of course, it costs more, and I have to keep copying it from place to place,
adding to the code complexity.  But I was feeling rather clever to have
found that hole!


2) The second is a spelling error that I noticed in passing.  It's within
the same diff -u area (close to other insertions both before and after), so
fixing it was trivial:

- * to increse this, although since:
+ * to increase this, although since:

Are we not supposed to fix spelling errors in comments?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists