[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
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 :)
- To: Pierangelo Masarati <ando@sys-net.it>
- Subject: 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 :)
- From: Hallvard B Furuseth <h.b.furuseth@usit.uio.no>
- Date: Sat, 1 Apr 2006 12:55:58 +0200
- Cc: openldap-devel@OpenLDAP.org
- In-reply-to: <1143844713.4496.25.camel@ando>
- References: <39487.131.175.154.56.1143620973.squirrel@131.175.154.56> <hbf.20060331puo@bombur.uio.no> <1143844713.4496.25.camel@ando>
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