Re: [PATCH] Customizable TCP backoff patch

[David Miller <davem@davemloft.net>]

From: Ben Woodard <woodard@redhat.com>
Date: Tue, 03 Oct 2006 11:14:38 -0700

> > Other issues:
> > 
> > 1) 2 "u32" in the tcp_sock is a lot of space to devote to this
> >    new state.  If it can fit in 2 "u16"'s or even less space,
> >    please use that.
> > 
> > 2) the expression "(tp->foo ? : sysctl_foo)" is repeated many times
> >    in the patch, please encapsulate it into an inline function
> >    or similar
> > 
> 
> How does this look to you for answering your two complaints above.

It looks a lot better.  I'd like you to take #2 further,
put the inline function into net/tcp.h and use it in the
tcp.c instances too.

Also, please don't format code like this:

+static inline __u16 rto_max(struct tcp_sock *tp){
+        return tp->rto_max ? : sysctl_tcp_rto_max;
+}
+
+static inline __u16 rto_init(struct tcp_sock *tp){
+        return tp->rto_init ? : sysctl_tcp_rto_init;
+}

The openning brace of each functions belongs on a line by itself,
thanks.

Finally, I'm not so sure "seconds" is the best unit for the
socket option.  In fact you use "seconds" as the unit for
the socket option and the sysctl is raw jiffies.  That's
inconsistency is really problematic.

At the very least, seconds might not be fine enough granularity
for some circumstances.  Heck, the default RTO_MIN is 1/5 of a
second. :-)

I also understand that going to milliseconds or microseconds would
make the size of the in-socket struct members an issue again.  These
things are never easy are they? :-/

Any better ideas?