Date: Thu, 11 Apr 2019 04:41:34 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Mateusz Guzik <mjguzik@gmail.com> 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: <QB1PR01MB35374204FA4B6764F34BFA23DD2F0@QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <CAGudoHGOyQVjrxt=555wsC_VQruZBnrA_jMjnXpLN8N9V%2B7-Xw@mail.gmail.com> References: <QB1PR01MB35371458E0E88BA357633D50DD2F0@QB1PR01MB3537.CANPRD01.PROD.OUTLOOK.COM>, <CAGudoHGOyQVjrxt=555wsC_VQruZBnrA_jMjnXpLN8N9V%2B7-Xw@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
Mateusz Guzik wrote:
>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_lock=
s,
>> 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 lockin=
g
>> 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_loc=
ks,
>> 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.
Long ago when I first did the NFSv4 code for OpenBSD2.6, I had a callback f=
unction
pointer in "struct proc" which the NFS code set non-null to get a callback.
{ The code still has remnants of that because it still has nfscl_cleanup_co=
mmon(),
which was code shared by that callback and this approach which was used =
for
the Mac OS X port, where I couldn't change "struct proc". }
I have never added anything like that for FreeBSD, but I suppose we could l=
ook
at doing it that way.
To be honest, since the current code works fine and can be difficult to tes=
t well,
I hesitate to change to using a callback.
>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...
Hmm, so you are saying that every element of the proc_zone always has a val=
id
p_mtx field in it that can be safely PROC_LOCK()'d no matter if the element
refers to a process at that time?
I would also need help with the code to determine if the structure refers t=
o
a process that currently exists with the same pid and creation time.
I suppose saving "p" with the lock/open owner string and then doing what yo=
u
suggest is possible, but it would take some work.
For now, I can just grab all the pidhashtbl_locks once/sec and fix head so =
it works.
rick
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?QB1PR01MB35374204FA4B6764F34BFA23DD2F0>