Ok, updated patch:
https://patch-diff.githubusercontent.com/raw/thorpej/netbsd-src/pull/5.diff
<https://patch-diff.githubusercontent.com/raw/thorpej/netbsd-src/pull/5.diff>
I went an used a simple hash table with 32 buckets. Seems good enough for now.
Since the “pshared” IDs are random anyway, I didn’t bother with any exotic
hash function — just extract some of the random bits to use as the bucket index.
I also added some test cases to exercise the basic lifecycle of the ksem
objects.
I plan to check this in later today.
> On Feb 2, 2019, at 11:44 AM, Jason Thorpe <[email protected]> wrote:
>
>
>> On Feb 2, 2019, at 10:33 AM, Mindaugas Rasiukevicius <[email protected]>
>> wrote:
>>
>> Jason Thorpe <[email protected]> wrote:
>>> Patch is here:
>>> https://patch-diff.githubusercontent.com/raw/thorpej/netbsd-src/pull/5.diff
>>
>> Thanks for working on this.
>>
>> - Why not just add a new syscall, instead of the KSEM_PSHARED stuff?
>
> Mainly because a new syscall is more intrusive, and harder to back-port.
> It’s a shame the existing system call didn’t have a way to pass flags up.
>
>> - If you add the 'fd' parameter to ksem_release(), then you can move
>> fd_putfile() there too (with fd != -1 case) and simplify a little bit.
>
> I’ll take a look. There are a few spots where I think the pattern still
> needs to “leak” out of ksem_release().
>
>> - ksem_alloc_pshared_id: can ~KSEM_MARKER_MASK range be exhausted as an
>> attach vector?
>
> You’d have to allocate a ton of pshared semaphores … it’s a 23-bit range, and
> that definitely exceeds the system max semaphores limit. I assign it
> randomly only to make it harder to guess that the in-use semaphore IDs are
> (mainly as a way to avoid bad behavior caused by buggy programs).
>
> (As an aside, the POSIX semaphore API is … annoyingly underspecified —
> there’s a lot of room for implementation-defined behavior that the spec
> doesn’t even bother to call out as “implementation-defined”).
>
>> - Can you eliminate ksem_insert_pshared_locked(),
>> ksem_remove_pshared_locked()
>> and ksem_remove_pshared() wrappers? They serve no purpose.
>
> I initially did that just to hide the “what kind of data structure contains
> the objects” away from the main logic. I’ll go ahead and simplify, though.
>
>> - ksem_lookup_pshared_locked: it makes sense to use a hash table here.
>> Using hashinit() is clumsy nowadays.. personally, I would replace most
>> of the subr_hash.c use cases with something like rhashmap [1] and much
>> more convenient get/put/del semantics. Anyway, that is off-topic, so
>> up to you if you want to bother.
>
> It does make sense to use something else .. I thought about using an r-b
> tree. Given the default system limits, we’re talking about a small number of
> objects, but yah, I’ll use something better there.
>
> -- thorpej
>
-- thorpej
