From patchwork Sun Jan 22 12:52:03 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: =?utf-8?b?TWFya28gTcOka2Vsw6Q=?= X-Patchwork-Id: 89119 Received: from [127.0.0.1] by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from ) id 1pJZpR-006XxM-4f; Sun, 22 Jan 2023 12:52:09 +0000 Received: from meesny.iki.fi ([195.140.195.201]) by www.linuxtv.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1pJZpO-006XxA-Es for vdr@linuxtv.org; Sun, 22 Jan 2023 12:52:07 +0000 Received: from jyty (dsl-hkibng31-54fae6-199.dhcp.inet.fi [84.250.230.199]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) (Authenticated sender: msmakela) by meesny.iki.fi (Postfix) with ESMTPSA id 2869320124 for ; Sun, 22 Jan 2023 14:52:04 +0200 (EET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1674391924; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type; bh=oMI6pWf2hchGz7FbxWFNnPVYE5HdSnInbi07NfTVIhk=; b=xU1JkB+uGtpvo9eT+vpoe24Rhe+H4XKSyfH2pkZbw8XnkmEobYXB8+Wz1+NMbKwnweniTr rLhCvV3t9E2vyEfrs2Tlaf9fZjFaSeavfom8fcdII7pQLfso6X2GVs3ejg7nYbVZoclHvv SSKApLBkLEHnUyvkAZvOmqbM0bb88ck= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=iki.fi; s=meesny; t=1674391924; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type; bh=oMI6pWf2hchGz7FbxWFNnPVYE5HdSnInbi07NfTVIhk=; b=aAvbjbexhZyq3mA1NrcTGdoIs5PPziS1ysxmhF+icqxB0I45JQXqPUzFT8+a7e4nwncVZb rjRgQf0X5n1bDYGFfCuhH94dWdwmbYXJ8U8p+ftMuKpy41sl89OTZ9RWDvFCbtZl/lE0ld 8g2oym8JeWfeJBdC3td1aREnwF10kXY= ARC-Authentication-Results: i=1; ORIGINATING; auth=pass smtp.auth=msmakela smtp.mailfrom=marko.makela@iki.fi ARC-Seal: i=1; s=meesny; d=iki.fi; t=1674391924; a=rsa-sha256; cv=none; b=rcvwOMT/h7vjaIPhib5Ph9dj3c/8+65yi6mAIiiLnyF+6vPcwls5MOsDQck0vfBK5q7mLV hh5pWLjlnvL0gwQHS44brSR++U1nMeYqYv09f3iu50PMHTmi2G97OTzx4o/LmDpZHDTeGf W+i2aZIvqKSpoi37H8FOWQ3iG1SCkI0= Date: Sun, 22 Jan 2023 14:52:03 +0200 From: Marko =?iso-8859-1?q?M=E4kel=E4?= To: VDR Mailing List Message-ID: MIME-Version: 1.0 Content-Disposition: inline X-LSpam-Score: -1.1 (-) X-LSpam-Report: No, score=-1.1 required=5.0 tests=BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FROM_EXCESS_BASE64=0.979 autolearn=ham autolearn_force=no Subject: [vdr] [PATCH] Use cThread::mutex with absolute cCondVar::TimedWait() X-BeenThere: vdr@linuxtv.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: VDR Mailing List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: VDR Mailing List Errors-To: vdr-bounces@linuxtv.org Sender: "vdr" 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 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; 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 @@ -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 {