On 22.01.23 13:52, Marko Mäkelä wrote:
Hi,
I would propose the following patch, or some equivalent interface that would allow cThread::mutex to be used with some cCondVar in derived classes:
diff --git a/thread.h b/thread.h index 16c4bd75..cd1d98ab 100644 --- a/thread.h +++ b/thread.h @@ -83,7 +83,9 @@ private: bool running; pthread_t childTid; tThreadId childThreadId; +protected: cMutex mutex; +private: char *description; bool lowPriority; static tThreadId mainThreadId;
I don't like the idea of exposing that mutex. If you really need such functionality, please suggest a function that does this without exposing the mutex.
Because cThread::mutx is declared private and there is no helper member function, derived classes that wish to use condition variables have to instantiate a separate cMutex for that. Here is an example from the rpihddevice plugin that illustrates my point:
diff --git a/omx.c b/omx.c --- a/omx.c +++ b/omx.c @@ -119,17 +119,17 @@ const char* cOmx::errStr(int err) void cOmx::Action(void) { cTimeMs timer; - m_mutex.Lock(); + Lock(); while (Running()) { while (m_portEvents.empty()) - if (m_portEventsAdded.TimedWait(m_mutex, 10)) + if (!m_portEventsAdded.TimedWait(mutex, 10)) goto timeout;
{ const Event event = m_portEvents.front(); m_portEvents.pop(); - m_mutex.Unlock(); + Unlock();
switch (event.event) {
Actually, there is a bug above: the condition for the TimedWait() call was inverted.
This code illustrates another limitation: There is no way to pass an absolute time to cCondVar::TimedWait(). On each call, a relative wake-up time (milliseconds from the current time) will be converted into an absolute time. If there was a way, we would be able to remove the "cTimeMs timer" and some related system calls, and have this loop both wake up every 100 milliseconds, and process events as soon as they arrive. Here is the VDR part of the patch:
diff --git a/thread.c b/thread.c index 93eb8c0d..3dcb44d4 100644 --- a/thread.c +++ b/thread.c @@ -36,7 +36,7 @@ #define dbglocking(a...) #endif
-static bool GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow) +bool cCondVar::GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow) { struct timeval now; if (gettimeofday(&now, NULL) == 0) { // get current time
What is the rationale behind this change, other than having to call it with cCondVar::GetAbsTime()?
@@ -81,7 +81,7 @@ bool cCondWait::Wait(int TimeoutMs) if (!signaled) { if (TimeoutMs) { struct timespec abstime; - if (GetAbsTime(&abstime, TimeoutMs)) { + if (cCondVar::GetAbsTime(&abstime, TimeoutMs)) { while (!signaled) { if (pthread_cond_timedwait(&cond, &mutex, &abstime) == ETIMEDOUT) break; @@ -129,20 +129,27 @@ void cCondVar::Wait(cMutex &Mutex) } }
+bool cCondVar::TimedWait(cMutex &Mutex, const timespec &abstime) +{ + int err = 0;
+ if (int locked = Mutex.locked) { + Mutex.locked = 0; // have to clear the locked count here, as pthread_cond_timedwait + // does an implicit unlock of the mutex. + err = pthread_cond_timedwait(&cond, &Mutex.mutex, &abstime); + Mutex.locked = locked; + } + return err != ETIMEDOUT; +}
bool cCondVar::TimedWait(cMutex &Mutex, int TimeoutMs) { bool r = true; // true = condition signaled, false = timeout
if (Mutex.locked) { struct timespec abstime; - if (GetAbsTime(&abstime, TimeoutMs)) { - int locked = Mutex.locked; - Mutex.locked = 0; // have to clear the locked count here, as pthread_cond_timedwait - // does an implicit unlock of the mutex. - if (pthread_cond_timedwait(&cond, &Mutex.mutex, &abstime) == ETIMEDOUT) - r = false; - Mutex.locked = locked; - } + if (GetAbsTime(&abstime, TimeoutMs)) + r = TimedWait(Mutex, abstime); } return r; } @@ -174,7 +181,7 @@ bool cRwLock::Lock(bool Write, int TimeoutMs) int Result = 0; struct timespec abstime; if (TimeoutMs) { - if (!GetAbsTime(&abstime, TimeoutMs)) + if (!cCondVar::GetAbsTime(&abstime, TimeoutMs)) TimeoutMs = 0; } if (Write) { diff --git a/thread.h b/thread.h index 16c4bd75..04bb4cc5 100644 --- a/thread.h +++ b/thread.h @@ -49,7 +49,9 @@ public: ~cCondVar(); void Wait(cMutex &Mutex); bool TimedWait(cMutex &Mutex, int TimeoutMs); + bool TimedWait(cMutex &Mutex, const timespec &abstime); void Broadcast(void); + static bool GetAbsTime(struct timespec *Abstime, int MillisecondsFromNow); };
class cRwLock {
So where and how would you be using this new function?
I did not complete the change to rpihddevice cOmx::Action() yet. We may be forced to retain two mutexes after all
You want to expose the cThread::mutex in order to avoid an additional mutex in the derived class, but may be forced to retain two mutexes after all - what am I missing here?
Klaus