Use cThread::mutex with absolute cCondVar::TimedWait()

  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:


I did not complete the change to rpihddevice cOmx::Action() yet. We may 
be forced to retain two mutexes after all, because OMX_EmptyThisBuffer() 
as well as one of the functions called from cOmx::StopAudio() would hang 
indefinitely, while holding the cOmx::mutex, blocking the cOmx::Action() 
thread. According to 
https://forums.raspberrypi.com/viewtopic.php?t=313170 the interface is 
largely deprecated. I will try to figure out something.

Best regards,

	Marko
  

Sun, Jan 22, 2023 at 02:52:03PM +0200, Marko Mäkelä wrote:
>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:

At https://github.com/reufer/rpihddevice/pull/14#discussion_r1083466069
I posted an example of using an absolute variant of 
cCondVar::TimedWait(). The thread cOmx::Action() would perform some 
action every 100 milliseconds, and additionally consume m_portEvents as 
soon as events are enqueued by cOmx::Add() in another thread.

	Marko
  

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
  

Wed, Feb 15, 2023 at 06:01:46PM +0100, Klaus Schmidinger wrote:
>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.

An alternative might be to add a member functions to cThread that would 
take a cCondVar& as a parameter and invoke it with the private 
cThread::mutex. But, we can disregard this idea; see below.

[snip]
>>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?

Meanwhile, I concluded that the only option is to have two mutexes in 
the rpihddevice class cOmx. The cThread::mutex makes calls to the 
ilclient and OMX thread-safe, and a private mutex of cOmx protects
data in callback functions that may be invoked from other threads of 
that external library. An attempt to acquire the cThread::mutex in the 
callback code would lead to a deadlock.

It looks like a simple way to reduce the number of system calls in the 
plugin is to use the POSIX standard pthread_mutex_t and pthread_cond_t 
directly, or via the C++11 std::mutex and std::condition_variable. So, 
there is no pressing need to change anything in the VDR core.

	Marko