Page MenuHomeFreeBSD

sockets: let protocols be responsible for socket buffer mutexes
ClosedPublic

Authored by glebius on Fri, Jan 30, 9:23 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Feb 3, 7:31 AM
Unknown Object (File)
Mon, Feb 2, 1:17 PM
Unknown Object (File)
Mon, Feb 2, 12:35 PM
Unknown Object (File)
Sun, Feb 1, 3:34 AM
Unknown Object (File)
Sat, Jan 31, 11:04 AM
Unknown Object (File)
Sat, Jan 31, 7:58 AM
Unknown Object (File)
Sat, Jan 31, 6:11 AM
Unknown Object (File)
Fri, Jan 30, 10:36 PM

Details

Summary

Sockets that implement their own socket buffers (marked with PR_SOCKBUF)
are now also responsible for initialization of socket buffer mutexes in
pr_attach and for destruction in pr_detach (or pr_close).

This removes a big bunch of reported LORs, as now WITNESS is able to see
that tcp(4) socket buffer mutex and netlink(4) socket buffer mutex are two
different things. Distinct names also improve diagnostics for blocked
threads.

This also removes a hack from unix(4), where we used to mtx_destroy().
Also removes an innocent bug from unix(4) where for accept(2)-ed socket
soreserve() was called twice. This one was innocent since first call to
soreserve() was asking for 0 bytes of space.

This slightly increased amount of pasted code in TCP's syncache_socket().
The problem is that while for sockets created with socket(2) it is
pr_attach responsible for call to soreserve() (including !PR_SOCKBUF
protocols), but for the sockets created with accept(2) it was
solisten_clone() doing soreserve(), combined with the fact that for
accept(2) TCP completely bypasses pr_attach. This all should improve once
TCP has its own socket buffers.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

markj added inline comments.
sys/kern/uipc_socket.c
915

Why not use VNET_SO_ASSERT(so)?

924

Not directly relevant, but it's a bit silly that soreserve() has to lock the socket buffers here. At this point there is no possible concurrent access, so I'm not sure why locking is important.

sys/kern/uipc_usrreq.c
509

A socket buffer lock is always going to be MTX_DEF, so it's a bit weird to make each socket type declare this. Cleaner IMO would be to just initialize rcvmtxopts = 0, set rcvmtxopts |= MTX_DUPOK here, and mtx_init(..., MTX_DEF | rcvmtxopts) below.

  • Use rcvmtxopts as Mark suggests.
glebius added inline comments.
sys/kern/uipc_socket.c
915

This is pretty useless macro, as it doesn't use the so argument in the assertion. It is declared only in this file and doesn't have much useful use. I actually want to get rid of it. All useful assertions in this file can be achieved with common VNET_ASSERT().

924

I totally agree, maybe will do that in future changes.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Feb 3, 5:10 PM
This revision was automatically updated to reflect the committed changes.