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

[Chris Johns]

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>
>>
>>  struct lock_object {
>> -#ifndef __rtems__
>>         const   char *lo_name;          /* Individual lock name. */
>>         u_int   lo_flags;
>>         u_int   lo_data;                /* General class specific data. */
>> +#ifndef __rtems__
>>         struct  witness *lo_witness;    /* Data for witness. */
>> -#else /* __rtems__ */
>> -       unsigned int lo_flags;
>> +#endif /* __rtems__ */
>> +#ifdef __rtems__
> Can use
> #else /* __rtems__ */
> instead of endif+ifdef

I will see how this ends up after looking at the fields Sebastian wants removed.

>> +static void
>> +lock_lockmgr(struct lock_object *lock, uintptr_t how)
>> +{
>> +  panic("lockmgr locks do not support sleep interlocking");
>> +}
>> +
>> +static uintptr_t
>> +unlock_lockmgr(struct lock_object *lock)
>> +{
>> +  panic("lockmgr locks do not support sleep interlocking");
>> +}
>> +
> Should there be different messages for the two cases here?

Sure. Either case means something is being used by new ported code and these
calls need to be looked into.

> 
>> +static struct thread *
>> +lockmgr_xholder(const struct lock *lk)
>> +{
>> +  uintptr_t x;
>> +  x = lk->lk_lock;
>> +  if ((x & LK_SHARE))
>> +    return NULL;
>> +  return rtems_bsd_get_thread(lk->lock_object.lo_mtx.queue.Queue.owner);
>> +}
>> +
>> +static void
>> +lockmgr_exit(u_int flags, struct lock_object *ilk, int wakeup_swapper)
> any good reason to use ilk instead of lk as elsewhere?

None. I copy the decl from FreeBSD ...

freebsd/sys/kern/kern_lock.c:lockmgr_exit(u_int flags, struct lock_object *ilk,
int wakeup_swapper)

>> +static int
>> +lockmgr_slock_hard(struct lock *lk, u_int flags, struct lock_object *ilk,
>> +                  const char *file, int line)
>> +{
>> +  uintptr_t x;
>> +  int error = 0;
>> +  lockmgr_trace("slock", 'I', lk);
>> +  rtems_bsd_mutex_lock(&lk->lock_object);
>> +  x = lk->lk_lock;
>> +  atomic_store_rel_ptr(&lk->lk_lock, x + LK_ONE_SHARER);
>> +  if (rtems_bsd_mutex_recursed(&lk->lock_object))
>> +    lk->lk_recurse++;
>> +  else
>> +    lk->lk_recurse = 0;
>> +  lockmgr_trace("slock", 'O', lk);
>> +  LOCK_LOG_LOCK("SLOCK", &lk->lock_object, 0, lk->lk_recurse, file, line);
>> +  lockmgr_exit(flags, ilk, 0);
>> +  return error;
> this is always 0 (successful)?

Yeap.

>> +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.

You could be correct and I welcome you reviewing the code in the FB kernel this
is based on.

>> +int
>> +lockstatus(const struct lock *lk)
>> +{
>> +  uintptr_t v, x;
>> +  int ret;
>> +
>> +  ret = LK_SHARED;
>> +  x = lk->lk_lock;
>> +
>> +  if ((x & LK_SHARE) == 0) {
>> +    v = rtems_bsd_mutex_owned(RTEMS_DECONST(struct lock_object*, &lk->lock_object));
>> +    if (v)
>> +      ret = LK_EXCLUSIVE;
>> +    else
>> +      ret = LK_EXCLOTHER;
>> +  } else if (x == LK_UNLOCKED) {
>> +    ret = 0;
>> +  }
> 
> there's no else {} here, is it possible that (x & LK_SHARE) != 0, and
> (x != LK_UNLOCKED)? If so, ret may be returned without initialization?
> 
> Maybe just initialize ret = 0.

Yes and thanks.

Chris