Date: Sat, 08 Oct 2011 11:56:13 +1100 From: Lawrence Stewart <lstewart@freebsd.org> To: Andre Oppermann <andre@FreeBSD.org> Cc: svn-src-head@FreeBSD.org, svn-src-all@FreeBSD.org, src-committers@FreeBSD.org, FreeBSD Release Engineering Team <re@freebsd.org>, John Baldwin <jhb@freebsd.org> Subject: Re: svn commit: r226113 - head/sys/netinet Message-ID: <4E8F9FAD.7070103@freebsd.org> In-Reply-To: <201110071639.p97Gd3t4019128@svn.freebsd.org> References: <201110071639.p97Gd3t4019128@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Hi Andre and RE team, I've had a patch sitting in re@'s inbox for this problem since 15th Sep and have been waiting for their go-ahead to commit. The patch I submitted is at: http://people.freebsd.org/~lstewart/patches/misctcp/tcpreassstackfix_9.x.r225576.diff The proposed commit message was: ########## Use a backup (stack allocated) struct tseg_qent when we are unable to allocate one from the TCP reassembly UMA zone and the incoming segment is the one we've been waiting for (i.e. th_seq == rcv_nxt). This avoids TCP connections stalling when the zone limit is reached. PR: kern/155407 Reported by: Slawa Olhovchenkov and Steven Hartland Tested by: Steven Hartland Submitted by: andre Reviewed by: jhb Approved by: re (?) MFC after: 1 week ########## I feel the logging changes should have been committed separately to the fix, but other than that, what you committed achieves the same thing as the patch I proposed. I should have updated the ML thread to say it was submitted and awaiting approval, so you weren't to know. Anyhoo, I guess I'll leave it up to you and re@ to sort out how you want to proceed, but wanted to make sure everyone was on the same page as RE would have gotten confused when you requested your patch be MFCed. Cheers, Lawrence On 10/08/11 03:39, Andre Oppermann wrote: > Author: andre > Date: Fri Oct 7 16:39:03 2011 > New Revision: 226113 > URL: http://svn.freebsd.org/changeset/base/226113 > > Log: > Prevent TCP sessions from stalling indefinitely in reassembly > when reaching the zone limit of reassembly queue entries. > > When the zone limit was reached not even the missing segment > that would complete the sequence space could be processed > preventing the TCP session forever from making any further > progress. > > Solve this deadlock by using a temporary on-stack queue entry > for the missing segment followed by an immediate dequeue again > by delivering the contiguous sequence space to the socket. > > Add logging under net.inet.tcp.log_debug for reassembly queue > issues. > > Reviewed by: lsteward (previous version) > Tested by: Steven Hartland<killing-at-multiplay.co.uk> > MFC after: 3 days > > Modified: > head/sys/netinet/tcp_reass.c > > Modified: head/sys/netinet/tcp_reass.c > ============================================================================== > --- head/sys/netinet/tcp_reass.c Fri Oct 7 16:09:44 2011 (r226112) > +++ head/sys/netinet/tcp_reass.c Fri Oct 7 16:39:03 2011 (r226113) > @@ -177,7 +177,9 @@ tcp_reass(struct tcpcb *tp, struct tcphd > struct tseg_qent *nq; > struct tseg_qent *te = NULL; > struct socket *so = tp->t_inpcb->inp_socket; > + char *s = NULL; > int flags; > + struct tseg_qent tqs; > > INP_WLOCK_ASSERT(tp->t_inpcb); > > @@ -215,19 +217,40 @@ tcp_reass(struct tcpcb *tp, struct tcphd > TCPSTAT_INC(tcps_rcvmemdrop); > m_freem(m); > *tlenp = 0; > + if ((s = tcp_log_addrs(&tp->t_inpcb->inp_inc, th, NULL, NULL))) { > + log(LOG_DEBUG, "%s; %s: queue limit reached, " > + "segment dropped\n", s, __func__); > + free(s, M_TCPLOG); > + } > return (0); > } > > /* > * Allocate a new queue entry. If we can't, or hit the zone limit > * just drop the pkt. > + * > + * Use a temporary structure on the stack for the missing segment > + * when the zone is exhausted. Otherwise we may get stuck. > */ > te = uma_zalloc(V_tcp_reass_zone, M_NOWAIT); > - if (te == NULL) { > + if (te == NULL&& th->th_seq != tp->rcv_nxt) { > TCPSTAT_INC(tcps_rcvmemdrop); > m_freem(m); > *tlenp = 0; > + if ((s = tcp_log_addrs(&tp->t_inpcb->inp_inc, th, NULL, NULL))) { > + log(LOG_DEBUG, "%s; %s: global zone limit reached, " > + "segment dropped\n", s, __func__); > + free(s, M_TCPLOG); > + } > return (0); > + } else if (th->th_seq == tp->rcv_nxt) { > + bzero(&tqs, sizeof(struct tseg_qent)); > + te =&tqs; > + if ((s = tcp_log_addrs(&tp->t_inpcb->inp_inc, th, NULL, NULL))) { > + log(LOG_DEBUG, "%s; %s: global zone limit reached, " > + "using stack for missing segment\n", s, __func__); > + free(s, M_TCPLOG); > + } > } > tp->t_segqlen++; > > @@ -304,6 +327,8 @@ tcp_reass(struct tcpcb *tp, struct tcphd > if (p == NULL) { > LIST_INSERT_HEAD(&tp->t_segq, te, tqe_q); > } else { > + KASSERT(te !=&tqs, ("%s: temporary stack based entry not " > + "first element in queue", __func__)); > LIST_INSERT_AFTER(p, te, tqe_q); > } > > @@ -327,7 +352,8 @@ present: > m_freem(q->tqe_m); > else > sbappendstream_locked(&so->so_rcv, q->tqe_m); > - uma_zfree(V_tcp_reass_zone, q); > + if (q !=&tqs) > + uma_zfree(V_tcp_reass_zone, q); > tp->t_segqlen--; > q = nq; > } while (q&& q->tqe_th->th_seq == tp->rcv_nxt);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4E8F9FAD.7070103>