Defer inpcb deletion until after a grace period has elapsed
ClosedPublic
Actions

Authored by mmacy on May 21 2018, 6:08 PM.

Details

Reviewers

jtl
rrs
gallatin
rwatson

Commits

rS335015: Defer inpcb deletion until after a grace period has elapsed

Summary

I believe that this fixes at least the issue raised in the revert request.

Test Plan

pho + LLNW canary

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mmacy created this revision.May 21 2018, 6:08 PM

Herald added a subscriber: imp. · View Herald TranscriptMay 21 2018, 6:08 PM

Harbormaster completed remote builds in B16737: Diff 42803.May 21 2018, 6:08 PM

mmacy added a reviewer: gallatin.May 21 2018, 6:10 PM

mmacy added subscribers: kbowling, shurd, sbruno.

jtl added a reviewer: rwatson.May 21 2018, 6:21 PM

Having stared at this a bit, I generally think its fine. In fact, in some aspects it is an improvement over what it replaces.

Personally, I can't see any obvious locking/lifecycle assumptions that are violated that could cause problems, but would appreciate other eyes staring at it.

sys/netinet/in_pcb.c
1353 ↗	(On Diff #42803)	It's not entirely clear to me what this comment means. Therefore, I wonder if this comment is still placed correctly after this change?
1373 ↗	(On Diff #42803)	Its a small nit (and has existed for a while), but I believe this needs to always return non-0 in this context. A zero return value would indicate a bug. So, this should probably be asserted to return non-zero for INVARIANTS.
1409 ↗	(On Diff #42803)	Since in_pcbremlists also increments the gencnt, this results in a double gencnt change. It looks like this was added 20 years ago (r36079). Why don't we fix this 20-year-old bug? :-)

This revision is now accepted and ready to land.May 21 2018, 6:59 PM

mmacy added inline comments.May 21 2018, 7:13 PM

sys/netinet/in_pcb.c
1353 ↗	(On Diff #42803)	Yes. It can go.
1373 ↗	(On Diff #42803)	You're right. Even if refcount is > 0 the INP_FREED check will cause it to return 1.
1409 ↗	(On Diff #42803)	Coming right up.

gallatin accepted this revision.May 21 2018, 10:04 PM

Looking closely at this and thinking about tcp_hpts.c, if I grok this correctly
this should all integrate ok. Assuming that someone calls inp_pcbfree() on
something that is in hpts, even if the epoch_call happens, the inpcb won't
be freed until the hpts's main loop see the INP_FREED flag and does
its in_pcbrele_wlocked() call at which time the INP will finally get destroyed.

I am not as familiar with your epoch system yet
(though it reminds me of something I did at Adara). I would suggest you
test this with the hpts system and INVARIANTS on to see if my
supposition is true. If you don't have access to the rack or bbr code
let me know I can provide you with it. I am a bit behind due to meetings
with google getting the phabricator up with rack in it which would have made
it a bit easier :-o