Re: srfi-18 requirements

[Neil Jerram <neil@ossau.uklinux.net>]

"Julian Graham" <joolean@gmail.com> 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