Hybla congestion control #1853
Hybla congestion control #1853
Conversation
|
Nice work. It is great to see implementations of a variety of congestion control algorithms in Picoquic. You should of course make sure that all the tests pass, but there is more. Before merging this work, I would like to have a framework so that we can test a large number of congestion control algorithms, while not inflating the size of executables built with the library. |
|
Please take a look at PR #1856 -- managing CC algorithms in a modular way. |
There was a problem hiding this comment.
Thanks for contributing an implementation of Hybla to picoquic. My main comment is related to the need for an extensible congestion control framework, and the effort in PR #1856. Reading your code, I see that this effort needs to be extended to provide a generic support for per algorithm parameters. Let's finish the "generic" part first, it will make your code simpler.
By the way, I don't see a test file "picoquictest/hybla_test.c" or any test case. How do we verify that your code works as expected?
| @@ -64,7 +64,8 @@ static option_table_line_t option_table[] = { | |||
| { picoquic_option_DisablePortBlocking, 'X', "disable_block", 0, "", "Disable the check for blocked ports"}, | |||
| { picoquic_option_SOLUTION_DIR, 'S', "solution_dir", 1, "folder", "Set the path to the source files to find the default files" }, | |||
| { picoquic_option_CC_ALGO, 'G', "cc_algo", 1, "cc_algorithm", | |||
| "Use the specified congestion control algorithm: reno, cubic, bbr or fast. Defaults to bbr." }, | |||
| "Use the specified congestion control algorithm: reno, cubic, bbr, fast, or hybla. Defaults to bbr." }, | |||
| { picoquic_option_HYBLA_RTT0, 'H', "rtt0", 1, "number", "Set RTT0 parameter for Hybla congestion control."}, | |||
| { picoquic_option_SPINBIT, 'P', "spinbit", 1, "number", "Set the default spinbit policy" }, | |||
There was a problem hiding this comment.
Please take a look at PR #1856 which removes the need to manually update that message.
| picoquic_hybla_set_rtt0_param(v); | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
I understand that you need a parameter, but I would rather try to find a generic way to pass parameters to CC algorithms. Maybe something like "-g hybla:360".
I would also need some clarity on whether this parameter is required at both clients and servers, or just on one side of the connection. Is it possible to set a default value, and see it use by all connections served by a given server?
There was a problem hiding this comment.
I can see how providing a Hybla-specific parameter as a Picoquic option can be a bit intrusive. As for your question, RTT0 is meant to be a one-sided parameter that influences how the local node calculates the congestion window.
There was a problem hiding this comment.
My plan is to add a "string" (text encoded parameter) after the algorithm ID, using maybe a colon as a separator. Then each algorithm can encode whatever they want in that string, or nothing. The parameter would be passed in the "init" call when a new congestion control context starts.
I now people already want something like that for experimental changes to BBR.
| * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS | ||
| * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. | ||
| */ | ||
|
|
There was a problem hiding this comment.
No objection to listing as the author, but how does this copyright notice affect the distribution to picoquic users?
There was a problem hiding this comment.
All source files have the same comment at the beginning of the file, so I figured I'd do the same in hybla.c, but I didn't know if it was appropriate to add the "Private Octopus, Inc." part. I don't intend to put any license restrictions on my work, so I will include that part.
|
|
||
| // Functions to set Hybla-specific parameters | ||
| void picoquic_hybla_set_rtt0_param(int rtt0); | ||
| void picoquic_hybla_set_initial_ssthresh_param(uint64_t initial_ssthresh); | ||
|
|
There was a problem hiding this comment.
Please check PR #1856. I need to reduce the number of functions and externals listed in picoquic.h.
|
Sorry for the delayed response, I'll soon be working on ensuring all tests pass, and I'll implement a hybla_test.c file. I'll be looking into PR #1856 as well. |
I implemented a Picoquic version of the TCP Hybla congestion control algorithm as part of my Master's final project. I thought you might be interested in including it among the other CC algorithms.
Hybla is a variant of NewReno designed for high-RTT paths. The NewReno cwin normally increases slower the higher the RTT, so Hybla is designed to compensate for that through specific multiplicative factors.
Some resources: