Re: A clean way to detect if a ldap_pvt_thread_mutex is locked (best would be also to know who's the owner of the lock :)

[Hallvard B Furuseth <h.b.furuseth@usit.uio.no>]

Pierangelo Masarati writes:
> On Fri, 2006-03-31 at 20:09 +0200, Hallvard B Furuseth wrote:
>>  Or more generally, use my thr_debug.c package and extend
>>  ldap_debug_thread_cond_t with the thread ID and info about whether the
>>  mutex is locked.  Get thread ID with ldap_pvt_thread_self(), compare
>>  with ldap_pvt_thread_equal().
>
> What about this?

Looks mostly OK, though I haven't tested it - nor my comments below.

>  I know for posix threads I could have inspected the mutex itself (I
> could specialize the implementation) but this should be pretty
> general.

That's why I wrote the package.  Stupid that I forgot it in this thread:-)

> When the owner is modified the datum is protected by
> the mutex itself; it may not be when the owner is read, though.

It is in LDAP_PVT_THREAD_MUTEX_MUSTBESELFLOCKED().  Do you really need
LDAP_PVT_THREAD_MUTEX_MUSTBELOCKED/LDAP_PVT_THREAD_MUTEX_MUSTBEUNLOCKED
too?  What good are they?  Like Kurt I and said, return values which may
no longer reflect the mutex's state by the time you execute the next
statement.  LDAP_PVT_THREAD_MUTEX_MUSTBEUNLOCKED can catch mutex leaks
of course, but thr_debug already does that - depending on which options
you invoke it with.

BTW, I'd s/MUSTBE/ASSERT_/ in the name, and if you just need that macro,
maybe call it LDAP_PVT_THREAD_ASSERT_MUTEX_OWNER().  The current names
are not too readable:-)

> +#ifndef LDAP_THREAD_DEBUG_NONE
> +/* if this definition is not suitable, define per-architecture */
> +#define	LDAP_THREAD_DEBUG_NONE		((ldap_int_thread_t)(-1))
> +#endif /* ! LDAP_THREAD_DEBUG_NONE */

Namespace pollution, it should be LDAP_INT_THREAD_DEBUG_NONE.  Except,

that fails if ldap_int_thread_t is a struct/union, and fails and/or
warns if it is a pointer.  That's OK since this is just a debugging;
someone can fix it if they encounter the problem.  But do prepare for
the possibility:

ldap_int_thread.h:
  #if LDAP_THREAD_DEBUG
    extern ldap_int_thread_t ldap_int_thread_debug_none;
  #endif

thr_debug.h:
  /* If this definition is not suitable,
   * define it per architecture or thread package */
  ldap_int_thread_t ldap_int_thread_debug_none = { -123 };

To avoid a bunch of #ifdef LDAP_R_COMPILEs in the rest of the code:

   #ifdef LDAP_R_COMPILE
    ...
> +#define	LDAP_PVT_THREAD_MUTEX_MUSTBESELFLOCKED(mutex) \
> +	assert(ldap_int_thread_equal((mutex)->owner, ldap_pvt_thread_self()))

   /* (please reduce to <80 chars when possible even with 8-width tabs:-) */

   ...
   #else
>  #define	LDAP_PVT_THREAD_MUTEX_MUSTBELOCKED(mutex) ((void)0)
   #endif

Macro cleanliness:

> +#define SETOWNER(ptr)
> +#define RESETOWNER(ptr)
> +#define SETOWNER(ptr)	(ptr)->owner = ldap_pvt_thread_self()
> +#define RESETOWNER(ptr)	(ptr)->owner = LDAP_THREAD_DEBUG_NONE

The expansions should be expressions with parens around them.
Also I suggest SET_OWNER and RESET_OWNER - easier to read.

>  #define SET_OWNER(ptr)	((void) 0)
>  #define RESET_OWNER(ptr)	((void) 0)
>  #define SET_OWNER(ptr)	((ptr)->owner = ldap_pvt_thread_self())
>  #define RESET_OWNER(ptr)	((ptr)->owner = LDAP_THREAD_DEBUG_NONE)

With code like this:

>  		adjust_count( Idx_mutex, -1 );
> +		RESETOWNER( mutex );

>  		adjust_count( Idx_locked_mutex, +1 );
> +		SETOWNER( mutex );

I assume it would be cleaner to either RESET after adjust and SET before
adjust, or the other way around.  Haven't thought about it clearly at
the moment though.  There is a bit too much magic going in in there:-)

Finally, except in mutex_init, RESET_OWNER could also do a
MUSTBESELFLOCKED check.  I expect all the ldap_pvt_thread_self() calls
could slow things down a lot though, so that should probably only be
done if a 'checkowner' option has been set in $LDAP_THREAD_DEBUG.

-- 
Hallvard