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