Date: Thu, 11 Apr 2019 03:49:31 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: Rick Macklem <rmacklem@uoguelph.ca> Cc: "kib@freebsd.org" <kib@freebsd.org>, "freebsd-current@FreeBSD.org" <freebsd-current@freebsd.org> Subject: Re: Do the pidhashtbl_locks added by r340742 need to be sx locks? Message-ID: <CAGudoHGOyQVjrxt=555wsC_VQruZBnrA_jMjnXpLN8N9V%2B7-Xw@mail.gmail.com> In-Reply-To: <QB1PR01MB35371458E0E88BA357633D50DD2F0@QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM> References: <QB1PR01MB35371458E0E88BA357633D50DD2F0@QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM>
next in thread | previous in thread | raw e-mail | index | archive | help
On 4/11/19, Rick Macklem <rmacklem@uoguelph.ca> wrote: > Hi, > > I finally got around to looking at what effect replacing pfind_locked() > with > pfind() has for the NFSv4 client and it is broken. > > The problem is that the NFS code needs to call some variant of "pfind()" > while > holding a mutex lock. The current _pfind() code uses the pidhashtbl_locks, > which are "sx" locks. > > There are a few ways to fix this: > 1 - Create a custom version of _pfind() for the NFS client with the sx_X() > calls > removed, plus replace the locking of allproc_lock with locking of all > the > pidhashtbl_locks, so that the "sx" locks are acquired before the > mutex. > --> Not very efficient, but since it is only done once/sec, I can live > with it. > 2 - Similar to the above, but still lock the allproc_lock and use a loop of > FOREACH_PROC_IN_SYSTEM(p) instead of a hash list for the pid in the > custom pfind(). (I don't know if this would be preferable to locking > all > the pidhashtbl_locks for other users of pfind()?) > 3 - Convert the pidhashtbl_locks to mutexes. Then the NFS client doesn't > need > to acquire any proc related locks and it just works. > I can't see anywhere that "sleeps" while holding the pidhashtbl_locks, > so I > think they can be converted, although I haven't tried it yet? > > From my perspective, #3 seems the better solution. > What do others think? > Changing the lock type to rwlock may be doable and worthwhile on its own, but I don't think it would constitute the right fix. Preferably there would be an easy to use mechanism which allows registering per-process callbacks. Currently it can be somewhat emulated with EVENTHANDLERs, but it would give calls for all exiting processes, not only the ones of interest. Then there would be no need to periodically scan as you would always get notified on process exit. Note the current code does not ref processes it is interested in any manner and just performs a timestamp check to see if it got the one it expected (with pid reuse in mind). So I think a temporary hack which will do the trick will take the current approach further: rely on struct proc being type-stable (i.e. never being freed) and also store the pointer. You can always safely PROC_LOCK it, do checks to see the proc is alive and has the right timestamp, all without needing to pfind or similar. -- Mateusz Guzik <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAGudoHGOyQVjrxt=555wsC_VQruZBnrA_jMjnXpLN8N9V%2B7-Xw>