Re: [PATCH] make rwlocks SHAREABLE + cleanups

> >
> > Why this check is not needed for the pshared case? The mx_owner is
> > always
> > set to current in pth_mutex_acquire() for both normal and pshared
> > mutexes.
> >
>
> I thought the pointers wouldn't have to be the same in different
> processes
> and address spaces.
>
> Process A aquires the lock - mutex->mx_owner=0xADDA
> Process B tries to release it (which is wrong) - ok, mx_owner is still
> NULL in
> _this_ address space
>
> I got that wrong.
>

The mutex is common and shared among different processes under pshared
case. So, when process A acquires it, the mx_owner will get set. Now, if
process B tries to release it, the mx_owner is not same as it's current
and returns EPERM. This check is necessary for that reason.


> For EDEADLK we have to test if the lock is already held by the caller -
> but
> only if the mutex is not PTHREAD_MUTEX_RECURSIVE.
>

Correct. See the code below.

> Does the check belongs here?
> pth_sync.s:239
> wait:
>      /* already locked by caller? */
>      if (mutex->mx_count >= 1 && mutex->mx_owner == current &&
>      mutex->mx_owner_pid==descr->pid &&
>      mutex->mx_type == PTH_MUTEX_RECURSIVE_NP) {
>          /* recursive lock */
>          mutex->mx_count++;
>          _pth_release_lock(&(mutex->mx_lock), descr->tid);
>          return 0;
>      }
>
>      /* should we just tryonly?                    ibm*/
>      if (tryonly) {                              /*ibm*/
>          _pth_release_lock(&(mutex->mx_lock), descr->tid);
>          return EBUSY;                           /*ibm*/
> !    } else
> !       return EDEADLK;
>

Actually, it should be like this:

if (mutex->mx_count >= 1 && mutex->mx_owner == current &&
     mutex->mx_owner_pid==descr->pid) {
        if (mutex->mx_type == PTH_MUTEX_RECURSIVE_NP) {
            /* recursive lock */
            mutex->mx_count++;
            _pth_release_lock(&(mutex->mx_lock), descr->tid);
            return 0;
        } else {
            _pth_release_lock(&(mutex->mx_lock), descr->tid);
            return EDEADLK;
        }
}

The way you had it, always returns EDEADLK.


> > Also,  if (!(mutex->mx_state & PTH_MUTEX_INITIALIZED)) return EINVAL;
> > is not needed, because the caller always check for valid mx_mutex.
> >
>
> The caller checks against mx_init.  But perhaps I'm still too dazed and
> get this
> also wrong.. ;-)
> pth_mutex_init sets mx_state to PTH_MUTEX_INITIALIZED, but the flag is
> also used to check if it's locked or not. I don't know if these tests
> are obsolete.
>

Now, the pth_mutex_t is kept under mx_mutex field of pthread_mutex_t,
so if that is valid, then no need to check again for PTH_MUTEX_INITIALIZED.

> Keep up the good work.
>

Thanks for the pshared rwlock patch.

--
Saurabh Desai
Internet: [email protected]
Lotus Notes: [email protected]
Ph: 512-838-2655  T/L: 678-2655