Doug,
a list of relatively quick comments, as I didn't go into too much detail in the analysis of each bit of your work.
I think your patch contains at least three levels of modifications; they all make sense separately and incrementally, so I think we should revise them separately.
- "cosmetic" cleanup (like wrapping locks in specific macros); they do
not strictly relate to the support of draft-zeilenga-ldap-c-api-concurrency (but they make sense, and I like them); they should be considered first, regardless of the rest (I note that according to the way you define LDAP_NEXT_MSGID when #ifdef LDAP_R_COMPILE, you don't need to #ifdef its definition :)
- a set of issues that fix the concurrent behavior of libldap relatively
irrespective of the support of draft-zeilenga-ldap-c-api-concurrency (see for example the private discussion we had on fixing concurrent access to lconn_refcnt).
- specific support to draft-zeilenga-ldap-c-api-concurrency, including
but not limited to the addition of ldap_dup(), ldap_destroy() and related stuff.
As you may have understood, I'm especially interested in (2), because it fixes a number of potential (or actual) issues in OpenLDAP code that uses libldap concurrently, like the proxy backends. So far, that code used libldap taking into account known weaknesses from a concurrency standpoint. For example, proxy backends protect handlers until a bind succeeds, to prevent the undesired simultaneous creation of default connections. However, some issues may slip in (as I believe is the case of ITS#6574).
One thing I'd like to ask is: you introduce a few additional mutexes:
ldapoptions -> ldo_mutex
ldapcommon -> ldc_msgid_mutex, ldc_abandon_mutex
in addition of the already existing
ldap -> ld_conn_mutex, ld_req_mutex, ld_res_mutex
that move to ldap_common.
My concern is: can you guarantee that the occurrences of locking/unlocking those additional mutexes, combined to the existing ones, do not result in deadlocks? I mean: did you explicitly check all possible logical paths or so, or take measures to avoid this possibility?
Another mostly "style" comment: I think you shouldn't be polluting libldap's space with thread implementation specific details, like
init.c: #if defined(HAVE_THR) #elif defined(HAVE_PTHREADS) PTHREAD_MUTEX_INITIALIZER #if !(defined(HAVE_THR) || defined(HAVE_PTHREADS))
and so. I understand PTHREAD_MUTEX_INITIALIZER is handy, but the whole thing should remain confined in libldap_r specific files.
Also, but since it's a test tool it is not a priority, slapd-mtread.c should use libldap_r and hide thread implementation details under OpenLDAP's portable interface.
p.
I will take a look at reworking the
On 08/22/10 10:09 AM, masarati@aero.polimi.it wrote:
My concern is: can you guarantee that the occurrences of locking/unlocking those additional mutexes, combined to the existing ones, do not result in deadlocks? I mean: did you explicitly check all possible logical paths or so, or take measures to avoid this possibility?
Another mostly "style" comment: I think you shouldn't be polluting libldap's space with thread implementation specific details, like
init.c: #if defined(HAVE_THR) #elif defined(HAVE_PTHREADS) PTHREAD_MUTEX_INITIALIZER #if !(defined(HAVE_THR) || defined(HAVE_PTHREADS))
and so. I understand PTHREAD_MUTEX_INITIALIZER is handy, but the whole thing should remain confined in libldap_r specific files.
I will take a look at reworking this for the next round of code review.
Also, but since it's a test tool it is not a priority, slapd-mtread.c should use libldap_r and hide thread implementation details under OpenLDAP's portable interface.
I agree, that slapd-mtread.c still needs additional work. [Also see first respose]. I will revise and clean this up for the next round of code review.
Doug.
I will take a look at reworking the
On 08/22/10 10:09 AM, masarati@aero.polimi.it wrote:
My concern is: can you guarantee that the occurrences of locking/unlocking those additional mutexes, combined to the existing ones, do not result in deadlocks? I mean: did you explicitly check all possible logical paths or so, or take measures to avoid this possibility?
Another mostly "style" comment: I think you shouldn't be polluting libldap's space with thread implementation specific details, like
init.c: #if defined(HAVE_THR) #elif defined(HAVE_PTHREADS) PTHREAD_MUTEX_INITIALIZER #if !(defined(HAVE_THR) || defined(HAVE_PTHREADS))
and so. I understand PTHREAD_MUTEX_INITIALIZER is handy, but the whole thing should remain confined in libldap_r specific files.
I will take a look at reworking this for the next round of code review.
You could
#if defined(HAVE_THR) #define LDAP_PVT_MUTEX_INITIALIZER DEFAULTMUTEX #elif defined(HAVE_PTHREADS) #define LDAP_PVT_MUTEX_INITIALIZER PTHREAD_MUTEX_INITIALIZER #else #define LDAP_PVT_MUTEX_INITIALIZER NULL #endif
then use LDAP_PVT_MUTEX_INITIALIZER in code; you could then add a ldap_pvt_thread_mutex_init_if(mutex) that assumes mutex was initialized with LDAP_PVT_MUTEX_INITIALIZER; for THR and PTHREADS this call should be empty, while in the remaining cases it would call ldap_pvt_thread_mutex_init() only if mutex is not equal to DAP_PVT_MUTEX_INITIALIZER (NULL).
Also, I get some nasty warnings from other parts of the code that (admittedly incorrectly) include ldap-int.h because LDAP_PVT_THREAD_ASSERT_MUTEX_OWNER() is being redefined. I suggest that instead of #defining it if LDAP_R_COMPILE is not defined, you rather
#ifdef LDAP_R_COMPILE #define LDAP_ASSERT_MUTEX_OWNER(mutex) \ LDAP_PVT_THREAD_ASSERT_MUTEX_OWNER( mutex ) #else #define LDAP_ASSERT_MUTEX_OWNER(mutex) #endif
so you avoid mixing LDAP_PVT_THREAD_* and LDAP_* macros.
Thanks, p.
The following is a link to the revised proposed patch to implement: draft-zeilenga-ldap-c-api-concurrency
I believe I have addressed all the issues with the first proposed patch. This included: Removal of the additional errno APIs The requested code and macro cleanup fixes Cleanup of the new test case (slapd-mtread.c/test060-mt-hot) Removal of the old SunOS THR_LWP code
The new web location is: http://cr.opensolaris.org/~djl/openldap-codereview-r2/
The new diffs include: a complete webrev diff of the proposed patch against HEAD (as of 8/30/10)
a webrev diff of the round 1 patches (with latest HEAD changes applied) vs the new round 2 patch
diff -U of both of the above
a complete source tarball of HEAD as of 8/30/10) plus the proposed patch.
And a link to the round 1 bits for reference if needed.
I am still working on producing some code coverage test results. I will send that in a separate email once I have it.
Thank you in advance for your consideration, Doug.
FYI: I have been working off line with p. [masarati@aero.polimi.it] to evaluate performance and behavior of this patch.
Based on that investigation, I plan to make some additional modifications to the result processing portion of the patch code.
Once completed, and re-tested, I will send out a new round of diffs.
Doug.
While I'm making the second round of changes, I have a follow up question. Is there any objection to me removing the THR_LWP code from libldap_r as part of this ITS?
Unless there is an objection, I plan to have that code removed from my next round of diffs.
[I looked it up] For the record, Sun stopped support of these interfaces, pre-Solaris, in SunOS 4.1.4. Officially Sun EOLed these interfaces on Dec 27, 1998, and halted the last of the vintage support of them on Sep 30, 2003.
The official statement of this is: http://www.sun.com/service/eosl/eosl_solaris.html
So, Sun hasn't supported liblwp (aka the LWP APIs) from SunOS for about 8 years, the last functional machines are sitting in places like the Computer history Museum and a museum of old machines at the old corporate offices. It really is time for them to RIP.
Side note: There was a time when some of us engineers still used old sun4m's as door stops, bookends, and height adjusters for flat panels. But I think for the most part even those boxes were carted off to the scrap metal heap last decade. I gave my last ones away in 2005. :( Should a kept one as a memento...
-
Doug Leavitt -
masarati＠aero.polimi.it