[PATCH rtems-libbsd 3/7] sys/kern: Add lockmgr support

[Chris Johns]

On 29/7/21 9:42 am, Gedare Bloom wrote:
> On Wed, Jul 28, 2021 at 5:05 PM Chris Johns <chrisj at rtems.org> wrote:
>> On 29/7/21 12:03 am, Gedare Bloom wrote:
>>> On Tue, Jul 27, 2021 at 2:59 AM <chrisj at rtems.org> wrote:
>>>> From: Chris Johns <chrisj at rtems.org>
>>>> +static int
>>>> +lockmgr_upgrade(struct lock *lk, u_int flags, struct lock_object *ilk,
>>>> +               const char *file, int line)
>>>> +{
>>>> +  uintptr_t x, v;
>>>> +  int error = 0;
>>>> +  LOCK_LOG_LOCK("XUPGRADE", &lk->lock_object, 0, 0, file, line);
>>>> +  lockmgr_trace("xupgrade", 'I', lk);
>>>> +  v = lk->lk_lock;
>>>> +  x = v & ~LK_SHARE;
>>>> +  atomic_store_rel_ptr(&lk->lk_lock, x);
>>>
>>> I think this function is to upgrade a shared (waiters/spinner) lock to
>>> non-shared (spinner) lock? I'm not confident about correctness here.
>>> It looks to me like there can be a race condition if multiple waiters
>>> attempt to upgrade in parallel. It works by a preemption after the
>>> first waiter loads `v = lk->lk_lock` and then another waiter loads
>>> `v`. Actually, there doesn't seem to be anything that prevents two
>>> separate calls to upgrade the lock by different waiters in sequence
>>> either. I could be wrong (of course).
>>
>> We do not support shared locks and we do not share specific paths through locks
>> so while the code may request a shared lock it is not shared.
>>
> ok that is helpful.
> 
>> You could be correct and I welcome you reviewing the code in the FB kernel this
>> is based on.
>>
> I took a quick look.

Thanks

> the FB kernel uses spinning and
> atomic_fcmpset_ptr instead, which I think exactly addresses this
> problem.

Is this something we should do or is this something we should not be worried about?

Chris