This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has submitted this change and it was merged. ( https://gerrit.osmocom.org/c/osmo-trx/+/15524 ) Change subject: radioInterface: Atomically fetch and change underrun variable ...................................................................... radioInterface: Atomically fetch and change underrun variable Otherwise, it could happen that underrun events are lost: TxLower (isUnderrun): RxLower (pullBuffer): read(underrun) read(underrun) write(underrun, |val) [maybe underrun becomes TRUE] write(underrun, false) Similary, it could happen the other direction if atomic was only applied to isUnderrun: TxLower (isUnderrun): RxLower (pullBuffer): read(underrun) -> true read(underrun)-> true write(underrun, false) write(underrun, true|val) where val=false So in here isUnderrun would return true twice while it should only return one. Change-Id: I684e0a5d2a9583a161d5a6593559b3a9e7cd57e3 --- M CommonLibs/Threads.cpp M CommonLibs/Threads.h M Transceiver52M/osmo-trx.cpp M Transceiver52M/radioInterface.cpp M Transceiver52M/radioInterface.h M Transceiver52M/radioInterfaceMulti.cpp M Transceiver52M/radioInterfaceResamp.cpp M configure.ac 8 files changed, 61 insertions(+), 19 deletions(-) Approvals: Jenkins Builder: Verified fixeria: Looks good to me, but someone else must approve laforge: Looks good to me, approved diff --git a/CommonLibs/Threads.cpp b/CommonLibs/Threads.cpp index dd57d40..020d94e 100644 --- a/CommonLibs/Threads.cpp +++ b/CommonLibs/Threads.cpp @@ -40,7 +40,9 @@ using namespace std; - +#ifndef HAVE_ATOMIC_OPS + pthread_mutex_t atomic_ops_mutex = PTHREAD_MUTEX_INITIALIZER; +#endif Mutex gStreamLock; ///< Global lock to control access to cout and cerr. diff --git a/CommonLibs/Threads.h b/CommonLibs/Threads.h index 4cc0884..df61c72 100644 --- a/CommonLibs/Threads.h +++ b/CommonLibs/Threads.h @@ -28,6 +28,8 @@ #ifndef THREADS_H #define THREADS_H +#include "config.h" + #include <pthread.h> #include <iostream> #include <assert.h> @@ -188,8 +190,30 @@ void cancel() { pthread_cancel(mThread); } }; +#ifdef HAVE_ATOMIC_OPS +#define osmo_trx_sync_fetch_and_and(ptr, value) __sync_fetch_and_and((ptr), (value)) +#define osmo_trx_sync_or_and_fetch(ptr, value) __sync_or_and_fetch((ptr), (value)) +#else +extern pthread_mutex_t atomic_ops_mutex; +static inline int osmo_trx_sync_fetch_and_and(int *ptr, int value) +{ + pthread_mutex_lock(&atomic_ops_mutex); + int tmp = *ptr; + *ptr &= value; + pthread_mutex_unlock(&atomic_ops_mutex); + return tmp; +} - +static inline int osmo_trx_sync_or_and_fetch(int *ptr, int value) +{ + int tmp; + pthread_mutex_lock(&atomic_ops_mutex); + *ptr |= value; + tmp = *ptr; + pthread_mutex_unlock(&atomic_ops_mutex); + return tmp; +} +#endif #endif // vim: ts=4 sw=4 diff --git a/Transceiver52M/osmo-trx.cpp b/Transceiver52M/osmo-trx.cpp index ab0b631..9fe6585 100644 --- a/Transceiver52M/osmo-trx.cpp +++ b/Transceiver52M/osmo-trx.cpp @@ -571,6 +571,11 @@ #endif #endif +#ifndef HAVE_ATOMIC_OPS +#pragma message ("Built without atomic operation support. Using Mutex, it may affect performance!") + printf("Built without atomic operation support. Using Mutex, it may affect performance!\n"); +#endif + if (!log_mutex_init()) { fprintf(stderr, "Failed to initialize log mutex!\n"); exit(2); diff --git a/Transceiver52M/radioInterface.cpp b/Transceiver52M/radioInterface.cpp index 6e49a75..fbcacf1 100644 --- a/Transceiver52M/radioInterface.cpp +++ b/Transceiver52M/radioInterface.cpp @@ -24,6 +24,7 @@ #include "radioInterface.h" #include "Resampler.h" #include <Logger.h> +#include <Threads.h> extern "C" { #include "convert.h" @@ -288,9 +289,9 @@ bool RadioInterface::isUnderrun() { - bool retVal = underrun; - underrun = false; - + bool retVal; + /* atomically get previous value of "underrun" and set the var to false */ + retVal = osmo_trx_sync_fetch_and_and(&underrun, false); return retVal; } @@ -340,7 +341,7 @@ segmentLen * 2); } - underrun |= local_underrun; + osmo_trx_sync_or_and_fetch(&underrun, local_underrun); readTimestamp += numRecv; return 0; } @@ -366,7 +367,7 @@ segmentLen, &local_underrun, writeTimestamp); - underrun |= local_underrun; + osmo_trx_sync_or_and_fetch(&underrun, local_underrun); writeTimestamp += numSent; return true; diff --git a/Transceiver52M/radioInterface.h b/Transceiver52M/radioInterface.h index b12c187..d72fb69 100644 --- a/Transceiver52M/radioInterface.h +++ b/Transceiver52M/radioInterface.h @@ -48,7 +48,7 @@ std::vector<short *> convertRecvBuffer; std::vector<short *> convertSendBuffer; std::vector<float> powerScaling; - bool underrun; ///< indicates writes to USRP are too slow + int underrun; ///< indicates writes to USRP are too slow bool overrun; ///< indicates reads from USRP are too slow TIMESTAMP writeTimestamp; ///< sample timestamp of next packet written to USRP TIMESTAMP readTimestamp; ///< sample timestamp of next packet read from USRP diff --git a/Transceiver52M/radioInterfaceMulti.cpp b/Transceiver52M/radioInterfaceMulti.cpp index eec426e..92e31e1 100644 --- a/Transceiver52M/radioInterfaceMulti.cpp +++ b/Transceiver52M/radioInterfaceMulti.cpp @@ -251,7 +251,7 @@ convert_short_float((float *) outerRecvBuffer->begin(), convertRecvBuffer[0], 2 * outerRecvBuffer->size()); - underrun |= local_underrun; + osmo_trx_sync_or_and_fetch(&underrun, local_underrun); readTimestamp += num; channelizer->rotate((float *) outerRecvBuffer->begin(), @@ -348,7 +348,7 @@ LOG(ALERT) << "Transmit error " << num; } - underrun |= local_underrun; + osmo_trx_sync_or_and_fetch(&underrun, local_underrun); writeTimestamp += num; return true; diff --git a/Transceiver52M/radioInterfaceResamp.cpp b/Transceiver52M/radioInterfaceResamp.cpp index 864cdee..b92432f 100644 --- a/Transceiver52M/radioInterfaceResamp.cpp +++ b/Transceiver52M/radioInterfaceResamp.cpp @@ -184,7 +184,7 @@ convert_short_float((float *) outerRecvBuffer->begin(), convertRecvBuffer[0], 2 * resamp_outchunk); - underrun |= local_underrun; + osmo_trx_sync_or_and_fetch(&underrun, local_underrun); readTimestamp += (TIMESTAMP) resamp_outchunk; /* Write to the end of the inner receive buffer */ @@ -232,7 +232,7 @@ LOG(ALERT) << "Transmit error " << numSent; } - underrun |= local_underrun; + osmo_trx_sync_or_and_fetch(&underrun, local_underrun); writeTimestamp += resamp_outchunk; return true; diff --git a/configure.ac b/configure.ac index fa9cf8d..350c77c 100644 --- a/configure.ac +++ b/configure.ac @@ -190,29 +190,39 @@ AC_DEFUN([CHECK_BUILTIN_SUPPORT], [ AC_CACHE_CHECK( [whether ${CC} has $1 built-in], - [osmo_cv_cc_has_builtin], [ + [osmo_cv_cc_has_$1], [ AC_LINK_IFELSE([ AC_LANG_PROGRAM([], [ - __builtin_cpu_supports("sse"); + $2 ]) ], - [AS_VAR_SET([osmo_cv_cc_has_builtin], [yes])], - [AS_VAR_SET([osmo_cv_cc_has_builtin], [no])]) + [AS_VAR_SET([osmo_cv_cc_has_$1], [yes])], + [AS_VAR_SET([osmo_cv_cc_has_$1], [no])]) ] ) - AS_IF([test yes = AS_VAR_GET([osmo_cv_cc_has_builtin])], [ + AS_IF([test yes = AS_VAR_GET([osmo_cv_cc_has_$1])], [ AC_DEFINE_UNQUOTED(AS_TR_CPP(HAVE_$1), 1, [Define to 1 if compiler has the '$1' built-in function]) ], [ - AC_MSG_WARN($2) + AC_MSG_WARN($3) ]) ]) dnl Check if the compiler supports runtime SIMD detection -CHECK_BUILTIN_SUPPORT([__builtin_cpu_supports], +CHECK_BUILTIN_SUPPORT([__builtin_cpu_supports], [__builtin_cpu_supports("sse");], [Runtime SIMD detection will be disabled]) +dnl Check for __sync_fetch_and_add(). +CHECK_BUILTIN_SUPPORT([__sync_fetch_and_and], [int x;__sync_fetch_and_and(&x,1);], + [Atomic operation not available, will use mutex]) +dnl Check for __sync_or_and_fetch(). +CHECK_BUILTIN_SUPPORT([__sync_or_and_fetch], [int x;__sync_or_and_fetch(&x,1);], + [Atomic operation not available, will use mutex]) +AS_IF([test "x$osmo_cv_cc_has___sync_fetch_and_and" = "xyes" && test "x$osmo_cv_cc_has___sync_or_and_fetch" = "xyes"], [ + AC_DEFINE(HAVE_ATOMIC_OPS, 1, [Support all required atomic operations], [AC_MSG_WARN("At least one aotmic operation missing, will use mutex")]) +]) + AM_CONDITIONAL(DEVICE_UHD, [test "x$with_uhd" != "xno"]) AM_CONDITIONAL(DEVICE_USRP1, [test "x$with_usrp1" = "xyes"]) AM_CONDITIONAL(DEVICE_LMS, [test "x$with_lms" = "xyes"]) -- To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/15524 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-Change-Id: I684e0a5d2a9583a161d5a6593559b3a9e7cd57e3 Gerrit-Change-Number: 15524 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann at sysmocom.de> Gerrit-Reviewer: fixeria <axilirator at gmail.com> Gerrit-Reviewer: laforge <laforge at osmocom.org> Gerrit-Reviewer: pespin <pespin at sysmocom.de> Gerrit-Reviewer: tnt <tnt at 246tNt.com> Gerrit-MessageType: merged -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190928/1f9c23f8/attachment.htm>