Change the Nagle algorithm to allow a single undersized packet in flight at a time

[thrnz]

Currently, libtorrent will hold back an undersized uTP packet until all previously transmitted packets have been ack'd. This can add an extra RTT of delay when sending the last few bytes of a 'big' send (eg. a stream of data followed by a choke).

This suggested change will only hold back the undersized packet if there is currently an undersized packet in flight, essentially allowing one undersized packet to be sent per RTT. This would allow the tail of a 'big' send to go out immediatley without any extra delay.

This is done by noting the sequence number of the last undersized packet sent, as suggested here

See #6935

[arvidn]

i m_nagle_seq_nr == m_acked_seq_nr, that means we don't have an outstanding undersized packet, right?
I believe this logic handles that case incorrectly. compare_less_wrap() would return false and we would hold off the packet.

If I understand correctly, this last condition should be:

&& compare_less_wrap(m_acked_seq_nr, m_nagle_seq_nr, ACK_MASK))

[arvidn]

I believe the comment aboc also applies here

[arvidn]

this is an interesting result of this test: https://github.com/arvidn/libtorrent/runs/8244601393?check_suite_focus=true#step:5:723

If it's not caused by the comparison issue, it would be good to understand where the redundant packet is coming from

[thrnz]

Well spotted. I'll fix that up and see how that effects that unexpected packet.

[thrnz]

Looks like its still there. Might have to do some more digging to work out whats going on.

[arvidn]

the redundant packet counter is incremented on utp_stream.cpp:2367.
We receive a packet that's ACKing a packet that hasn't been sent. The socket is in fin_sent state, and when sending a FIN, the seq_nr is not incremented. So it looks like this is the FIN being acked, but it's ignored as an invalid ack.
The incoming packet is also a FIN.

[arvidn]

I believe this change should be made as well. It's unrelated to your change except that they affect the simulation outcome:

diff --git a/src/utp_stream.cpp b/src/utp_stream.cpp
index 4d142daac..18a134ceb 100644
--- a/src/utp_stream.cpp
+++ b/src/utp_stream.cpp
@@ -2353,7 +2353,7 @@ bool utp_socket_impl::incoming_packet(span<char const> b
        // Note that when we send a FIN, we don't increment m_seq_nr
        std::uint16_t const cmp_seq_nr =
                ((state() == state_t::syn_sent || state() == state_t::fin_sent)
-                       && ph->get_type() == ST_STATE)
+                       && (ph->get_type() == ST_STATE || ph->get_type() == ST_FIN))
                ? m_seq_nr : (m_seq_nr - 1) & ACK_MASK;
 
        if ((state() != state_t::none || ph->get_type() != ST_SYN)

[arvidn]

feel free to incorporate that patch in this PR, or just fix up the test and I can add it afterwards

[thrnz]

feel free to incorporate that patch in this PR, or just fix up the test and I can add it afterwards

Will do.

I'll fix the expected packet counts in test_utp.cpp once #7056 is merged as it will likely effect the results.