Clean up unused bandwidth entry in the TCP hostcache
ClosedPublic
Actions

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

Details

Reviewers

hiren
rrs

Group Reviewers

transport

Commits

rS292087: Clean up unused bandwidth entry in the TCP hostcache.

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 - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

j-nitrology.com updated this revision to Diff 10167.Nov 13 2015, 9:54 PM

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.

Herald added a subscriber: imp. · View Herald TranscriptNov 13 2015, 9:54 PM

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.

Looks good to me :-)

kbowling added a subscriber: kbowling.Nov 13 2015, 10:59 PM

Closed by commit rS292087: Clean up unused bandwidth entry in the TCP hostcache. (authored by hiren). · Explain WhyDec 11 2015, 6:23 AM

This revision was automatically updated to reflect the committed changes.

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

Herald added a reviewer: transport. · View Herald TranscriptJan 25 2018, 12:01 PM

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.