This is an automated email from the ASF dual-hosted git repository.
oknet pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/trafficserver.git
The following commit(s) were added to refs/heads/master by this push:
new 8ae5611 Optimize: Do not signal EThreads which are not blocked on
cond_timedwait.
8ae5611 is described below
commit 8ae5611ae1c555198a4ddeda68055ac4ff20afd5
Author: Oknet Xu <xuc...@skyguard.com.cn>
AuthorDate: Wed Dec 19 20:40:24 2018 +0800
Optimize: Do not signal EThreads which are not blocked on cond_timedwait.
From the man page of `pthread_cond_timedwait()`:
```
The pthread_cond_timedwait() function atomically blocks the current
thread waiting on the condition variable specified by cond, and releases
the mutex specified by mutex. The waiting thread unblocks only after
another thread calls pthread_cond_signal(3), or
pthread_cond_broadcast(3) with the same condition variable, or if the
system time reaches the time specified in abstime, and the current
thread reacquires the lock on mutex.
```
Refer to the code of `ProtectedQueue::wait()`,
```
ink_mutex_acquire(&lock);
if (INK_ATOMICLIST_EMPTY(al)) {
timespec ts = ink_hrtime_to_timespec(timeout);
ink_cond_timedwait(&might_have_data, &lock, &ts);
}
ink_mutex_release(&lock);
```
The `EThread::lock` is acquired and then released immediatelly by
`ink_cond_timedwait()`.
When the thread unblocks, the thread reacquires the lock on
`EThread::lock`, and then released immediatelly.
Refer to the code of `ProtectedQueue::signal()`,
```
TS_INLINE void
ProtectedQueue::signal()
{
// Need to get the lock before you can signal the thread
ink_mutex_acquire(&lock);
ink_cond_signal(&might_have_data);
ink_mutex_release(&lock);
}
```
The threads that try to send signal to the same target Event Thread are
blocked on the mutex and send meaningless wakeup signals one by one.
It is not necessory,
- To acquire and release the `EThread::lock` again and again in the
event loop,
- To send wakeup signal to an Event Thread that does not blocked on
`pthread_cond_timedwait()`.
Changes of the commit:
The Event Thread has two status: busy and sleep,
- Keep `EThread::lock` locked while Event Thread is busy,
- The `EThread::lock` is released while Event Thread is sleep.
When other threads try to acquire the `EThread::lock` of the target
Event Thread,
- Acquired, indicating that the target Event Thread is sleep, must send
a wakeup signal to the Event Thread.
- Failed, indicating that the target Event Thread is busy, do nothing.
Backports:
If you backport the commit to the branch without PR#2541,
- It is safe to remove `flush_signals()` from the event loop.
- Also safe to remove any code related with `ethreads_to_be_signalled[]`
and `n_ethreads_to_be_signalled`.
- Always do `try_signal()` within `ProtectedQueue::enqueue()`.
- Suggest to remove `ProtectedQueue::signal()`.
---
iocore/eventsystem/I_EThread.h | 7 ++++++-
iocore/eventsystem/ProtectedQueue.cc | 17 ++++++++---------
iocore/eventsystem/UnixEThread.cc | 9 +++++++++
3 files changed, 23 insertions(+), 10 deletions(-)
diff --git a/iocore/eventsystem/I_EThread.h b/iocore/eventsystem/I_EThread.h
index 7f62545..8ce926a 100644
--- a/iocore/eventsystem/I_EThread.h
+++ b/iocore/eventsystem/I_EThread.h
@@ -371,7 +371,12 @@ public:
void
signalActivity() override
{
- _q.signal();
+ /* Try to acquire the `EThread::lock` of the Event Thread:
+ * - Acquired, indicating that the Event Thread is sleep,
+ * must send a wakeup signal to the Event Thread.
+ * - Failed, indicating that the Event Thread is busy, do nothing.
+ */
+ (void)_q.try_signal();
}
ProtectedQueue &_q;
diff --git a/iocore/eventsystem/ProtectedQueue.cc
b/iocore/eventsystem/ProtectedQueue.cc
index c4b5377..bd434e7 100644
--- a/iocore/eventsystem/ProtectedQueue.cc
+++ b/iocore/eventsystem/ProtectedQueue.cc
@@ -114,15 +114,14 @@ ProtectedQueue::dequeue_external()
void
ProtectedQueue::wait(ink_hrtime timeout)
{
- // If there are no external events available, don't do a cond_timedwait.
+ /* If there are no external events available, will do a cond_timedwait.
+ *
+ * - The `EThread::lock` will be released,
+ * - And then the Event Thread goes to sleep and waits for the wakeup
signal of `EThread::might_have_data`,
+ * - The `EThread::lock` will be locked again when the Event Thread wakes
up.
+ */
if (INK_ATOMICLIST_EMPTY(al)) {
- ink_mutex_acquire(&lock);
- // The "al" may have new events while waiting for the mutex become
available.
- // We have to recheck the external queue again.
- if (INK_ATOMICLIST_EMPTY(al)) {
- timespec ts = ink_hrtime_to_timespec(timeout);
- ink_cond_timedwait(&might_have_data, &lock, &ts);
- }
- ink_mutex_release(&lock);
+ timespec ts = ink_hrtime_to_timespec(timeout);
+ ink_cond_timedwait(&might_have_data, &lock, &ts);
}
}
diff --git a/iocore/eventsystem/UnixEThread.cc
b/iocore/eventsystem/UnixEThread.cc
index a5c72c5..06920db 100644
--- a/iocore/eventsystem/UnixEThread.cc
+++ b/iocore/eventsystem/UnixEThread.cc
@@ -323,7 +323,16 @@ EThread::execute()
switch (tt) {
case REGULAR: {
+ /* The Event Thread has two status: busy and sleep:
+ * - Keep `EThread::lock` locked while Event Thread is busy,
+ * - The `EThread::lock` is released while Event Thread is sleep.
+ * When other threads try to acquire the `EThread::lock` of the target
Event Thread:
+ * - Acquired, indicating that the target Event Thread is sleep,
+ * - Failed, indicating that the target Event Thread is busy.
+ */
+ ink_mutex_acquire(&EventQueueExternal.lock);
this->execute_regular();
+ ink_mutex_release(&EventQueueExternal.lock);
break;
}
case DEDICATED: {
