Date: Fri, 19 Nov 2021 04:28:14 GMT From: Gleb Smirnoff <glebius@FreeBSD.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org Subject: git: ff94500855c1 - main - Add tcp_freecb() - single place to free tcpcb. Message-ID: <202111190428.1AJ4SEAh021121@gitrepo.freebsd.org>
next in thread | raw e-mail | index | archive | help
The branch main has been updated by glebius: URL: https://cgit.FreeBSD.org/src/commit/?id=ff94500855c16d0d9cc18aa8b0ba73ea94020c56 commit ff94500855c16d0d9cc18aa8b0ba73ea94020c56 Author: Gleb Smirnoff <glebius@FreeBSD.org> AuthorDate: 2021-11-19 04:26:09 +0000 Commit: Gleb Smirnoff <glebius@FreeBSD.org> CommitDate: 2021-11-19 04:27:45 +0000 Add tcp_freecb() - single place to free tcpcb. Until this change there were two places where we would free tcpcb - tcp_discardcb() in case if all timers are drained and tcp_timer_discard() otherwise. They were pretty much copy-n-paste, except that in the default case we would run tcp_hc_update(). Merge this into single function tcp_freecb() and move new short version of tcp_timer_discard() to tcp_timer.c and make it static. Reviewed by: rrs, hselasky Differential revision: https://reviews.freebsd.org/D32965 --- sys/netinet/tcp_subr.c | 180 +++++++++++++++++++++--------------------------- sys/netinet/tcp_timer.c | 23 +++++++ sys/netinet/tcp_timer.h | 1 - sys/netinet/tcp_var.h | 1 + 4 files changed, 101 insertions(+), 104 deletions(-) diff --git a/sys/netinet/tcp_subr.c b/sys/netinet/tcp_subr.c index 5563148a52ad..5fa16f43f28b 100644 --- a/sys/netinet/tcp_subr.c +++ b/sys/netinet/tcp_subr.c @@ -2392,11 +2392,6 @@ void tcp_discardcb(struct tcpcb *tp) { struct inpcb *inp = tp->t_inpcb; - struct socket *so = inp->inp_socket; -#ifdef INET6 - int isipv6 = (inp->inp_vflag & INP_IPV6) != 0; -#endif /* INET6 */ - int released __unused; INP_WLOCK_ASSERT(inp); @@ -2458,121 +2453,100 @@ tcp_discardcb(struct tcpcb *tp) CC_ALGO(tp) = NULL; inp->inp_ppcb = NULL; if (tp->t_timers->tt_draincnt == 0) { - /* We own the last reference on tcpcb, let's free it. */ + bool released __diagused; + + released = tcp_freecb(tp); + KASSERT(!released, ("%s: inp %p should not have been released " + "here", __func__, inp)); + } +} + +bool +tcp_freecb(struct tcpcb *tp) +{ + struct inpcb *inp = tp->t_inpcb; + struct socket *so = inp->inp_socket; +#ifdef INET6 + bool isipv6 = (inp->inp_vflag & INP_IPV6) != 0; +#endif + + INP_WLOCK_ASSERT(inp); + MPASS(tp->t_timers->tt_draincnt == 0); + + /* We own the last reference on tcpcb, let's free it. */ #ifdef TCP_BLACKBOX - tcp_log_tcpcbfini(tp); + tcp_log_tcpcbfini(tp); #endif - TCPSTATES_DEC(tp->t_state); - if (tp->t_fb->tfb_tcp_fb_fini) - (*tp->t_fb->tfb_tcp_fb_fini)(tp, 1); + TCPSTATES_DEC(tp->t_state); + if (tp->t_fb->tfb_tcp_fb_fini) + (*tp->t_fb->tfb_tcp_fb_fini)(tp, 1); + /* + * If we got enough samples through the srtt filter, + * save the rtt and rttvar in the routing entry. + * 'Enough' is arbitrarily defined as 4 rtt samples. + * 4 samples is enough for the srtt filter to converge + * to within enough % of the correct value; fewer samples + * and we could save a bogus rtt. The danger is not high + * as tcp quickly recovers from everything. + * XXX: Works very well but needs some more statistics! + * + * XXXRRS: Updating must be after the stack fini() since + * that may be converting some internal representation of + * say srtt etc into the general one used by other stacks. + * Lets also at least protect against the so being NULL + * as RW stated below. + */ + if ((tp->t_rttupdated >= 4) && (so != NULL)) { + struct hc_metrics_lite metrics; + uint32_t ssthresh; + + bzero(&metrics, sizeof(metrics)); /* - * If we got enough samples through the srtt filter, - * save the rtt and rttvar in the routing entry. - * 'Enough' is arbitrarily defined as 4 rtt samples. - * 4 samples is enough for the srtt filter to converge - * to within enough % of the correct value; fewer samples - * and we could save a bogus rtt. The danger is not high - * as tcp quickly recovers from everything. - * XXX: Works very well but needs some more statistics! + * Update the ssthresh always when the conditions below + * are satisfied. This gives us better new start value + * for the congestion avoidance for new connections. + * ssthresh is only set if packet loss occurred on a session. * - * XXXRRS: Updating must be after the stack fini() since - * that may be converting some internal representation of - * say srtt etc into the general one used by other stacks. - * Lets also at least protect against the so being NULL - * as RW stated below. + * XXXRW: 'so' may be NULL here, and/or socket buffer may be + * being torn down. Ideally this code would not use 'so'. */ - if ((tp->t_rttupdated >= 4) && (so != NULL)) { - struct hc_metrics_lite metrics; - uint32_t ssthresh; - - bzero(&metrics, sizeof(metrics)); + ssthresh = tp->snd_ssthresh; + if (ssthresh != 0 && ssthresh < so->so_snd.sb_hiwat / 2) { /* - * Update the ssthresh always when the conditions below - * are satisfied. This gives us better new start value - * for the congestion avoidance for new connections. - * ssthresh is only set if packet loss occurred on a session. - * - * XXXRW: 'so' may be NULL here, and/or socket buffer may be - * being torn down. Ideally this code would not use 'so'. + * convert the limit from user data bytes to + * packets then to packet data bytes. */ - ssthresh = tp->snd_ssthresh; - if (ssthresh != 0 && ssthresh < so->so_snd.sb_hiwat / 2) { - /* - * convert the limit from user data bytes to - * packets then to packet data bytes. - */ - ssthresh = (ssthresh + tp->t_maxseg / 2) / tp->t_maxseg; - if (ssthresh < 2) - ssthresh = 2; - ssthresh *= (tp->t_maxseg + + ssthresh = (ssthresh + tp->t_maxseg / 2) / tp->t_maxseg; + if (ssthresh < 2) + ssthresh = 2; + ssthresh *= (tp->t_maxseg + #ifdef INET6 - (isipv6 ? sizeof (struct ip6_hdr) + - sizeof (struct tcphdr) : + (isipv6 ? sizeof (struct ip6_hdr) + + sizeof (struct tcphdr) : #endif - sizeof (struct tcpiphdr) + sizeof (struct tcpiphdr) #ifdef INET6 - ) + ) #endif - ); - } else - ssthresh = 0; - metrics.rmx_ssthresh = ssthresh; + ); + } else + ssthresh = 0; + metrics.rmx_ssthresh = ssthresh; - metrics.rmx_rtt = tp->t_srtt; - metrics.rmx_rttvar = tp->t_rttvar; - metrics.rmx_cwnd = tp->snd_cwnd; - metrics.rmx_sendpipe = 0; - metrics.rmx_recvpipe = 0; + metrics.rmx_rtt = tp->t_srtt; + metrics.rmx_rttvar = tp->t_rttvar; + metrics.rmx_cwnd = tp->snd_cwnd; + metrics.rmx_sendpipe = 0; + metrics.rmx_recvpipe = 0; - tcp_hc_update(&inp->inp_inc, &metrics); - } - refcount_release(&tp->t_fb->tfb_refcnt); - tp->t_inpcb = NULL; - uma_zfree(V_tcpcb_zone, tp); - released = in_pcbrele_wlocked(inp); - KASSERT(!released, ("%s: inp %p should not have been released " - "here", __func__, inp)); + tcp_hc_update(&inp->inp_inc, &metrics); } -} -void -tcp_timer_discard(void *ptp) -{ - struct inpcb *inp; - struct tcpcb *tp; - struct epoch_tracker et; + refcount_release(&tp->t_fb->tfb_refcnt); + uma_zfree(V_tcpcb_zone, tp); - tp = (struct tcpcb *)ptp; - CURVNET_SET(tp->t_vnet); - NET_EPOCH_ENTER(et); - inp = tp->t_inpcb; - KASSERT(inp != NULL, ("%s: tp %p tp->t_inpcb == NULL", - __func__, tp)); - INP_WLOCK(inp); - KASSERT((tp->t_timers->tt_flags & TT_STOPPED) != 0, - ("%s: tcpcb has to be stopped here", __func__)); - tp->t_timers->tt_draincnt--; - if (tp->t_timers->tt_draincnt == 0) { - /* We own the last reference on this tcpcb, let's free it. */ -#ifdef TCP_BLACKBOX - tcp_log_tcpcbfini(tp); -#endif - TCPSTATES_DEC(tp->t_state); - if (tp->t_fb->tfb_tcp_fb_fini) - (*tp->t_fb->tfb_tcp_fb_fini)(tp, 1); - refcount_release(&tp->t_fb->tfb_refcnt); - tp->t_inpcb = NULL; - uma_zfree(V_tcpcb_zone, tp); - if (in_pcbrele_wlocked(inp)) { - NET_EPOCH_EXIT(et); - CURVNET_RESTORE(); - return; - } - } - INP_WUNLOCK(inp); - NET_EPOCH_EXIT(et); - CURVNET_RESTORE(); + return (in_pcbrele_wlocked(inp)); } /* diff --git a/sys/netinet/tcp_timer.c b/sys/netinet/tcp_timer.c index feea3765821c..67e550b83bce 100644 --- a/sys/netinet/tcp_timer.c +++ b/sys/netinet/tcp_timer.c @@ -1042,6 +1042,29 @@ tcp_timers_unsuspend(struct tcpcb *tp, uint32_t timer_type) } } +static void +tcp_timer_discard(void *ptp) +{ + struct inpcb *inp; + struct tcpcb *tp; + struct epoch_tracker et; + + tp = (struct tcpcb *)ptp; + CURVNET_SET(tp->t_vnet); + NET_EPOCH_ENTER(et); + inp = tp->t_inpcb; + KASSERT(inp != NULL, ("%s: tp %p tp->t_inpcb == NULL", + __func__, tp)); + INP_WLOCK(inp); + KASSERT((tp->t_timers->tt_flags & TT_STOPPED) != 0, + ("%s: tcpcb has to be stopped here", __func__)); + if (--tp->t_timers->tt_draincnt > 0 || + tcp_freecb(tp) == false) + INP_WUNLOCK(inp); + NET_EPOCH_EXIT(et); + CURVNET_RESTORE(); +} + void tcp_timer_stop(struct tcpcb *tp, uint32_t timer_type) { diff --git a/sys/netinet/tcp_timer.h b/sys/netinet/tcp_timer.h index 9a711d173386..c5317d1a4155 100644 --- a/sys/netinet/tcp_timer.h +++ b/sys/netinet/tcp_timer.h @@ -217,7 +217,6 @@ void tcp_inpinfo_lock_del(struct inpcb *inp, struct tcpcb *tp); void tcp_timer_init(void); void tcp_timer_2msl(void *xtp); -void tcp_timer_discard(void *); struct tcptw * tcp_tw_2msl_scan(int reuse); /* XXX temporary? */ void tcp_timer_keep(void *xtp); diff --git a/sys/netinet/tcp_var.h b/sys/netinet/tcp_var.h index 1511da3c70fd..46d00914354f 100644 --- a/sys/netinet/tcp_var.h +++ b/sys/netinet/tcp_var.h @@ -967,6 +967,7 @@ int tcp_ccalgounload(struct cc_algo *unload_algo); struct tcpcb * tcp_close(struct tcpcb *); void tcp_discardcb(struct tcpcb *); +bool tcp_freecb(struct tcpcb *); void tcp_twstart(struct tcpcb *); void tcp_twclose(struct tcptw *, int); void tcp_ctlinput(int, struct sockaddr *, void *);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?202111190428.1AJ4SEAh021121>