[PATCH] COMMON Darwin Semaphore Bug


Philip Preston
 

Fix in Darwin Semaphore. Count variable was not being checked before decrementing, which was
causing issues in managing the queue size, when running high loads. Further change is to use
Barrier versions of the OSAtomic functions to make changes visible in other threads.

Further details here:

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/OSAtomicDecrement32.3.html

* All unit tests passing on MacOSX - no other operating system needed checked.
* capturereplay / mamalistenc now working correctly
* publisher / consumer tested

All on Qpid

Signed-off-by: Phil Preston <philippreston@mac.com>
---
common/c_cpp/src/c/darwin/wSemaphore.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/common/c_cpp/src/c/darwin/wSemaphore.c b/common/c_cpp/src/c/darwin/wSemaphore.c
index 7df2f06..5ff2f90 100644
--- a/common/c_cpp/src/c/darwin/wSemaphore.c
+++ b/common/c_cpp/src/c/darwin/wSemaphore.c
@@ -63,7 +63,7 @@ wsem_post(wsem_t * sem) {
int result;

if(dispatch_semaphore_signal(sem->dsema) == 0) {
- OSAtomicIncrement32(&sem->count);
+ OSAtomicIncrement32Barrier(&sem->count);
result = WSEM_SUCCEED;
}
else
@@ -77,7 +77,8 @@ int wsem_wait(wsem_t * sem) {
int result;

if (dispatch_semaphore_wait(sem->dsema, DISPATCH_TIME_FOREVER) == 0) {
- OSAtomicDecrement32(&sem->count);
+ if(sem->count)
+ OSAtomicDecrement32Barrier(&sem->count);
result = WSEM_SUCCEED;
}
else
@@ -91,7 +92,7 @@ int wsem_trywait(wsem_t * sem) {
int result;

if (dispatch_semaphore_wait(sem->dsema, DISPATCH_TIME_NOW) == 0) {
- OSAtomicDecrement32(&sem->count);
+ OSAtomicDecrement32Barrier(&sem->count);
result = WSEM_SUCCEED;
}
else
@@ -106,11 +107,12 @@ int wsem_timedwait (wsem_t* sem, unsigned int ts) {

if (dispatch_semaphore_wait(sem->dsema,
dispatch_time(DISPATCH_TIME_NOW, ts * 1000)) == 0) {
- OSAtomicDecrement32(&sem->count);
+ if(sem->count)
+ OSAtomicDecrement32Barrier(&sem->count);
result = WSEM_SUCCEED;
}
else
- result = WSEM_FAILED;
+ result = WSEM_FAILED;

return result;
}
--
1.8.3.4 (Apple Git-47)


Damian Maguire <DMaguire@...>
 

Hey Phil, 

I think you've already noted the Bugzilla tickets for your various issues already, but just to make sure the list is aware of them, the following have been raised for your changes.

Ticket Numbers: 61, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 59, 60, 62, 64, 71

Most of them are awaiting test at present, so we'll get back to you with any feedback on those.

Cheers, 


From: Philip Preston <philippreston@...>
Date: Friday, February 21, 2014 3:30 PM
To: OpenMAMA Dev List <openmama-dev@...>
Subject: [Openmama-dev] [PATCH] COMMON Darwin Semaphore Bug

Fix in Darwin Semaphore.  Count variable was not being checked before decrementing, which was
causing issues in managing the queue size, when running high loads.  Further change is to use
Barrier versions of the OSAtomic functions to make changes visible in other threads.

Further details here:

https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/OSAtomicDecrement32.3.html

* All unit tests passing on MacOSX - no other operating system needed checked.
* capturereplay / mamalistenc now working correctly
* publisher / consumer tested

All on Qpid

Signed-off-by: Phil Preston <philippreston@...>
---
 common/c_cpp/src/c/darwin/wSemaphore.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/common/c_cpp/src/c/darwin/wSemaphore.c b/common/c_cpp/src/c/darwin/wSemaphore.c
index 7df2f06..5ff2f90 100644
--- a/common/c_cpp/src/c/darwin/wSemaphore.c
+++ b/common/c_cpp/src/c/darwin/wSemaphore.c
@@ -63,7 +63,7 @@ wsem_post(wsem_t * sem) {
     int result;
 
     if(dispatch_semaphore_signal(sem->dsema) == 0) {
-        OSAtomicIncrement32(&sem->count);
+        OSAtomicIncrement32Barrier(&sem->count);
         result =  WSEM_SUCCEED;
     }
     else
@@ -77,7 +77,8 @@ int wsem_wait(wsem_t * sem) {
     int result;
 
     if (dispatch_semaphore_wait(sem->dsema, DISPATCH_TIME_FOREVER) == 0) {
-        OSAtomicDecrement32(&sem->count);
+        if(sem->count)
+            OSAtomicDecrement32Barrier(&sem->count);
         result = WSEM_SUCCEED;
     }
     else
@@ -91,7 +92,7 @@ int wsem_trywait(wsem_t * sem) {
     int result;
 
     if (dispatch_semaphore_wait(sem->dsema, DISPATCH_TIME_NOW) == 0) {
-        OSAtomicDecrement32(&sem->count);
+        OSAtomicDecrement32Barrier(&sem->count);
         result = WSEM_SUCCEED;
     }
     else
@@ -106,11 +107,12 @@ int wsem_timedwait (wsem_t* sem, unsigned int ts) {
 
     if (dispatch_semaphore_wait(sem->dsema,
                                 dispatch_time(DISPATCH_TIME_NOW, ts * 1000)) == 0) {
-        OSAtomicDecrement32(&sem->count);
+        if(sem->count)
+            OSAtomicDecrement32Barrier(&sem->count);
         result = WSEM_SUCCEED;
     }
     else
-       result = WSEM_FAILED;
+        result = WSEM_FAILED;
 
     return result;
 }
--
1.8.3.4 (Apple Git-47)



_______________________________________________
Openmama-dev mailing list
Openmama-dev@...
https://lists.openmama.org/mailman/listinfo/openmama-dev

This message may contain confidential information and is intended for specific recipients unless explicitly noted otherwise. If you have reason to believe you are not an intended recipient of this message, please delete it and notify the sender. This message may not represent the opinion of IntercontinentalExchange Group, Inc. (ICE), NYSE Euronext or any of their subsidiaries or affiliates, and does not constitute a contract or guarantee. Unencrypted electronic mail is not secure and the recipient of this message is expected to provide safeguards from viruses and pursue alternate means of communication where privacy or a binding message is desired.