Re: Do the pidhashtbl_locks added by r340742 need to be sx locks?

Mateusz Guzik wrote:
>On 4/11/19, Rick Macklem <rmacklem_at_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.
Long ago when I first did the NFSv4 code for OpenBSD2.6, I had a callback function
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_common(),
   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 look
at doing it that way.
To be honest, since the current code works fine and can be difficult to test 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 valid
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 to
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 you
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