"Julian Graham" <[EMAIL PROTECTED]> writes:
> Find attached a patch (in two parts [...]
Thanks; I've reviewed and applied those to my tree, and rebuilding
now; will push shortly unless I see any build errors.
> * Re-added support for locking mutexes with an owner other than the
> current thread
> * Enabled the previously ifdef'd out functions scm_mutex_owner and
> scm_mutex_level
> * Added a new function, scm_mutex_locked_p, useful for distinguishing
> between unlocked and unowned mutex states.
> * Updated the threads.test file to reflect the changes above
> * Updated the documentation in api-scheduling.texi to reflect the changes
> above
> * Updated the ChangeLog to reflect the changes above
All great.
> Also attached are updated versions of the Scheme SRFI-18
> implementation as well as the SRFI-18-specific test code.
I haven't covered these yet. Will try to soon, but could you resubmit
anyway as a GIT commit patch, so that you end up being properly
credited for the commit?
> A couple of notes: For purposes of elegance, I've changed semantics of
> fat_mutex.level -- where previously all non-recursive mutexes had a
> level of -1, recursiveness is now denoted by an integer field on
> fat_mutex. Any mutex (recursive or not) is in a locked state iff its
> level is greater than 0.
Cool; I think this is nicer than the previous -1 representation.
> Second, during the testing I did for this round of changes, I noticed
> a few more deadlocks that I believe are related to the existing core
> threading model (as opposed to the changes included in this patch).
> These seem to crop up when I run overnight tests on modified core
> code, probably because different timings and thread interactions are
> introduced. I think I've got fixes for some of them, and I'll
> follow-up in a separate mailing list thread.
OK. I'll look out for those.
Even though I've already committed, I had one query...
> @@ -1211,79 +1212,77 @@ SCM_DEFINE (scm_make_recursive_mutex,
> "make-recursive-mutex", 0, 0, 0,
> SCM_SYMBOL (scm_abandoned_mutex_error_key, "abandoned-mutex-error");
>
> static SCM
> -fat_mutex_lock (SCM mutex, scm_t_timespec *timeout, int *ret)
> +fat_mutex_lock (SCM mutex, scm_t_timespec *timeout, SCM owner, int *ret)
> {
> fat_mutex *m = SCM_MUTEX_DATA (mutex);
>
> - SCM thread = scm_current_thread ();
> - scm_i_thread *t = SCM_I_THREAD_DATA (thread);
> -
> + SCM new_owner = SCM_UNBNDP (owner) ? scm_current_thread() : owner;
> SCM err = SCM_BOOL_F;
>
> struct timeval current_time;
>
> scm_i_scm_pthread_mutex_lock (&m->lock);
> - if (scm_is_false (m->owner))
> - {
> - m->owner = thread;
> - scm_i_pthread_mutex_lock (&t->admin_mutex);
> - t->mutexes = scm_cons (mutex, t->mutexes);
> - scm_i_pthread_mutex_unlock (&t->admin_mutex);
> - *ret = 1;
> - }
> - else if (scm_is_eq (m->owner, thread))
> +
> + while (1)
> {
> - if (m->level >= 0)
> + if (m->level == 0)
> {
> + m->owner = new_owner;
> m->level++;
> - *ret = 1;
> - }
> - else
> - err = scm_cons (scm_misc_error_key,
> - scm_from_locale_string ("mutex already locked by "
> - "current thread"));
> - }
> - else
> - {
> - int first_iteration = 1;
> - while (1)
> - {
> - if (scm_is_eq (m->owner, thread) || scm_c_thread_exited_p (m->owner))
> +
> + if (SCM_I_IS_THREAD (new_owner))
> {
> + scm_i_thread *t = SCM_I_THREAD_DATA (new_owner);
> scm_i_pthread_mutex_lock (&t->admin_mutex);
> t->mutexes = scm_cons (mutex, t->mutexes);
> scm_i_pthread_mutex_unlock (&t->admin_mutex);
> - *ret = 1;
> - if (scm_c_thread_exited_p (m->owner))
> - {
> - m->owner = thread;
> - err = scm_cons (scm_abandoned_mutex_error_key,
> - scm_from_locale_string ("lock obtained on "
> - "abandoned mutex"));
> - }
> - break;
> }
> - else if (!first_iteration)
> + *ret = 1;
> + break;
> + }
> + else if (SCM_I_IS_THREAD (m->owner) && scm_c_thread_exited_p
> (m->owner))
> + {
> + m->owner = new_owner;
Should m->level be set to 1 here?
Regards, and thanks once again for your work on this area!
Neil
