Messages in this thread | | | From | Neal Cardwell <> | Date | Wed, 4 Jul 2018 10:13:56 -0400 | Subject | Re: [net-next,v2] tcp: Improve setsockopt() TCP_USER_TIMEOUT accuracy |
| |
On Tue, Jul 3, 2018 at 8:06 PM Jon Maxwell <jmaxwell37@gmail.com> wrote: > > Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com> > --- > net/ipv4/tcp_timer.c | 48 +++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 39 insertions(+), 9 deletions(-) > > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c > index 3b3611729928..d129e670d02a 100644 > --- a/net/ipv4/tcp_timer.c > +++ b/net/ipv4/tcp_timer.c > @@ -22,6 +22,39 @@ > #include <linux/gfp.h> > #include <net/tcp.h> > > +unsigned int tcp_retransmit_stamp(struct sock *sk) > +{ > + unsigned int start_ts = tcp_sk(sk)->retrans_stamp;
Since retrans_stamp and tcp_skb_timestamp() are both u32, I'd suggest using u32 for the local variable start_ts and the return type of the function.
> + > + if (unlikely(!start_ts)) { > + struct sk_buff *head = tcp_rtx_queue_head(sk); > + > + if (!head) > + return false;
Looks like a copy-and-paste holdover: since this function is returning an integer, I would suggest returning 0 rather than false.
> + start_ts = tcp_skb_timestamp(head); > + } > + return start_ts; > +} > + > +static __u32 tcp_clamp_rto_to_user_timeout(struct sock *sk) > +{ > + struct inet_connection_sock *icsk = inet_csk(sk); > + __u32 rto = icsk->icsk_rto; > + __u32 elapsed, user_timeout; > + unsigned int start_ts;
I'd suggest u32 here for start_ts (per the rationale above).
> + > + start_ts = tcp_retransmit_stamp(sk); > + if (!icsk->icsk_user_timeout || !start_ts) > + return rto; > + elapsed = tcp_time_stamp(tcp_sk(sk)) - start_ts; > + user_timeout = jiffies_to_msecs(icsk->icsk_user_timeout); > + if (elapsed >= user_timeout) > + rto = 1; /* user timeout has passed; fire ASAP */ > + else > + rto = min(rto, (__u32)msecs_to_jiffies(user_timeout - elapsed));
My sense is that min_t would be preferred here, e.g:
rto = min_t(__u32, rto, msecs_to_jiffies(user_timeout - elapsed));
> + return rto; > +} > + > /** > * tcp_write_err() - close socket and save error info > * @sk: The socket the error has appeared on. > @@ -166,14 +199,9 @@ static bool retransmits_timed_out(struct sock *sk, > if (!inet_csk(sk)->icsk_retransmits) > return false; > > - start_ts = tcp_sk(sk)->retrans_stamp; > - if (unlikely(!start_ts)) { > - struct sk_buff *head = tcp_rtx_queue_head(sk); > - > - if (!head) > - return false; > - start_ts = tcp_skb_timestamp(head); > - } > + start_ts = tcp_retransmit_stamp(sk); > + if (!start_ts) > + return false; > > if (likely(timeout == 0)) { > linear_backoff_thresh = ilog2(TCP_RTO_MAX/rto_base); > @@ -407,6 +435,7 @@ void tcp_retransmit_timer(struct sock *sk) > struct tcp_sock *tp = tcp_sk(sk); > struct net *net = sock_net(sk); > struct inet_connection_sock *icsk = inet_csk(sk); > + __u32 rto; > > if (tp->fastopen_rsk) { > WARN_ON_ONCE(sk->sk_state != TCP_SYN_RECV && > @@ -535,7 +564,8 @@ void tcp_retransmit_timer(struct sock *sk) > /* Use normal (exponential) backoff */ > icsk->icsk_rto = min(icsk->icsk_rto << 1, TCP_RTO_MAX); > } > - inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, icsk->icsk_rto, TCP_RTO_MAX); > + rto = tcp_clamp_rto_to_user_timeout(sk); > + inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, rto, TCP_RTO_MAX); > if (retransmits_timed_out(sk, net->ipv4.sysctl_tcp_retries1 + 1, 0)) > __sk_dst_reset(sk);
Thanks! neal
| |