From 48257aec7ce3d6fdb1e21a55677bc1f6eae04100 Mon Sep 17 00:00:00 2001 From: "W. Eric Norum" Date: Tue, 7 Oct 2008 11:05:41 +0000 Subject: [PATCH] Fix race condition exposed by compilers with more agressive optimization. Add test procedure for epicsRingBytes. --- documentation/RELEASE_NOTES.html | 4 + src/libCom/ring/epicsRingBytes.c | 106 +++++++++---------- src/libCom/ring/epicsRingPointer.h | 28 ++--- src/libCom/test/Makefile | 5 + src/libCom/test/epicsRunLibComTests.c | 3 + src/libCom/test/ringBytesTest.c | 142 ++++++++++++++++++++++++++ src/libCom/test/ringPointerTest.c | 2 +- 7 files changed, 222 insertions(+), 68 deletions(-) create mode 100644 src/libCom/test/ringBytesTest.c diff --git a/documentation/RELEASE_NOTES.html b/documentation/RELEASE_NOTES.html index ea0239e6a..77ef2ca30 100644 --- a/documentation/RELEASE_NOTES.html +++ b/documentation/RELEASE_NOTES.html @@ -12,6 +12,10 @@

Changes between 3.14.9 and 3.14.10

+

epicsRingPointer, epicsRingBytes

+ +

Fixed race condition exposed by compilers with more agressive optimization.

+

camonitor timestamp support

