Re: [net-next PATCH v4 2/3] net: TCP thin linear timeouts

[Franco Fichtner]

Andreas Petlund wrote:
> On 02/18/2010 10:09 AM, Ilpo Järvinen wrote:
>   
> > On Thu, 18 Feb 2010, Franco Fichtner wrote:
> >
> >     
> > > Andreas Petlund wrote:
> > >       
> > > > On 02/18/2010 09:41 AM, Ilpo Järvinen wrote:
> > > >   
> > > >         
> > > > > On Wed, 17 Feb 2010, David Miller wrote:
> > > > >
> > > > >     
> > > > >           
> > > > > > From: Andreas Petlund <apetlund@xxxxxxxxx>
> > > > > > Date: Tue, 16 Feb 2010 15:40:41 +0100
> > > > > >
> > > > > >       
> > > > > >             
> > > > > > > @@ -341,6 +342,8 @@ struct tcp_sock {
> > > > > > >  	u16	advmss;		/* Advertised MSS
> > > > > > > */
> > > > > > >  	u8	frto_counter;	/* Number of new acks after RTO */
> > > > > > >  	u8	nonagle;	/* Disable Nagle algorithm?
> > > > > > > */
> > > > > > > +	u8      thin_lto    : 1,/* Use linear timeouts for thin
> > > > > > > streams */
> > > > > > > +		thin_undef  : 7;
> > > > > > >          
> > > > > > >               
> > > > > > There is now a gap of 3 unused bytes here in this critical
> > > > > > core TCP socket data structure.
> > > > > >
> > > > > > Please either find a way to avoid this hole, or document
> > > > > > it with a comment.
> > > > > >       
> > > > > >             
> > > > > There would be multiple bits free for use in both frto_counter and nonagle
> > > > > byte.
> > > > >
> > > > >     
> > > > >           
> > > > I was playing aroud with this setup:
> > > >
> > > > =========
> > > > u8	nonagle     : 4,/* Disable Nagle algorithm?             */
> > > > 	thin_lto    : 1,/* Use linear timeouts for thin streams */
> > > > 	thin_dupack : 1,/* Fast retransmit on first dupack      */
> > > > 	thin_undef  : 2;
> > > > =========
> > > >
> > > > Do you think that would do the trick?
> > > >   
> > > >         
> > > According to Ilpo, it would be ok to reduce both ftro_counter and
> > > nonagle, so why not join all these into u16 and leave the remaining
> > > free bits documented for other people. Like this:
> > >
> > >    u16 frto_counter:x; /* Number of new acks after RTO */
> > >    u16 nonagle:y; /* Disable Nagle algorithm? */
> > >    u16 thin_lto:1; /* Use linear timeouts for thin streams */
> > >    u16 unused:15-x-y;
> > >
> > > Not sure about the y and x. Ilpo, can you comment on those values?
> > >       
> > I don't remember top of the hat how much of nonagle used, but for 
> > frto_counter max value was 3 iirc. 
> >     
>
> I think nonagle uses 4 bits:
> ======
> #define TCP_NAGLE_OFF           1       /* Nagle's algo is disabled */
> #define TCP_NAGLE_CORK          2       /* Socket is corked         */
> #define TCP_NAGLE_PUSH          4       /* Cork is overridden for already queued data */
> ======
>
>   
That would be 3 bits. :)

> > However, I'm unsure if compiler is 
> > nowadays wise enough to handle bitfields in some not all so stupid way.
> >     
>
> Would you then recommend to use a byte for each value, thus avoiding the bitfields?
>   
No, he meant he's not sure if the compiler can merge the bitfields in a 
clever
way if written like this.

I use this style all the time at work and according to pahole it's okay. 
But then,
I'm not sure if pahole can be trusted or if there is some compiler twist on
non-intel architectures.


Franco
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/