[fdo-internals] MOTION: RFC 47 - Add Thread Support to FdoIDisposable AddRef/Release

Jason Birch jason at jasonbirch.com
Fri Jan 22 19:36:53 EST 2010 

+1 Me

On 2010-01-21, Greg Boone <greg.boone at autodesk.com> wrote:
> Hi All,
>
> If there are no additional comments, I would like to propose a vote on RFC
> 47. http://trac.osgeo.org/fdo/wiki/FDORfc47
>
> The plan at this time is to proceed with the alternate solution as outlined
> by Traian, with atomic operations being used on Linux instead of a Mutex.
>
> Regards,
> Greg
>
>
> -----Original Message-----
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Tuesday, January 19, 2010 3:55 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> For those interested in a detailed explanation of the asm code, refer to...
>
> http://www.experts-exchange.com/Programming/Languages/Assembly/Q_23571643.html
>
> Regards,
> Greg
>
>
> -----Original Message-----
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
> Sent: Tuesday, January 19, 2010 2:04 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
>
> Small correction for the asm call, in case we end up using it:
>
> {
> int ret = 1;
> __asm__ __volatile__ ("lock xaddl %0,%1" : "=r"(ret), "=m"(m_refCount) :
> "0"(ret), "m"(m_refCount) : "memory");
> return ret+1;
> }
>
> This one compiles ok on 4.4 also. I'm no expert on GCC inline assembly, but
> it may be somehow possible to have the instruction set the new value to ret
> instead of having to increment it in a second instruction before returning
> it. Returning the refcount from AddRef is for debugging only, so that +1
> feels like kind of a waste... Same applies to the decrement that happens in
> Release. Fortunately this snippet only really needs to be used for the older
> GCC.
>
> For Release(), we can use the same asm, but set ret=-1 initially, so that it
> subtracts 1 from the refcount.
>
> Traian
>
>
> -----Original Message-----
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Tuesday, January 19, 2010 1:38 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> I agree that we should really get 4.1.2 installed on the build machine.
>
> Tom... Can we make this upgrade?
>
> -----Original Message-----
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
> Sent: Tuesday, January 19, 2010 1:28 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
>
> Dang, 3.4 is ancient... Even 4.1.2, where those were introduced is like 3
> years old. Well, it means we need more ifdefs, something like calling the
> atomic instruction directly with inline assembly:
>
>
>    int AddRef(){
>  #if defined(__GNUC__) && defined (__GNUC_MINOR__) && ((__GNUC__ < 4) ||
> (__GNUC__ == 4 && __GNUC_MINOR__ < 1))
>      int ret=1;
>      __asm__ __volatile__ ("lock\n\t"
> 	                       "xaddl %0,%1\n\t" : "=r"(ret), "=m"(m_refCount) :
> "0"(m_refCount), "m"(val) : "memory");
>      return ret+1;
>  #elif defined (__GNUC__)
>      return __sync_add_and_fetch(&m_refCount,1);
>  #elif defined(WIN32)
>      return InterlockedAdd(&m_refCount,1);
>  #else
>      return ++val;
>  #endif
>      }
>
> Note I didn't check the correctness of the first ifdef, but it looks about
> right...
>
> We also need to determine which version of GCC/G++ we should target in the
> next release, since 3.4 is really really really old.
>
> Traian
>
>
>
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Tuesday, January 19, 2010 1:25 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> A slight issue to using these would be that they are not included in earlier
> version of gcc, such as 3.4.3, which is still being used on our build
> machines. Gah...
>
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
> Sent: Tuesday, January 19, 2010 1:13 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> In my Ubuntu 32 bit machine with a relatively newer GCC, the following
> compiles without having to include any headers at all, since the Intel
> atomic functions are built into GCC itself.
>
>     FDO_API_COMMON virtual FdoInt32 AddRef()
>     {
>         return __sync_add_and_fetch(&m_refCount, 1);
> //        return ++m_refCount;
>     }
>
> Here is the documentation for those:
>
> http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html
>
>
> Traian
>
>
>
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Tuesday, January 19, 2010 1:13 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> Do you know which function/header file to use?
>
> /usr/include/alsa/iatomic.h?
>
> Greg
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
> Sent: Tuesday, January 19, 2010 12:55 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
>
> This is correct if atomic_inc is in a kernel header, that header should not
> be included from user-space.
>
> However, GCC does have other headers defining atomic intrinsics, which can
> be included from user space.
>
> My point was about using mutex vs. atomic, not specifically about which
> header (kernel or gcc) to include for the atomic functions.
>
> Traian
>
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Tuesday, January 19, 2010 12:46 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> When exploring various posts on the usage of atomic_inc, I often see a
> warning not to include kernel headers from userspace. There is even a pragma
> warning to this effect in atomic.h.
>
> Are you saying that it is safe to ignore this advice?
>
> Greg
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
> Sent: Friday, January 15, 2010 12:22 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
>
> Theoretically both the GCC and the MS VC++ implementations would be calling
> the same underlying Intel CPU dedicated instruction that does the atomic
> operation. Chatter on the web is not always reliable., and I would
> personally doubt threading on Windows before I doubt threading on Linux,
> given that Linux is used a lot more in thread-heavy environments.
>
> Traian
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Friday, January 15, 2010 12:17 PM
> To: FDO Internals Mail List
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> I took a look at functions such as atomic_dec_and_test and
> atomic_inc_and_test, but there was a lot of chatter on the web that these
> calls were not sufficient to provide full thread synchronization. In that
> light I chose to adopt the strategy MapGuide used and implement the solution
> on Linux using a mutex.
>
> Not being a 100% familiar with these operations, I welcome any feedback on
> their use and the pros and cons of doing so.
>
> NOTE:  I do agree that having a mutex per object is significant overhead on
> Linux.
>
> Greg
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Traian Stanev
> Sent: Friday, January 15, 2010 12:12 PM
> To: 'FDO Internals Mail List'
> Subject: RE: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
>
> Hi Greg,
>
> What is the reason for using a mutex on Linux instead of atomic operations
> like on Windows?
>
> Traian
>
>
> From: fdo-internals-bounces at lists.osgeo.org
> [mailto:fdo-internals-bounces at lists.osgeo.org] On Behalf Of Greg Boone
> Sent: Friday, January 15, 2010 11:53 AM
> To: FDO Internals Mail List
> Subject: [fdo-internals] FOR REVIEW: RFC 47 - Add Thread Support to
> FdoIDisposable AddRef/Release
>
> Hi All,
>
> I have written and posted FDO RFC 47: Add Thread Support to FdoIDisposable
> AddRef/Release, accessible at:
>
>        http://trac.osgeo.org/fdo/wiki/FDORfc47
>
> Overview:
>
> The AddRef and Release methods on FdoIDisposable are not currently
> thread-safe. As a consequence the m_refCount data member can get corrupted
> if two separate threads simultaneously call any of these methods on the same
> object. This is known to happen during garbage collection of Managed FDO
> objects. Because the managed objects implement a finalizer, they are
> processed by a separate GC finalizer thread. So while the GC finalizer
> thread is releasing ref counts on unmanaged objects, the main thread can be
> simultaneously modifying ref-counts on those same objects.
>
> Proposed Solution:
>
> In MapGuide the refcount threading issue is addressed by having the
> refcounted classes extend from MgGuardDisposable, which in turn uses a mutex
> to limit access to the refcount member variable to one thread at a time. A
> similar solution will be implemented for the FDO API, with a slight
> variation. On Linux, a mutex will be used. On Windows, the FDO API will use
> the InterlockedIncrement and InterlockedDecrement methods
>
> On Windows, the use of thread safe functions will always be activated in
> case where unmanaged FDO objects are wrapped by a Managed object wrapper.
> Otherwise, API callers will have the option of setting a per-object or
> global flag that will activate thread-safe access of the reference count
> member variable.
>
>
> Please refer to the RFC for a full description on the change and a working
> draft of the proposed solution.
>
> All comments are welcome.
>
> Regards,
> Greg
> _______________________________________________
> fdo-internals mailing list
> fdo-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/fdo-internals
> _______________________________________________
> fdo-internals mailing list
> fdo-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/fdo-internals
> _______________________________________________
> fdo-internals mailing list
> fdo-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/fdo-internals
> _______________________________________________
> fdo-internals mailing list
> fdo-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/fdo-internals
> _______________________________________________
> fdo-internals mailing list
> fdo-internals at lists.osgeo.org
> http://lists.osgeo.org/mailman/listinfo/fdo-internals
>

More information about the fdo-internals mailing list