tcp_hostcache: ensure expire and prune values stay consistent with each other
ClosedPublic
Actions

Authored by rscheff on Thu, Jul 23, 7:09 PM.

Details

Reviewers

tuexen
dch
glebius
peter.lei_ieee.org

Group Reviewers

transport

Commits

rG2d01998e9e8a: tcp_hostcache: ensure expire and prune values stay consistent with each other

Summary

While getting more information around PR296474 it was found that existing
configurations can end up with unexpectedly long pruning intervals, which
defy an explict short expire timeout of the hostcache entries.

When configuring the expire timeout to something short, make sure that the
prune time runs at least at that interval. Similarly, when adjusting the
prune interval up, ensure the expire timeout reflect that expected minimum
time also.

MFC after 3 days alongside D58360

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 75131
Build 72014: arc lint + arc unit

Event Timeline

rscheff created this revision.Thu, Jul 23, 7:09 PM

Herald added a reviewer: transport. · View Herald TranscriptThu, Jul 23, 7:09 PM

Herald added subscribers: melifaro, imp. · View Herald Transcript

rscheff requested review of this revision.Thu, Jul 23, 7:09 PM

Harbormaster completed remote builds in B75057: Diff 182554.Thu, Jul 23, 7:09 PM

pouria added a subscriber: pouria.Thu, Jul 23, 7:19 PM

pouria added inline comments.

sys/netinet/tcp_hostcache.c
184	Can we use UINT here?
636
638–639	Maybe use u_int?

also restart the callout so the next pruning will occur at the expected time

Harbormaster completed remote builds in B75131: Diff 182706.Sat, Jul 25, 8:04 PM

I agree that switching to unsigned for time intervals should be more correct. But the struct tcp_hostcache members now are int and that makes the sysctls to be signed. If structure members are also changed, a more thorough review of all the code that uses expire and prune needed. @rscheff since you already catched one difficult signedness bug in this code, maybe you can do such sweep, too?

make sysctl configurables uint

Harbormaster completed remote builds in B75227: Diff 182914.Tue, Jul 28, 7:30 AM

In D58424#1342278, @glebius wrote:

I agree that switching to unsigned for time intervals should be more correct. But the struct tcp_hostcache members now are int and that makes the sysctls to be signed. If structure members are also changed, a more thorough review of all the code that uses expire and prune needed. @rscheff since you already catched one difficult signedness bug in this code, maybe you can do such sweep, too?

Checking the use of purge, purgeall and expire, I agree with @pouria that those should be uint; their use is benign as they really only interface with userland / sysctl, and negative values for them are illogical. Validation handling for negatives is then handled by sysctl_handler_int already too.

Fix other sim

glebius accepted this revision.Tue, Jul 28, 5:39 PM

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Jul 28, 9:21 PM

Closed by commit rG2d01998e9e8a: tcp_hostcache: ensure expire and prune values stay consistent with each other (authored by rscheff). · Explain Why

This revision was automatically updated to reflect the committed changes.

rscheff added a commit: rG2d01998e9e8a: tcp_hostcache: ensure expire and prune values stay consistent with each other.