The camonitor program now supports the ability to display both diff --git a/src/libCom/ring/epicsRingBytes.c b/src/libCom/ring/epicsRingBytes.c index 82c026379..5f33fae7f 100644 --- a/src/libCom/ring/epicsRingBytes.c +++ b/src/libCom/ring/epicsRingBytes.c @@ -22,20 +22,28 @@ #include "cantProceed.h" #include "epicsRingBytes.h" +/* + * Need at least one extra byte to be able to distinguish a completely + * full buffer from a completely empty one. Allow for a little extra + * space to try and keep good alignment and avoid multiple calls to + * memcpy for a single put/get operation. + */ +#define SLOP 16 + typedef struct ringPvt { - int nextPut; - int nextGet; - int size; - char *buffer; + volatile int nextPut; + volatile int nextGet; + int size; + volatile char *buffer; }ringPvt; - epicsShareFunc epicsRingBytesId epicsShareAPI epicsRingBytesCreate(int size) { ringPvt *pring = mallocMustSucceed(sizeof(ringPvt),"epicsRingBytesCreate"); - pring->size = size + 1; + pring->size = size + SLOP; pring->buffer = mallocMustSucceed(pring->size,"ringCreate"); - pring->nextGet = pring->nextPut = 0; + pring->nextGet = 0; + pring->nextPut = 0; return((void *)pring); } @@ -45,7 +53,7 @@ epicsShareFunc void epicsShareAPI epicsRingBytesDelete(epicsRingBytesId id) free((void *)pring->buffer); free((void *)pring); } - + epicsShareFunc int epicsShareAPI epicsRingBytesGet( epicsRingBytesId id, char *value,int nbytes) { @@ -59,7 +67,8 @@ epicsShareFunc int epicsShareAPI epicsRingBytesGet( count = nextPut - nextGet; if (count < nbytes) nbytes = count; - memcpy (value, &pring->buffer[nextGet], nbytes); + if (nbytes) + memcpy (value, &pring->buffer[nextGet], nbytes); nextGet += nbytes; } else { @@ -67,7 +76,8 @@ epicsShareFunc int epicsShareAPI epicsRingBytesGet( if (count > nbytes) count = nbytes; memcpy (value, &pring->buffer[nextGet], count); - if ((nextGet = nextGet + count) == size) { + nextGet += count; + if (nextGet == size) { int nLeft = nbytes - count; if (nLeft > nextPut) nLeft = nextPut; @@ -90,78 +100,68 @@ epicsShareFunc int epicsShareAPI epicsRingBytesPut( int nextGet = pring->nextGet; int nextPut = pring->nextPut; int size = pring->size; - int count; + int freeCount, copyCount, topCount; if (nextPut < nextGet) { - count = nextGet - nextPut - 1; - if (nbytes > count) - nbytes = count; - memcpy (&pring->buffer[nextPut], value, nbytes); - nextPut += nbytes; - } - else if (nextGet == 0) { - count = size - nextPut - 1; - if (nbytes > count) - nbytes = count; - memcpy (&pring->buffer[nextPut], value, nbytes); + freeCount = nextGet - nextPut - SLOP; + if (nbytes > freeCount) + nbytes = freeCount; + if (nbytes) + memcpy (&pring->buffer[nextPut], value, nbytes); nextPut += nbytes; } else { - count = size - nextPut; - if (count > nbytes) - count = nbytes; - memcpy (&pring->buffer[nextPut], value, count); - if ((nextPut = nextPut + count) == size) { - int nLeft = nbytes - count; - if (nLeft > (nextGet - 1)) - nLeft = nextGet - 1; - memcpy (&pring->buffer[0], value+count, nLeft); + freeCount = size - nextPut + nextGet - SLOP; + if (nbytes > freeCount) + nbytes = freeCount; + topCount = size - nextPut; + copyCount = (nbytes > topCount) ? topCount : nbytes; + if (copyCount) + memcpy (&pring->buffer[nextPut], value, copyCount); + nextPut += copyCount; + if (nextPut == size) { + int nLeft = nbytes - copyCount; + if (nLeft) + memcpy (&pring->buffer[0], value+copyCount, nLeft); nextPut = nLeft; - nbytes = count + nLeft; - } - else { - nbytes = count; - nextPut = nextPut; } } pring->nextPut = nextPut; return nbytes; } - + epicsShareFunc void epicsShareAPI epicsRingBytesFlush(epicsRingBytesId id) { ringPvt *pring = (ringPvt *)id; - pring->nextGet = pring->nextPut = 0; + pring->nextGet = 0; + pring->nextPut = 0; } - + epicsShareFunc int epicsShareAPI epicsRingBytesFreeBytes(epicsRingBytesId id) { ringPvt *pring = (ringPvt *)id; - int n; + int nextGet = pring->nextGet; + int nextPut = pring->nextPut; - n = pring->nextGet - pring->nextPut - 1; - if (n < 0) - n += pring->size; - return n; + if (nextPut < nextGet) + return nextGet - nextPut - SLOP; + else + return pring->size - nextPut + nextGet - SLOP; } epicsShareFunc int epicsShareAPI epicsRingBytesUsedBytes(epicsRingBytesId id) { ringPvt *pring = (ringPvt *)id; - int n; - n = pring->nextPut - pring->nextGet; - if (n < 0) - n += pring->size; - return n; + return pring->size - epicsRingBytesFreeBytes(id) - SLOP; } epicsShareFunc int epicsShareAPI epicsRingBytesSize(epicsRingBytesId id) { ringPvt *pring = (ringPvt *)id; - return pring->size - 1; + return pring->size - SLOP; } epicsShareFunc int epicsShareAPI epicsRingBytesIsEmpty(epicsRingBytesId id) @@ -173,9 +173,5 @@ epicsShareFunc int epicsShareAPI epicsRingBytesIsEmpty(epicsRingBytesId id) epicsShareFunc int epicsShareAPI epicsRingBytesIsFull(epicsRingBytesId id) { - ringPvt *pring = (ringPvt *)id; - int count; - - count = (pring->nextPut - pring->nextGet) + 1; - return ((count == 0) || (count == pring->size)); + return (epicsRingBytesFreeBytes(id) <= 0); } diff --git a/src/libCom/ring/epicsRingPointer.h b/src/libCom/ring/epicsRingPointer.h index 4aca810d3..57534586a 100644 --- a/src/libCom/ring/epicsRingPointer.h +++ b/src/libCom/ring/epicsRingPointer.h @@ -43,10 +43,10 @@ private: /* Prevent compiler-generated member functions */ epicsRingPointer& operator=(const epicsRingPointer &); private: /* Data */ - int nextPush; - int nextPop; + volatile int nextPush; + volatile int nextPop; int size; - T **buffer; + T * volatile * buffer; }; extern "C" { @@ -93,10 +93,11 @@ inline epicsRingPointer::~epicsRingPointer() template inline bool epicsRingPointer::push(T *p) { - int newNext = nextPush +1; + int next = nextPush; + int newNext = next + 1; if(newNext>=size) newNext=0; if(newNext==nextPop) return(false); - buffer[nextPush] = p; + buffer[next] = p; nextPush = newNext; return(true); } @@ -104,18 +105,21 @@ inline bool epicsRingPointer::push(T *p) template inline T* epicsRingPointer::pop() { - if(nextPop == nextPush) return(0); - T*p = buffer[nextPop]; - int newNext= nextPop; - ++newNext; - if(newNext >=size) newNext = 0; - nextPop = newNext; + int next = nextPop; + if(next == nextPush) return(0); + T*p = buffer[next]; + ++next; + if(next >=size) next = 0; + nextPop = next; return(p); } template inline void epicsRingPointer::flush() -{ nextPop = nextPush = 0;} +{ + nextPop = 0; + nextPush = 0; +} template inline int epicsRingPointer::getFree() const diff --git a/src/libCom/test/Makefile b/src/libCom/test/Makefile index 567a27ab4..4a99bd98e 100644 --- a/src/libCom/test/Makefile +++ b/src/libCom/test/Makefile @@ -77,6 +77,11 @@ ringPointerTest_SRCS += ringPointerTest.c testHarness_SRCS += ringPointerTest.c TESTS += ringPointerTest +TESTPROD_HOST += ringBytesTest +ringBytesTest_SRCS += ringBytesTest.c +testHarness_SRCS += ringBytesTest.c +TESTS += ringBytesTest + TESTPROD_HOST += epicsEventTest epicsEventTest_SRCS += epicsEventTest.cpp testHarness_SRCS += epicsEventTest.cpp diff --git a/src/libCom/test/epicsRunLibComTests.c b/src/libCom/test/epicsRunLibComTests.c index e5e7058e0..5d9559eab 100644 --- a/src/libCom/test/epicsRunLibComTests.c +++ b/src/libCom/test/epicsRunLibComTests.c @@ -32,6 +32,7 @@ int epicsTimeTest(void); int macLibTest(void); int macEnvExpandTest(void); int ringPointerTest(void); +int ringBytesTest(void); int blockingSockTest(void); int taskwdTest(void); int epicsExitTest(void); @@ -80,6 +81,8 @@ void epicsRunLibComTests(void) runTest(ringPointerTest); + runTest(ringBytesTest); + runTest(blockingSockTest); runTest(taskwdTest); diff --git a/src/libCom/test/ringBytesTest.c b/src/libCom/test/ringBytesTest.c new file mode 100644 index 000000000..6c908a729 --- /dev/null +++ b/src/libCom/test/ringBytesTest.c @@ -0,0 +1,142 @@ +/*************************************************************************\ +* Copyright (c) 2008 UChicago Argonne LLC, as Operator of Argonne +* National Laboratory. +* Copyright (c) 2002 The Regents of the University of California, as +* Operator of Los Alamos National Laboratory. +* EPICS BASE is distributed subject to a Software License Agreement found +* in file LICENSE that is included with this distribution. +\*************************************************************************/ +/* ringBytesTest.c */ + +#include +#include +#include +#include +#include +#include +#include + +#include "epicsThread.h" +#include "epicsRingBytes.h" +#include "errlog.h" +#include "epicsEvent.h" +#include "epicsUnitTest.h" +#include "testMain.h" + +#define RINGSIZE 10 + +static char msg[] = "This is a very long string to be sent through the ring. " + "It will take multiple transmissions, you know."; + +typedef struct info { + epicsEventId consumerEvent; + epicsRingBytesId ring; +}info; + +static void check(epicsRingBytesId ring, int expectedFree) +{ + int expectedUsed = RINGSIZE - expectedFree; + int expectedEmpty = (expectedUsed == 0); + int expectedFull = (expectedFree == 0); + int nFree = epicsRingBytesFreeBytes(ring); + int nUsed = epicsRingBytesUsedBytes(ring); + int isEmpty = epicsRingBytesIsEmpty(ring); + int isFull = epicsRingBytesIsFull(ring); + + testOk(nFree == expectedFree, "Free: %d == %d", nFree, expectedFree); + testOk(nUsed == expectedUsed, "Used: %d == %d", nUsed, expectedUsed); + testOk(isEmpty == expectedEmpty, "Empty: %d == %d", isEmpty, expectedEmpty); + testOk(isFull == expectedFull, "Full: %d == %d", isFull, expectedFull); +} + +static void consumer(void *arg) +{ + info *pinfo = (info *)arg; + char getMsg[sizeof(msg) + RINGSIZE]; + int i = 0; + int n; + + testDiag("Consumer starting"); + while(1) { + epicsEventMustWait(pinfo->consumerEvent); + n = epicsRingBytesGet(pinfo->ring, getMsg+i, RINGSIZE); + testOk(n != 0, "Read from ring"); + i += n; + if (i >= sizeof(msg)) + break; + } + testOk(i==sizeof(msg), "Correct number of bytes received %d==%d", i, (int)sizeof(msg)); + testOk(strcmp(msg, getMsg) == 0, "Message received"); +} + +MAIN(ringBytesTest) +{ + int i, n; + info *pinfo; + epicsEventId consumerEvent; + char put[RINGSIZE+1]; + char get[RINGSIZE+1]; + epicsRingBytesId ring; + + testPlan(242); + + pinfo = calloc(1,sizeof(info)); + if (!pinfo) { + testAbort("calloc failed"); + } + pinfo->consumerEvent = consumerEvent = epicsEventCreate(epicsEventEmpty); + if (!consumerEvent) { + testAbort("epicsEventCreate failed"); + } + + pinfo->ring = ring = epicsRingBytesCreate(RINGSIZE); + if (!ring) { + testAbort("epicsRingBytesCreate failed"); + } + check(ring, RINGSIZE); + + for (i = 0 ; i < sizeof(put) ; i++) + put[i] = i; + for(i = 0 ; i < RINGSIZE ; i++) { + n = epicsRingBytesPut(ring, put, i); + testOk(n==i, "ring put %d", i); + check(ring, RINGSIZE-i); + n = epicsRingBytesGet(ring, get, i); + testOk(n==i, "ring get %d", i); + check(ring, RINGSIZE); + testOk(memcmp(put,get,i)==0, "get matches write"); + } + + for(i = 0 ; i < RINGSIZE ; i++) { + n = epicsRingBytesPut(ring, put+i, 1); + testOk(n==1, "ring put 1, %d", i); + check(ring, RINGSIZE-1-i); + } + n = epicsRingBytesPut(ring, put+RINGSIZE, 1); + testOk(n==0, "put to full ring"); + check(ring, 0); + for(i = 0 ; i < RINGSIZE ; i++) { + n = epicsRingBytesGet(ring, get+i, 1); + testOk(n==1, "ring get 1, %d", i); + check(ring, 1+i); + } + testOk(memcmp(put,get,RINGSIZE)==0, "get matches write"); + n = epicsRingBytesGet(ring, get+RINGSIZE, 1); + testOk(n==0, "get from empty ring"); + check(ring, RINGSIZE); + + epicsThreadCreate("consumer", 50, + epicsThreadGetStackSize(epicsThreadStackSmall), consumer, pinfo); + epicsThreadSleep(0.1); + + for (i = 0; i < sizeof(msg) ; ) { + n = epicsRingBytesPut(ring, msg+i, sizeof(msg)-i); + epicsEventSignal(consumerEvent); + epicsThreadSleep(0.2); + i += n; + } + epicsThreadSleep(0.2); + check(ring, RINGSIZE); + + return testDone(); +} diff --git a/src/libCom/test/ringPointerTest.c b/src/libCom/test/ringPointerTest.c index e176e9057..d6da2ddbe 100644 --- a/src/libCom/test/ringPointerTest.c +++ b/src/libCom/test/ringPointerTest.c @@ -27,7 +27,7 @@ #define ringSize 10 -static int testExit = 0; +static volatile int testExit = 0; typedef struct info { epicsEventId consumerEvent;