Date: Sat, 16 Apr 2016 07:09:28 +0000 From: "hiren (hiren panchasara)" <phabric-noreply@FreeBSD.org> To: freebsd-net@freebsd.org Subject: [Differential] [Commented On] D5872: tcp: Don't prematurely drop receiving-only connections Message-ID: <b838025746fa312cdc5ca9e3f1f0fd2f@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-5vu7wrfibtoing77xfvt-req@FreeBSD.org> References: <differential-rev-PHID-DREV-5vu7wrfibtoing77xfvt-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
hiren added a comment. In https://reviews.freebsd.org/D5872#127345, @jtl wrote: > In https://reviews.freebsd.org/D5872#127343, @mike-karels.net wrote: > > > If we get an ENOBUFS when sending data, we will already be running the retransmit timer. > > > Good point, but see below. > > > If we drop an ACK on ENOBUFS, either we will receive more data and > > attempt another ACK, or the sender will time out and resend data. Either > > will get the connection started again. > > True, but it may take time if we have to wait for a retransmission time out. That isn't necessarily a bad thing. But setting retransmission timer on an ACK seems... wrong. >> I believe lines 1552-1554 should >> simply be deleted. > > On the surface, that seems like a reasonable alternative. However, its interesting to see why this code was originally added. It looks like it was added by jlemon in r61179. Here's what the commit message says: > >> When attempting to transmit a packet, if the system fails to allocate >> a mbuf, it may return without setting any timers. If no more data is >> scheduled to be transmitted (this was a FIN) the system will sit in >> LAST_ACK state forever. >> >> Thus, when mbuf allocation fails, set the retransmit timer if neither >> the retransmit or persist timer is already pending. > > Even back then, the retransmit and/or persist timers should have been running by this point in the code. It would be interesting to know if jlemon proved this code fixed the problem, or it was just speculated. We fixed a similar-sounding problem last year, and the code is different now, so these particular lines may truly no longer be necessary. But, I am a little hesitant to remove this code without knowing more about its origin. > > Just my 2c. I guess this boils down to how you want to "deal" with this (possibly) transient memory error. I'd like to go with @mike-karels.net's suggestion as when we reach to this point in code, I don't see how timers couldn't have been set already. And I doubt we can get any more info from that old commit :/ But because this is (sigh..) tcp and if we want to be really cautious, I like @jtl 's patch of handling this for 'acks' with delack. (In that patch, I am not sure why we want to drop cwnd down to 1mss only in case of data and not for acks. I think we should do that for both?) (fwiw, other BSDs don't have this special handling as Mark suggested.) REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, lstewart, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl, hiren Cc: mike-karels.net, jtl
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b838025746fa312cdc5ca9e3f1f0fd2f>