Page MenuHomeFreeBSD

Get TCP CDG CC working if net.inet.tcp.cc.cdg.smoothing_factor = 0
ClosedPublic

Authored by tuexen on Feb 3 2019, 1:09 PM.

Details

Summary

When using the CDG TCP congestion control, setting the sysctl variable net.inet.tcp.cc.cdg.smoothing_factor to 0 should disabled the smoothing. Right now it results in a panic due to a division by zero. This patch fixes this.

This problem was reported in PR 193762.

Test Plan

Run this packetdrill script, which triggers the bug:

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

tuexen created this revision.Feb 3 2019, 1:09 PM
rrs accepted this revision.Feb 4 2019, 4:25 PM
This revision is now accepted and ready to land.Feb 4 2019, 4:25 PM
lstewart accepted this revision.Feb 5 2019, 12:36 PM

Thank you for fixing this.

Minor nit: this change will leave any pending qdiffmin_q/qdiffmax_q entries from the qdiffsample UMA zone in limbo until the control block is destroyed. I think it's ok to ignore though.

Actually, we probably need to flush the qdiffmin_q and qdiffmax_q lists in the sample_q_size == 0 branch so that a change back to non-zero smoothing_factor doesn't start operating with stale data.

lstewart requested changes to this revision.Feb 5 2019, 12:53 PM

Suggest moving stailq flush loops out of cdg_cb_destroy() into an inline function, changing smoothing_factor sysctl to a SYSCTL_PROC with custom handler similar to the exp_backoff_scale sysctl, and calling flush function from both cdg_cb_destroy() and sysctl handler when smoothing_factor is set to zero

This revision now requires changes to proceed.Feb 5 2019, 12:53 PM
tuexen added a comment.EditedFeb 5 2019, 2:39 PM

Suggest moving stailq flush loops out of cdg_cb_destroy() into an inline function, changing smoothing_factor sysctl to a SYSCTL_PROC with custom handler similar to the exp_backoff_scale sysctl, and calling flush function from both cdg_cb_destroy() and sysctl handler when smoothing_factor is set to zero

If I read the code right, changing smoothing_factor does no impact any existing cc_var, but only future ones. So there is no need for an operation you suggested.

The only reason for a SYSCTL_PROC I see, is to enforce that smoothing_factor is between 0 and INT_MAX to avoid an overflow when assigning it to sample_q_size, which is declared as an int. But that is a different issue than addressing this division by zero error.

lstewart accepted this revision.Feb 7 2019, 9:08 PM

Agreed and apologies, I misread/miunderstood the code. I also touched base with one of the original CDG authors to sanity check and they agree this change is a good idea.

This revision is now accepted and ready to land.Feb 7 2019, 9:08 PM
This revision was automatically updated to reflect the committed changes.