From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/5925 Path: news.gmane.org!not-for-mail From: Jens Gustedt Newsgroups: gmane.linux.lib.musl.general Subject: [PATCH 2/2] avoid taking _c_lock if we know it isn't necessary Date: Wed, 27 Aug 2014 11:57:47 +0200 Message-ID: <1409133335.4476.30.camel@eris.loria.fr> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Transfer-Encoding: 7bit X-Trace: ger.gmane.org 1409133489 11461 80.91.229.3 (27 Aug 2014 09:58:09 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Wed, 27 Aug 2014 09:58:09 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-5932-gllmg-musl=m.gmane.org@lists.openwall.com Wed Aug 27 11:58:02 2014 Return-path: Envelope-to: gllmg-musl@plane.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1XMZzO-0007M9-6t for gllmg-musl@plane.gmane.org; Wed, 27 Aug 2014 11:58:02 +0200 Original-Received: (qmail 32104 invoked by uid 550); 27 Aug 2014 09:58:01 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Original-Received: (qmail 32095 invoked from network); 27 Aug 2014 09:58:01 -0000 X-IronPort-AV: E=Sophos;i="5.04,409,1406584800"; d="scan'208";a="91655497" Resent-From: Jens Gustedt Resent-To: musl@lists.openwall.com X-Mailer: Evolution 3.4.4-3 Xref: news.gmane.org gmane.linux.lib.musl.general:5925 Archived-At: This avoids calls to __wait for both, the signaler and the waiter, by eventually adding a __wake to the waiter. Since __wait may unschedule the calling thread and a private __wake is just a not too expensive syscall, this is considered being an advantage. The additional __wake call could be avoided by tracing the fact that the signaler is inside the "ref" count, making it a win-win. --- src/thread/pthread_cond_timedwait.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/src/thread/pthread_cond_timedwait.c b/src/thread/pthread_cond_timedwait.c index 52b1026..9790c84 100644 --- a/src/thread/pthread_cond_timedwait.c +++ b/src/thread/pthread_cond_timedwait.c @@ -25,7 +25,7 @@ struct waiter { struct waiter *prev, *next; int state, barrier, mutex_ret; - int *notify; + int *volatile notify; pthread_mutex_t *mutex; pthread_cond_t *cond; int shared; @@ -42,6 +42,19 @@ static inline void lock(volatile int *l) } } +/* Avoid taking the lock if we know it isn't necessary. */ +static inline int lockRace(volatile int *l, int*volatile* notifier) +{ + int ret = 1; + if (!*notifier && (ret = a_cas(l, 0, 1))) { + a_cas(l, 1, 2); + do __wait(l, 0, 2, 1); + while (!*notifier && (ret = a_cas(l, 0, 2))); + } + return ret; +} + + static inline void unlock(volatile int *l) { if (a_swap(l, 0)==2) @@ -86,21 +99,29 @@ static void unwait(void *arg) pthread_cond_t *c = node->cond; int * ref; - lock(&c->_c_lock); + int locked = lockRace(&c->_c_lock, &node->notify); /* Our membership to the list may have changed while waiting for the lock. */ - /* Ensure that the notify field is only read while we hold the lock. */ + /* This field is written with an atomic op, so we don't need the lock to be taken. */ + /* But it might also have changed while we were waiting, so reload it, again. */ ref = node->notify; /* If there has been no race with a signaler, splice us out of the list. */ /* Otherwise, the signaler has already taken care of it. */ if (!ref) { + /* In this case locked is always 0 and the lock is acquired. */ if (c->_c_head == node) c->_c_head = node->next; else if (node->prev) node->prev->next = node->next; if (c->_c_tail == node) c->_c_tail = node->prev; else if (node->next) node->next->prev = node->prev; } - unlock(&c->_c_lock); + if (!locked) unlock(&c->_c_lock); + + /* Since this leaving waiter might not have held the _c_lock, the following */ + /* __wake might be issued when the signaler is still inside its CS. */ + /* But if so, this avoids a __wait of the signaler, which more important. */ + /* This should not target any spurious wake up in any other thread: */ + /* ref is on the stack of the signaler, and that signaler is still alive. */ if (ref) { if (a_fetch_add(ref, -1)==1) __wake(ref, 1, 1); @@ -178,8 +199,8 @@ int __private_cond_signal(pthread_cond_t *c, int n) lock(&c->_c_lock); for (p=c->_c_tail; n && p; p=p->prev) { if (a_cas(&p->state, WAITING, SIGNALED) != WAITING) { - ref++; - p->notify = &ref; + a_fetch_add(&ref, 1); + a_cas_p(&p->notify, 0, &ref); /* Let the signaler do all work concerning consistency of the list. */ if (c->_c_head == p) c->_c_head = p->next; else if (p->prev) p->prev->next = p->next; -- 1.7.10.4