cvs commit: src/sys/kern uipc_usrreq.c

[Robert Watson]

On Thu, 10 Jun 2004, Alfred Perlstein wrote:

> >   - Use of an sx lock for the file list mutex may cause problems with regard
> >     to unp_mtx when garbage collection passed file descriptors.
> 
> That is true, but I don't think there is a reason to hold the UNP lock
> inside of unp_gc except to protect the unp_gcing variable and when
> calling into unp_scan. 
> 
> You might be able to gloss over all of it by just using an sx lock
> instead of a mutex for the UNP lock. 

Yeah.  Most of it is pretty reasonable, but the unp_gc() bit is both one
of the more sensitive bits of our kernel code, and one of the harder bits
to handle lockingwise as it tends to reach across a lot of layers.  George
suggested using an sx lock for unp_gcing, actually, which I think is a
good idea.  I'm going to have my hands off the UNIX domain socket code
except for merging socket buffer locking, so if this is something you're
interested in looking at, that would be wonderful.  My current plan is to
merge the basic first pass of the locking code, then revisit a lot of the
details.

> >   - The locking in unp_pcblist() for sysctl monitoring is correct subject to
> >     the unpcb zone not returning memory for reuse by other subsystems
> >     (consistent with similar existing concerns).
> >   
> >   - Sam's version of this change, as with the BSD/OS version, made use of
> >     both a global lock and per-unpcb locks.  However, in practice, the
> >     global lock covered all accesses, so I have simplified out the unpcb
> >     locks in the interest of getting this merged faster (reducing the
> >     overhead but not sacrificing granularity in most cases).  We will want
> >     to explore possibilities for improving lock granularity in this code in
> >     the future.
> 
> I noticed this in the BSD/os version, it was sort of like...  "the
> global lock covers everything, what's the point of the underlying
> locks..?" 

Well, I think the goal here was similar to that in the inpcbinfo code: use
a global lock as a pseudo-refcount/holder to prevent GC'ing of the objects
during use.  In the TCP/UDP code, the macros offer a reader/writer model,
but in practice it's implemented with a mutex.  Presumably their intent
was to get the basic bits done, then do a lot of profiling to decide
whether the added cost of reader/writer was worth it, whether to move more
towards true reference counting, per-cpu, etc.  However, I ended up
removing that before merging to CVS because there didn't appear to be any
resolution of the lock order concern between two unpcb mutexes.

> Anyhow, good work, but the unp_gc stuff is surely going to bite us and
> needs to be fixed. 

Agreed.  If the caller doesn't mind us dropping the unp_mtx, the first
logical thing to try is to replace unp_gcing with an sx lock (or the
like), and only acquire unp_mtx when calling into code that needs it. 

Robert N M Watson             FreeBSD Core Team, TrustedBSD Projects
robert at fledge.watson.org      Senior Research Scientist, McAfee Research