Fix indices calculation for congestion control.

[kurnevsky]

Fixes:

• It seems the second index for sum calculated in a wrong way:

  c-toxcore/toxcore/net_crypto.c

  Line 2536 in d89f83f

  (long signed int)conn->last_sendqueue_size[(pos - (CONGESTION_QUEUE_ARRAY_SIZE - 1)) % CONGESTION_QUEUE_ARRAY_SIZE];

  
  pos is a value between 0 and CONGESTION_QUEUE_ARRAY_SIZE. So the value pos - (CONGESTION_QUEUE_ARRAY_SIZE - 1) is likely to be overflowed. It's unsigned so it won't lead to out-of-bounds access but incorrect value will be taken instead.

• Move incrementing of last_sendqueue_counter - it seems pretty illogical that it's used before and after incrementing:
  

  c-toxcore/toxcore/net_crypto.c

  Line 2530 in d89f83f

  unsigned int pos = conn->last_sendqueue_counter % CONGESTION_QUEUE_ARRAY_SIZE;

  
  

  c-toxcore/toxcore/net_crypto.c

  Line 2538 in d89f83f

  unsigned int n_p_pos = conn->last_sendqueue_counter % CONGESTION_LAST_SENT_ARRAY_SIZE;

• Take remainder of the division so that last_sendqueue_counter won't be overflowed.

Also I can't understand the meaning of this comparison:

c-toxcore/toxcore/net_crypto.c

Line 2586 in d89f83f

if (send_array_ratio > SEND_QUEUE_RATIO && CRYPTO_MIN_QUEUE_LENGTH < npackets) {

Does somebody know what it means? It seams send_array_ratio variable means the time that is required to send all packets from the send queue with the current calculated speed. And if it's higher than 2 seconds the packet_send_rate will be reduced. Why? There comments says:

/* If the send queue is SEND_QUEUE_RATIO times larger than the
 * calculated link speed the packet send speed will be reduced
 * by a value depending on this number.
 */

But how the send queue size and the link speed can be compared? They have different units of measurement. Or do I miss something?

---

This change is 

[codecov]

Codecov Report

Merging #1268 into master will increase coverage by <.1%.
The diff coverage is 83.6%.

@@           Coverage Diff            @@
##           master   #1268     +/-   ##
========================================
+ Coverage    83.3%   83.3%   +<.1%     
========================================
  Files          82      82             
  Lines       14827   14867     +40     
========================================
+ Hits        12361   12398     +37     
- Misses       2466    2469      +3

Impacted Files	Coverage Δ
auto_tests/encryptsave_test.c	95.7% <100%> (+0.6%)	⬆️
toxcore/net_crypto.c	95.2% <100%> (-0.1%)	⬇️
toxcore/crypto_core.c	91.4% <72.4%> (-8.6%)	⬇️
toxav/toxav.c	67.3% <0%> (-0.9%)	⬇️
toxcore/Messenger.c	86.7% <0%> (-0.1%)	⬇️
toxav/msi.c	65% <0%> (+0.5%)	⬆️
toxcore/friend_connection.c	95.7% <0%> (+0.8%)	⬆️
toxcore/LAN_discovery.c	85.8% <0%> (+1.8%)	⬆️
auto_tests/toxav_many_test.c	96.6% <0%> (+4%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 72ef085...e532057. Read the comment docs.

[zugz]

* Tuesday, 2018-11-20 at 09:22 -0800 - Evgeny Kurnevsky <[email protected]>:

Does somebody know what it means? It seams `send_array_ratio` variable means the time that is required to send all packets from the send queue with the current calculated speed. And if it's higher than 2 seconds the `packet_send_rate` will be reduced. Why?

I agree with your analysis (this being the first time I've looked at this code). So the idea seems to be that we have a target of keeping the queue down to a size such that it would take 2 seconds to clear, and we decrease packet_send_rate as needed to ensure this. Calling it a 'ratio' is indeed maybe a bit misleading, because the units matter; I'd call it something like TARGET_QUEUE_CLEARANCE_TIME. In any case, I checked the changes in this PR, and I approve. It seems when the code was first written CONGESTION_QUEUE_ARRAY_SIZE was a power of 2 so the wrapping wasn't a problem. But now it is.