[Differential] [Requested Changes To] D5872: tcp: Don't prematurely drop receiving-only connections
jtl requested changes to this revision. jtl added a reviewer: jtl. jtl added a comment. This revision now requires changes to proceed. Doesn't tp->t_rxtshift get updated on a successful send? If not, I think that is what we should be fixing. Personally, I think a connection should drop if we aren't able to send any ACKs for 350 seconds. INLINE COMMENTS sys/netinet/tcp_output.c:1561 You already own a lock on tp. No need to do atomic operations. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, hiren, lstewart, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl Cc: jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] [Commented On] D5872: tcp: Don't prematurely drop receiving-only connections
jtl added a comment. In https://reviews.freebsd.org/D5872#127063, @jtl wrote: > Doesn't tp->t_rxtshift get updated on a successful send? If not, I think that is what we should be fixing. Of course, tp->t_rxtshift doesn't get updated on a successful send. That's not the way this works. :-) Now that I've had more coffee... This probably amounts to the generic problem of: - When we send data, we should be setting the retransmit timer to make sure we retransmit. - When we are not sending data, we should be setting a different timer (DELACK, perhaps?) to make sure we send an ACK, when able. The key feature that makes the retransmit timer inappropriate for an ACK-only case is that it is only stopped when we receive input; however, in the ACK-only case, we really want to stop it as soon as we transmit a successful ACK. Of course, we could just drop the ACK and everything would "just work". But, it //probably// is still a good idea to try to re-transmit the ACK. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, hiren, lstewart, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl Cc: jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] [Commented On] D5872: tcp: Don't prematurely drop receiving-only connections
jtl added a comment.
I think something like this code will get you closer to what you want:
Index: sys/netinet/tcp_output.c
===
--- sys/netinet/tcp_output.c(revision 298090)
+++ sys/netinet/tcp_output.c(working copy)
@@ -1545,10 +1545,16 @@
tp->t_softerror = error;
return (error);
case ENOBUFS:
- if (!tcp_timer_active(tp, TT_REXMT) &&
- !tcp_timer_active(tp, TT_PERSIST))
- tcp_timer_activate(tp, TT_REXMT, tp->t_rxtcur);
- tp->snd_cwnd = tp->t_maxseg;
+ if (len > 0 || (flags & (TH_SYN|TH_FIN))) {
+ if (!tcp_timer_active(tp, TT_REXMT) &&
+ !tcp_timer_active(tp, TT_PERSIST))
+ tcp_timer_activate(tp, TT_REXMT,
+ tp->t_rxtcur);
+ tp->snd_cwnd = tp->t_maxseg;
+ } else if (!tcp_timer_active(tp, TT_DELACK) &&
+ (flags & TH_RST) == 0)
+ tcp_timer_activate(tp, TT_DELACK,
+ tcp_delacktime);
return (0);
case EMSGSIZE:
/*
I think this code gets you closer to what you want.
I agree that the present code seems wrong. Using the retransmit timers for
ACKs seems inappropriate. But, I am generally concerned about how the system
responds when loaded. I'm not sure its a good thing to add more work to a
system that is already overloaded. Things will automatically fix themselves in
the presence of lost ACKs. The reason to retransmit ACKs is simply to prevent
unnecessary retransmissions and the latency that comes with waiting for the
retransmission to timeout. But, on an overloaded system, that may or may not be
a good thing.
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
___
freebsd-net@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] [Commented On] D5872: tcp: Don't prematurely drop receiving-only connections
jtl added a comment. 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. > 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. 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 ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
jtl added a comment. FWIW, I agree with deleting the ENOBUFs special-case. If we haven't already set the right timers by here, we have another bug which needs to be fixed. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, transport, jtl, hiren, lstewart Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
[Differential] D5872: tcp: Don't prematurely drop receiving-only connections
jtl added inline comments. INLINE COMMENTS sys/netinet/tcp_output.c:1551 In my opinion, this does //not// need to be a panic. A KASSERT() should be sufficient. Also, this is not the re-usable macro which Lawrence suggested. REVISION DETAIL https://reviews.freebsd.org/D5872 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: sepherosa_gmail.com, network, glebius, adrian, delphij, decui_microsoft.com, honzhan_microsoft.com, howard0su_gmail.com, freebsd-net-list, lstewart, hiren, jtl, transport Cc: gnn, mike-karels.net, jtl ___ freebsd-net@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/freebsd-net To unsubscribe, send any mail to "freebsd-net-unsubscr...@freebsd.org"
