Do not send too much

[kazuho]

Up until now, quicly has been sending too much, unless the amount of data that can be sent at given moment is the exact multiple of the MTU size.

In effect, quicly is thought to have been rounding up the send window size by the unit of 10 datagrams (the number of datagram buffers supplied by the caller to to quicly_send), because when st_quicly_send_context_t::send_window is calculated from CWND size and bytes inflight, the value rarely becomes an exact multiple of MTU.

When PTO expires or when a time threshold loss detection is triggered while the CWND is full, send_window is set to an exact multiple of MTU and therefore quicly has behaved correctly.

EDIT. This regression seems to have gone in in #409.

[hfujita]

Looks good.

Just curious, if send_window could become negative, should we also change this line?
https://github.com/h2o/quicly/blob/master/lib/quicly.c#L4165

[kazuho]

@hfujita Thank you for the review.

Just curious, if send_window could become negative, should we also change this line? https://github.com/h2o/quicly/blob/master/lib/quicly.c#L4165

I do not think we have to, as the return type of calc_send_window is size_t. Underflow becomes an issue once we start modifying the variable.

Thinking a bit more, it's my impression that the danger is us accidentally changing code that depends on the signedness of the value; hence 79d6178.

[hfujita]

I see. I just realized that I thought calc_send_window would return ssize_t by mistake.

[janaiyengar]

Why do we want to retain this property? Why should send_window be allowed to become negative?

[kazuho]

@janaiyengar

Why should send_window be allowed to become negative?

That's an interesting point. Not that I have a strong opinion, but this value is not the window retained by the congestion controller; this variable is a member of st_quicly_send_context_t.

The variable is initialized in do_send, and gets subtracted each time a QUIC packet is finalized. Quicly continues building full-sized packets while the variable remains positive (meaning that at least one more byte can be sent). Therefore, overshooting the "window" is a norm. After generating packets, quicly_send_context_t is destroyed.

IMO the question here are:

• Is it a good idea to retain maximum_bytes_to_sent - bytes_actually_sent?
• Assuming that the answer to the first question is yes, is it a good idea to call the variable "send_window"?

I tend to think that the answer to the first question is yes. Regarding the second question, I do not have an opinion.

[janaiyengar]

Thanks for your response, @kazuho. After our discussion (offline), I think the following is true:

• it is ok for the local send_window variable to be negative.
• the damage is limited because each send opportunity sends at most 10 packets. This means that at any given time, we are going to be limited to having at most 9.x packets over the congestion window. This is significantly better than what I thought at first.
• we need to have some sort of an end-to-end test that breaks on egregious cc breakages.

I agree that we should get this PR in asap however.

[kazuho]

Thank you for the reviews.

the damage is limited because each send opportunity sends at most 10 packets. This means that at any given time, we are going to be limited to having at most 9.x packets over the congestion window. This is significantly better than what I thought at first.

I am super glad to hear this analysis, and I think it is correct.

[kazuho]

Confirmed that master behaves as @janaiyengar assumed. https://gist.github.com/kazuho/51981d3a3b36dced97bb52d167ecd573 has logs from each branch.

In case of this branch, the first application flight is 13 packets, then for every ACK acking 2 packets, the sender sends 4 packets.

In case of master, the first application flight is 20 packets, then it waits for 2 ACKs acking 4 packets, before sending another burst.