Page MenuHomeFreeBSD

tcp: Mbuf leak while holding a socket buffer lock
ClosedPublic

Authored by rrs on Jun 9 2021, 6:09 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 25, 1:57 PM
Unknown Object (File)
Oct 6 2024, 9:55 PM
Unknown Object (File)
Oct 3 2024, 7:35 PM
Unknown Object (File)
Oct 3 2024, 1:54 PM
Unknown Object (File)
Oct 3 2024, 7:19 AM
Unknown Object (File)
Oct 1 2024, 7:10 PM
Unknown Object (File)
Oct 1 2024, 4:17 PM
Unknown Object (File)
Oct 1 2024, 4:01 PM
Subscribers

Details

Summary

When running at NF the current Rack and BBR changes with the recent
commits from Richard that cause the socket buffer lock to be held over
the ip_output() call and then finally culminating in a call to tcp_handle_wakeup()
we get a lot of leaked mbufs. I don't think that this leak is actually caused
by holding the lock or what Richard has done, but is exposing some other
bug that has probably been lying dormant for a long time. I will continue to
look (using his changes) at what is going on to try to root cause out the issue.

In the meantime I can't leave the leaks out for everyone else. So this commit
will revert all of Richards changes and move both Rack and BBR back to just
doing the old sorwakeup_locked() calls after messing with the so_rcv buffer.

We may want to look at adding back in Richards changes after I have pinpointed
the root cause of the mbuf leak and fixed it.

Test Plan

After making the changes validate that
a) Things are waking up <and>
b) No mbuf leak occurs.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

rrs requested review of this revision.Jun 9 2021, 6:09 PM
rrs added reviewers: tuexen, rscheff.

Oh I almost forgot the other change here, i.e. the centralization
of counting bytes sent and bytes retransmitted. This is an essential
thing for hardware TLS. That way hardware TLS can have a place
they can notice a connection is retransmitting a large amount
and "remove" that connection from using hardware and instead
use software.

Are the pure whitespace changes intended?

This comment was removed by rscheff.

forget my last comment, I was looking on the wrong side of the screen or whatever. looks good.

This revision is now accepted and ready to land.Jun 9 2021, 6:58 PM

Yes the pure whitespace changes are intended..