(D)CUBIC, NewReno and Prague refactoring

[hfstco]

WORK IN PROGRESS

Refactor of CC code. I moved many common parts to cc_common, but I like to keep the code readable and understandable, as well as what the CC algorithm is doing, by just looking at its code. So, some parts stay in the CC code file even if some code duplicates another CC algorithm. (avoiding general picoquic_cc_do_congestion_avoidance(cc_algo, cc_param) functions)

Additionally, I try to change the CWIN (and other path variables) inside the CC code file. Most of the common CC functions return new values or increase them by values.

While doing some coverage runs, I recognized that CUBIC will not enter the recovery phase anyway. (_ alg_state _ isn't set to _ picoquic_cubic_alg_recovery _ at any time.) Even if we call enter_recovery(), we immediately transition to SS or CA. Of course, recovery_sequence is set. Is this intended, or can we remove the recovery state from the enum? Or set it to recovery while the recovery_sequence is set.

Changes:

• cubic/dcubic notification callback refactor
• dcubic calls cubic notification callback except for delay variations
• CWND increase functions (build on each other) for all CC algorithms ((D)CUBIC, NewReno and Prague) in cc_common
• app limited for all CC algorithms (as part of CWND increase functions, except NewReno currently)
• moved some CC common functions (e.g. update_bandwidth()) to cc_common
• moved common parts of congestion avoidance to cc_common
• loss filter for Prague
• introduced HyStart++ constants
• added some test cases to improve code coverage

TODO:

• changes which require lots of changes in the test suite (memlog keylog_test packet_trace ready_to_send ready_to_skip ready_to_zfin ready_to_zero pacing_update quality_update multipath_callback multipath_quality multipath_stream_af)
     - loss filter in NewReno
     - app limited for NewReno 
• different betas for (D)CUBIC/calc of beta like in Prague
• (more test cases to improve coverage)

Any feedback, changes, and help would be appreciated.

[huitema]

Wow! That a lot of work. I will review later, but this is impressive.

Regarding recovery: yes, that's a bug. We should make sure that recovery is properly entered for cubic, etc. But I share your concern about trying to change too much in a single PR. It is probably better to first complete the code cleanup that you are doing while keeping test results unchanged, then do a series of smaller PRs each fixing an individual issue. That way, we can separate refactor, which should not change test results, and individual improvements, for which we can verify that the test results change in the expected ways.

[huitema]

I left a series of comments, mostly asking for a few comments in the code in places when we will need further actions, plus a suggested name change for one of the cc functions.

I see that all tests are passing, with just a few requiring minor adjustment of the completion time target. For me, that's the key. If you address the comments below, we can probably merge that now. I would not try to solve all problems in this one large PR, because I am concerned about potential merge conflicts if we wait much longer.

We should then open specialized PR. First, one PR to add support for recovery. Probably another to verify the "app limited" detection and reaction. And another for DCubic, with additional tests to simulate competition on the same link between Cubic (plain) and Dcubic, and investigate potential issues.

[huitema]

There was only one open discussion, the need or not to compare w_cubic and w_reno when entering recovery. You were right, this test is not necessary. More precisely, it is not necessary as long as beta_cubic is larger than (-1 + sqrt(1+4))/2, about 0.618033988749895. I fixed the code accordingly (commit in the PR branch).

I think we should merge this PR now. I will let you take a look at the changes and do it.

[hfstco]

Ready to merge from my side.

[huitema]

Did a final review. Entered issue #1823 to track implementation of recovery. Time to merge this great piece of work.