Page MenuHomeFreeBSD

Clean up unused bandwidth entry in the TCP hostcache
ClosedPublic

Authored by j-nitrology.com on Nov 13 2015, 9:54 PM.

Details

Summary

The bandwidth calculation that was saved into the hostcache was removed by r211315, and r212765 marked it unused in tcp_info as it was found conflicting with the new modular congestion control. Clean up the remnants of this in the hostcache.

Test Plan

This has been tested on production boxes within Limelight.

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

j-nitrology.com retitled this revision from to Clean up unused bandwidth entry in the TCP hostcache.
j-nitrology.com updated this object.
j-nitrology.com edited the test plan for this revision. (Show Details)
j-nitrology.com added a reviewer: hiren.
hiren accepted this revision.EditedNov 13 2015, 10:09 PM
hiren edited edge metadata.

Looks good to me.

Only thing I am not sure about is if we want to keep struct size unchanged and also keep a spare field where we removed the bandwidth field in struct hc_metrics and hc_metrics_lite. On a second thought, I can't think of anything that I'd want to add to hostcache at this point so might as well remove them. And as I understand, MFC is not planned for this.

It'd be great if someone else can also review the change.

Thank you, Jason.

This revision is now accepted and ready to land.Nov 13 2015, 10:09 PM
hiren edited edge metadata.Nov 13 2015, 10:13 PM
hiren edited subscribers, added: network, transport; removed: imp.
rrs accepted this revision.Nov 13 2015, 10:53 PM
rrs added a reviewer: rrs.
rrs added a subscriber: rrs.

Looks good to me :-)

This revision was automatically updated to reflect the committed changes.
julian added a subscriber: julian.Jan 25 2018, 12:01 PM

dammit we have a functional bandwidth estimator that used this field..

If it could be useful to others, might you commit it?

This was committed over 2 years ago. So that sounds like a pretty local mod. I would recommend either carrying it in your local tree or otherwise upstreaming the estimator.

bz added a subscriber: bz.Jan 25 2018, 8:11 PM
This comment was removed by bz.
In D4154#295111, @kevin.bowling_kev009.com wrote:

This was committed over 2 years ago. So that sounds like a pretty local mod. I would recommend either carrying it in your local tree or otherwise upstreaming the estimator.

that's what we are doing (just adding the fields back locally..
The estimator is, unfortunately, a bit crude for public consumption, or I'd commit it.

Contact me privately if you (anyone) needs a per-peer bandwidth estimation feature.