Full_Name: Ondrej Kuznik Version: master OS: URL: ftp://ftp.openldap.org/incoming/Ondrej-Kuznik-20170414-libldap_r-recursive-mutex.patch Submission from: (NULL) (2a02:c7f:2247:ec00:a3e:8eff:fe52:dac5) Integrating with libevent, I would find it useful if libldap_r provided a recursive mutex as well, since this is what libevent uses. Attached is an implementation. Platforms included: - POSIX (the only one I could test properly) - Win NT (regular mutexes, since they're recursive already, according to the docs) - GNU pth (same as NT, documentation says that all mutexes are recursive) - stub (noop) Not included: - thr_thr.c - not sure what that is - thr_cthreads.c - does not seem to support a recursive thread implementation I'm not sure who still uses the older platforms, I suppose only POSIX and Win NT are being used in the wild? Not that any users should be impacted by this since it's not referenced anywhere in the project yet.
okuznik@symas.com wrote: > Full_Name: Ondrej Kuznik > Version: master > OS: > URL: ftp://ftp.openldap.org/incoming/Ondrej-Kuznik-20170414-libldap_r-recursive-mutex.patch > Submission from: (NULL) (2a02:c7f:2247:ec00:a3e:8eff:fe52:dac5) > > > Integrating with libevent, I would find it useful if libldap_r provided a > recursive mutex as well, since this is what libevent uses. You mean libldap_r/rmutex.c ? -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
On Fri, Apr 14, 2017 at 01:14:08PM +0000, hyc@symas.com wrote: > okuznik@symas.com wrote: >> Integrating with libevent, I would find it useful if libldap_r provided a >> recursive mutex as well, since this is what libevent uses. > > You mean libldap_r/rmutex.c ? Hmm, I see that now, but is quite heavy weight if this already exists in many implementations natively, often at no cost at all. What if I renamed the current ldap_pvt_*_rmutex_* functions to ldap_int_* and deferred to them in the two implementations that can't do recursive mutexes only? This would be an ABI break for rmutex users, other than that, there seems to be no expectation that two versions of libldap_r compiled against a different thread implementation should be ABI compatible already. -- Ondřej Kuzník Senior Software Engineer Symas Corporation http://www.symas.com Packaged, certified, and supported LDAP solutions powered by OpenLDAP
Ondřej Kuzník wrote: > On Fri, Apr 14, 2017 at 01:14:08PM +0000, hyc@symas.com wrote: >> okuznik@symas.com wrote: >>> Integrating with libevent, I would find it useful if libldap_r provided a >>> recursive mutex as well, since this is what libevent uses. >> >> You mean libldap_r/rmutex.c ? > > Hmm, I see that now, but is quite heavy weight if this already exists in > many implementations natively, often at no cost at all. What if I > renamed the current ldap_pvt_*_rmutex_* functions to ldap_int_* and > deferred to them in the two implementations that can't do recursive > mutexes only? > > This would be an ABI break for rmutex users, other than that, there > seems to be no expectation that two versions of libldap_r compiled > against a different thread implementation should be ABI compatible > already. IMO using recursive mutexes means your code is broken. We introduced these for accesslog.c but in fact we could avoid them at zero cost. Also I don't see the relevance of libevent to this discussion. We use our own event mechanism and it is more efficient than libevent. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
> IMO using recursive mutexes means your code is broken. We introduced these for > accesslog.c but in fact we could avoid them at zero cost. Also I don't see the > relevance of libevent to this discussion. We use our own event mechanism and > it is more efficient than libevent. libevent is a dependency for the load balancer that I intend to propose for integration into the project after all the relevant dependencies have come in. There is a new version of this patch that provides an implementation on each platform or defers to the existing one (as per each platform's documentation). Untested except on POSIX and most of them seem pretty arcane anyway. ftp://ftp.openldap.org/incoming/Ondrej-Kuznik-20170918-ITS8638-libldap_r-recursive-mutex.patch -- Ondřej Kuzník Senior Software Engineer Symas Corporation http://www.symas.com Packaged, certified, and supported LDAP solutions powered by OpenLDAP
ondra@mistotebe.net wrote: >> IMO using recursive mutexes means your code is broken. We introduced these for >> accesslog.c but in fact we could avoid them at zero cost. Also I don't see the >> relevance of libevent to this discussion. We use our own event mechanism and >> it is more efficient than libevent. > > libevent is a dependency for the load balancer that I intend to propose > for integration into the project after all the relevant dependencies > have come in. > > There is a new version of this patch that provides an implementation on > each platform or defers to the existing one (as per each platform's > documentation). Untested except on POSIX and most of them seem pretty > arcane anyway. > > ftp://ftp.openldap.org/incoming/Ondrej-Kuznik-20170918-ITS8638-libldap_r-recursive-mutex.patch > It looks like glibc still doesn't define PTHREAD_MUTEX_RECURSIVE by default, it requires compiling with either -D_GNU_SOURCE or -D_XOPEN_SOURCE. The feature itself appears to be part of UNIX98. It's likely that all pthread implementations available today support it, but it still seems a bit iffy. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
hyc@symas.com wrote: > ondra@mistotebe.net wrote: >>> IMO using recursive mutexes means your code is broken. We introduced these for >>> accesslog.c but in fact we could avoid them at zero cost. Also I don't see the >>> relevance of libevent to this discussion. We use our own event mechanism and >>> it is more efficient than libevent. >> >> libevent is a dependency for the load balancer that I intend to propose >> for integration into the project after all the relevant dependencies >> have come in. >> >> There is a new version of this patch that provides an implementation on >> each platform or defers to the existing one (as per each platform's >> documentation). Untested except on POSIX and most of them seem pretty >> arcane anyway. >> >> ftp://ftp.openldap.org/incoming/Ondrej-Kuznik-20170918-ITS8638-libldap_r-recursive-mutex.patch >> > It looks like glibc still doesn't define PTHREAD_MUTEX_RECURSIVE by default, > it requires compiling with either -D_GNU_SOURCE or -D_XOPEN_SOURCE. The > feature itself appears to be part of UNIX98. It's likely that all pthread > implementations available today support it, but it still seems a bit iffy. > OK, I see that current glibc defaults to _POSIX_C_SOURCE=200809 which includes __XOPEN_2K8. All of this came later than our rmutex.c which was written in 2006, so that explains where the need arose from. We should be safe pushing this in, go ahead. -- -- Howard Chu CTO, Symas Corp. http://www.symas.com Director, Highland Sun http://highlandsun.com/hyc/ Chief Architect, OpenLDAP http://www.openldap.org/project/
Added to master
changed notes changed state Open to Closed moved from Incoming to Software Enhancements
changed state Closed to Test