Date: Thu, 10 Jun 2004 22:16:51 -0400 (EDT) From: Robert Watson <rwatson@freebsd.org> To: Alfred Perlstein <alfred@freebsd.org> Cc: cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/kern uipc_usrreq.c Message-ID: <Pine.NEB.3.96L.1040610215435.44958C-100000@fledge.watson.org> In-Reply-To: <20040611012513.GI78955@elvis.mu.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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@fledge.watson.org Senior Research Scientist, McAfee Research
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.NEB.3.96L.1040610215435.44958C-100000>