Don't require the socket lock for sorele().
ClosedPublic
Actions

Authored by jhb on Oct 29 2021, 9:10 PM.

Details

Reviewers

mjg
markj
kib

Group Reviewers

transport

Commits

rGe3ba94d4f34e: Don't require the socket lock for sorele().

Summary

Previously, sorele() always required the socket lock and dropped the
lock if the released reference was not the last reference. Many
callers locked the socket lock just before calling sorele() resulting
in a wasted lock/unlock when not dropping the last reference.

Move the previous implementation of sorele() into a new
sorele_locked() function and use it instead of sorele() for various
places in uipc_socket.c that called sorele() while already holding the
socket lock.

The sorele() macro now uses refcount_release_if_not_last() try to drop
the socket reference without locking the socket. If that shortcut
fails, it locks the socket and calls sorele_locked().

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 42442
Build 39330: arc lint + arc unit

Event Timeline

jhb created this revision.Oct 29 2021, 9:10 PM

Herald added a subscriber: imp. · View Herald TranscriptOct 29 2021, 9:10 PM

jhb requested review of this revision.Oct 29 2021, 9:10 PM

Harbormaster completed remote builds in B42442: Diff 97687.Oct 29 2021, 9:10 PM

jhb added a child revision: D32742: Use sorele_unlocked()..Oct 29 2021, 9:10 PM

Did you consider just changing the semantics of sorele()? Every user of this macro outside of uipc_socket.c contains the pattern SOCK_LOCK(so); sorele(so). Then, uipc_socket.c can use a private routine to drop a ref on a locked socket.

kib accepted this revision.Oct 29 2021, 9:18 PM

This revision is now accepted and ready to land.Oct 29 2021, 9:18 PM

In D32741#738872, @markj wrote:

Did you consider just changing the semantics of sorele()? Every user of this macro outside of uipc_socket.c contains the pattern SOCK_LOCK(so); sorele(so). Then, uipc_socket.c can use a private routine to drop a ref on a locked socket.

Well, I worried that would be more of a pain with having the API differ between versions (not sure if there are any out-of-tree modules that might use this, but perhaps not?)

It is true that having sorele_locked() and sorele() is probably more typical of the API names we use.

In D32741#738889, @jhb wrote:

In D32741#738872, @markj wrote:

Did you consider just changing the semantics of sorele()? Every user of this macro outside of uipc_socket.c contains the pattern SOCK_LOCK(so); sorele(so). Then, uipc_socket.c can use a private routine to drop a ref on a locked socket.

Well, I worried that would be more of a pain with having the API differ between versions (not sure if there are any out-of-tree modules that might use this, but perhaps not?)

It is true that having sorele_locked() and sorele() is probably more typical of the API names we use.

I'm not aware of anything in the ports tree that makes any real use of the socket API.

In D32741#738900, @markj wrote:

In D32741#738889, @jhb wrote:

In D32741#738872, @markj wrote:

Did you consider just changing the semantics of sorele()? Every user of this macro outside of uipc_socket.c contains the pattern SOCK_LOCK(so); sorele(so). Then, uipc_socket.c can use a private routine to drop a ref on a locked socket.

Well, I worried that would be more of a pain with having the API differ between versions (not sure if there are any out-of-tree modules that might use this, but perhaps not?)

It is true that having sorele_locked() and sorele() is probably more typical of the API names we use.

I'm not aware of anything in the ports tree that makes any real use of the socket API.

If you think it's safe to change the API, then I could make the new one "sorele" and make the old one "sorele_locked"? Note that the new macro still needs to call the old one to drop the last reference, so it can't be a static function in uipc_socket.c (though perhaps sorele_locked() could be a public function in uipc_socket.c?)

In D32741#740268, @jhb wrote:

In D32741#738900, @markj wrote:

In D32741#738889, @jhb wrote:

In D32741#738872, @markj wrote:

Did you consider just changing the semantics of sorele()? Every user of this macro outside of uipc_socket.c contains the pattern SOCK_LOCK(so); sorele(so). Then, uipc_socket.c can use a private routine to drop a ref on a locked socket.

Well, I worried that would be more of a pain with having the API differ between versions (not sure if there are any out-of-tree modules that might use this, but perhaps not?)

It is true that having sorele_locked() and sorele() is probably more typical of the API names we use.

I'm not aware of anything in the ports tree that makes any real use of the socket API.

To be clear, I extracted all sources for kmods in the ports tree and searched for sorele(). Nothing turns up.

If you think it's safe to change the API, then I could make the new one "sorele" and make the old one "sorele_locked"? Note that the new macro still needs to call the old one to drop the last reference, so it can't be a static function in uipc_socket.c (though perhaps sorele_locked() could be a public function in uipc_socket.c?)

I think it'd be good to have an out-of-line sorele_locked() in uipc_socket.c.

jhb retitled this revision from Add sorele_unlocked(). to Don't require the socket lock for sorele()..Nov 5 2021, 9:20 PM

jhb edited the summary of this revision. (Show Details)