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

[Gedare Bloom]

On Wed, Jul 28, 2021 at 5:58 PM Chris Johns <chrisj at rtems.org> wrote:
>
> 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?
>
I'd suggest a comment in there or so, just to clarify that we don't
support shared locks, and that this implementation would need to be
improved if so.

> Chris