Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inflight-queue optimization and remove fifo #197

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

jthomas43
Copy link
Contributor

This PR updates the linked-queue PR

  1. adds a circularly-linked queue data structure that replaces the fifo data structure. It doesn't leave gaps when an item is removed and doesn't scan if the gc_hint is wrong.
  2. it replaces the fifo data structure used for: stream->retransmit_queue, stream->unordered_queue, socket->send_queue, stream->write_queue with the linked queue, and removes the fifo data structure.

@@ -274,7 +272,7 @@ print_interval (udxperf_client_t *client, uint64_t bytes, uint64_t start, uint64
byte_snprintf(bps_buf, sizeof bps_buf, bytes / time_sec, 'a');
bps_buf[19] = '\0';

printf("[%3d] %6.4f-%6.4f sec %s %s/sec", stream->local_id, (start - client->start_time) / 1000.0, (end - client->start_time) / 1000.0, bytes_buf, bps_buf, stream->cwnd);
printf("[%3d] %6.4f-%6.4f sec %s %s/sec", stream->local_id, (start - client->start_time) / 1000.0, (end - client->start_time) / 1000.0, bytes_buf, bps_buf);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes warning of extra printf parameter

@@ -34,7 +35,7 @@ debug_print_cwnd_stats (udx_stream_t *stream) {
static void
debug_print_outgoing (udx_stream_t *stream) {
if (DEBUG) {
for (uint32_t s = stream->remote_acked; s < stream->seq; s++) {
for (uint32_t s = stream->remote_acked; s != stream->seq; s++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matters when sequence numbers wrap


if (pkt->type & UDX_PACKET_FREE_ON_SEND) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is free is safe to remove. the only packet type put in the unordered queue are packet UDX_PACKET_TYPE_STREAM_SEND, which is not free-on-send.

assert(pkt->transmits > 0);

// debug_printf("%lu > %lu=%d\n", stream->rack_time_sent, pkt->time_sent, stream->rack_time_sent > pkt->time_sent);

if (!rack_sent_after(stream->rack_time_sent, stream->rack_next_seq, pkt->time_sent, pkt->seq + 1)) {
continue;
if (pkt->time_sent > stream->rack_time_sent) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the optimization from RFC 8985:

As an optimization, an implementation can choose to check only
segments that have been sent before RACK.xmit_ts. This can be more
efficient than scanning the entire SACK scoreboard, especially when
there are many segments in flight. The implementation can use a
separate doubly linked list ordered by Segment.xmit_ts, insert a
segment at the tail of the list when it is (re)transmitted, and
remove a segment from the list when it is delivered or marked as
lost. In Linux TCP, this optimization improved CPU usage by orders
of magnitude during some fast recovery episodes on high-speed WAN
networks.

@@ -1289,13 +1284,13 @@ udx_rto_timeout (uv_timer_t *timer) {

// rack 6.3

for (uint32_t seq = stream->remote_acked; seq < stream->seq; seq++) {
for (uint32_t seq = stream->remote_acked; seq != stream->seq; seq++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bug if RTO triggers during sequence number wrap

@@ -1510,15 +1505,10 @@ process_sacks (udx_stream_t *stream, char *buf, size_t buf_len) {

static void
process_data_packet (udx_stream_t *stream, int type, uint32_t seq, char *data, ssize_t data_len) {
if (seq == stream->ack && type == UDX_HEADER_DATA) {
if (seq == stream->ack && type & UDX_HEADER_DATA) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A packet could have type DATA | END, such a packet would end up in the out-of-order queue, and then processed like normal.

if (stream->socket != NULL) {
update_poll(stream->socket);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is redundant - write_wanted |= WRITE_WANT_STATE is set later in process_packet if the packet type was DATA or END

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant