* [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu [not found] ` <4CCBDE0B.8060102@redhat.com> @ 2010-11-01 20:04 ` Paul E. McKenney 2010-11-01 20:25 ` Mathieu Desnoyers 0 siblings, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2010-11-01 20:04 UTC (permalink / raw) On Sat, Oct 30, 2010 at 10:57:47AM +0200, Paolo Bonzini wrote: > >+static void *call_rcu_thread(void *arg) > >+{ > >+ [...] > >+ else { > >+ call_rcu_lock(&crdp->mtx); > >+ crdp->flags &= ~URCU_CALL_RCU_RUNNING; > >+ if (&cbs->next != cbs_tail&& > >+ pthread_cond_wait(&crdp->cond,&crdp->mtx) != 0) { > >+ perror("pthread_cond_wait"); > >+ exit(-1); > >+ } else > >+ poll(NULL, 0, 10); > >+ crdp->flags |= URCU_CALL_RCU_RUNNING; > >+ call_rcu_unlock(&crdp->mtx); > >+ } > >+ } > >+ return NULL; /* NOTREACHED */ > >+} > > Given the way you handle URCU_CALL_RCU_RUNNING above, the flag will > be reset in call_rcu iff call_rcu sees contention on the lock. Doesn't pthread_cond_wait() release the mutex for the duration of the wait? Ah, are you worried about the poll() under the lock? I am moving this out from under the lock. > >+ call_rcu_lock(&crdp->mtx); > >+ if (!(crdp->flags& URCU_CALL_RCU_RUNNING)) { > >+ if (pthread_cond_signal(&crdp->cond) != 0) { > >+ perror("pthread_cond_signal"); > >+ exit(-1); > >+ } > >+ } > >+ call_rcu_unlock(&crdp->mtx); > >+ } > >+} > > So, the mutex is basically unnecessary if some futex magic replaces > the condition variable. For example, in the thread: > > else { > retry: > flags = crdp->flags; > if ((flags & URCU_CALL_RCU_REQUESTED)) > continue; > if (cmpxchg (&crdp->flags, flags, > flags & ~URCU_CALL_RCU_RUNNING) != flags) > goto retry; > > futex_wait (&crdp->flags, > flags & ~URCU_CALL_RCU_RUNNING); > } > > and in call_rcu: > > mb (); > /* If the thread is not blocked, it will see our request. */ > do { > flags = crdp->flags; > /* If there's already a request pending, no need to > wake up the process. If the thread is running, no > need to do anything, it'll pick up our request. */ > if (flags & > (URCU_CALL_RCU_REQUESTED | URCU_CALL_RCU_RUNNING)) > return; > } while (cmpxchg (&crdp->flags, flags, > flags | URCU_CALL_RCU_REQUESTED) != flags); > futex_wake (&crdp->flags, 1); OK, sounds like a nice optimization, though a bit Linux-specific. I will stick with the POSIX stuff for the moment, and once I am convinced that it really is working, I might consider doing futexes if running on Linux. Thanx, Paul ^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu 2010-11-01 20:04 ` [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu Paul E. McKenney @ 2010-11-01 20:25 ` Mathieu Desnoyers 2010-11-02 12:12 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Mathieu Desnoyers @ 2010-11-01 20:25 UTC (permalink / raw) * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote: > On Sat, Oct 30, 2010 at 10:57:47AM +0200, Paolo Bonzini wrote: > > >+static void *call_rcu_thread(void *arg) > > >+{ > > >+ [...] > > >+ else { > > >+ call_rcu_lock(&crdp->mtx); > > >+ crdp->flags &= ~URCU_CALL_RCU_RUNNING; > > >+ if (&cbs->next != cbs_tail&& > > >+ pthread_cond_wait(&crdp->cond,&crdp->mtx) != 0) { > > >+ perror("pthread_cond_wait"); > > >+ exit(-1); > > >+ } else > > >+ poll(NULL, 0, 10); > > >+ crdp->flags |= URCU_CALL_RCU_RUNNING; > > >+ call_rcu_unlock(&crdp->mtx); > > >+ } > > >+ } > > >+ return NULL; /* NOTREACHED */ > > >+} > > > > Given the way you handle URCU_CALL_RCU_RUNNING above, the flag will > > be reset in call_rcu iff call_rcu sees contention on the lock. > > Doesn't pthread_cond_wait() release the mutex for the duration of the > wait? Ah, are you worried about the poll() under the lock? I am > moving this out from under the lock. > > > >+ call_rcu_lock(&crdp->mtx); > > >+ if (!(crdp->flags& URCU_CALL_RCU_RUNNING)) { > > >+ if (pthread_cond_signal(&crdp->cond) != 0) { > > >+ perror("pthread_cond_signal"); > > >+ exit(-1); > > >+ } > > >+ } > > >+ call_rcu_unlock(&crdp->mtx); > > >+ } > > >+} > > > > So, the mutex is basically unnecessary if some futex magic replaces > > the condition variable. For example, in the thread: > > > > else { > > retry: > > flags = crdp->flags; > > if ((flags & URCU_CALL_RCU_REQUESTED)) > > continue; > > if (cmpxchg (&crdp->flags, flags, > > flags & ~URCU_CALL_RCU_RUNNING) != flags) > > goto retry; > > > > futex_wait (&crdp->flags, > > flags & ~URCU_CALL_RCU_RUNNING); > > } > > > > and in call_rcu: > > > > mb (); > > /* If the thread is not blocked, it will see our request. */ > > do { > > flags = crdp->flags; > > /* If there's already a request pending, no need to > > wake up the process. If the thread is running, no > > need to do anything, it'll pick up our request. */ > > if (flags & > > (URCU_CALL_RCU_REQUESTED | URCU_CALL_RCU_RUNNING)) > > return; > > } while (cmpxchg (&crdp->flags, flags, > > flags | URCU_CALL_RCU_REQUESTED) != flags); > > futex_wake (&crdp->flags, 1); > > OK, sounds like a nice optimization, though a bit Linux-specific. > I will stick with the POSIX stuff for the moment, and once I am > convinced that it really is working, I might consider doing futexes > if running on Linux. Paul, this is why I created urcu-futex.h. It offers the futex API, but with a polling or pthread-cond-based fallback for non-Linux platforms. So please feel free to implement this using futex_noasync() (this is the version using a pthread_cond() fallback, which is not signal-safe, but does not require polling). Thanks, Mathieu -- Mathieu Desnoyers Operating System Efficiency R&D Consultant EfficiOS Inc. http://www.efficios.com ^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu 2010-11-01 20:25 ` Mathieu Desnoyers @ 2010-11-02 12:12 ` Paul E. McKenney 2010-11-11 8:44 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Paul E. McKenney @ 2010-11-02 12:12 UTC (permalink / raw) On Mon, Nov 01, 2010 at 04:25:30PM -0400, Mathieu Desnoyers wrote: > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote: > > On Sat, Oct 30, 2010 at 10:57:47AM +0200, Paolo Bonzini wrote: > > > >+static void *call_rcu_thread(void *arg) > > > >+{ > > > >+ [...] > > > >+ else { > > > >+ call_rcu_lock(&crdp->mtx); > > > >+ crdp->flags &= ~URCU_CALL_RCU_RUNNING; > > > >+ if (&cbs->next != cbs_tail&& > > > >+ pthread_cond_wait(&crdp->cond,&crdp->mtx) != 0) { > > > >+ perror("pthread_cond_wait"); > > > >+ exit(-1); > > > >+ } else > > > >+ poll(NULL, 0, 10); > > > >+ crdp->flags |= URCU_CALL_RCU_RUNNING; > > > >+ call_rcu_unlock(&crdp->mtx); > > > >+ } > > > >+ } > > > >+ return NULL; /* NOTREACHED */ > > > >+} > > > > > > Given the way you handle URCU_CALL_RCU_RUNNING above, the flag will > > > be reset in call_rcu iff call_rcu sees contention on the lock. > > > > Doesn't pthread_cond_wait() release the mutex for the duration of the > > wait? Ah, are you worried about the poll() under the lock? I am > > moving this out from under the lock. > > > > > >+ call_rcu_lock(&crdp->mtx); > > > >+ if (!(crdp->flags& URCU_CALL_RCU_RUNNING)) { > > > >+ if (pthread_cond_signal(&crdp->cond) != 0) { > > > >+ perror("pthread_cond_signal"); > > > >+ exit(-1); > > > >+ } > > > >+ } > > > >+ call_rcu_unlock(&crdp->mtx); > > > >+ } > > > >+} > > > > > > So, the mutex is basically unnecessary if some futex magic replaces > > > the condition variable. For example, in the thread: > > > > > > else { > > > retry: > > > flags = crdp->flags; > > > if ((flags & URCU_CALL_RCU_REQUESTED)) > > > continue; > > > if (cmpxchg (&crdp->flags, flags, > > > flags & ~URCU_CALL_RCU_RUNNING) != flags) > > > goto retry; > > > > > > futex_wait (&crdp->flags, > > > flags & ~URCU_CALL_RCU_RUNNING); > > > } > > > > > > and in call_rcu: > > > > > > mb (); > > > /* If the thread is not blocked, it will see our request. */ > > > do { > > > flags = crdp->flags; > > > /* If there's already a request pending, no need to > > > wake up the process. If the thread is running, no > > > need to do anything, it'll pick up our request. */ > > > if (flags & > > > (URCU_CALL_RCU_REQUESTED | URCU_CALL_RCU_RUNNING)) > > > return; > > > } while (cmpxchg (&crdp->flags, flags, > > > flags | URCU_CALL_RCU_REQUESTED) != flags); > > > futex_wake (&crdp->flags, 1); > > > > OK, sounds like a nice optimization, though a bit Linux-specific. > > I will stick with the POSIX stuff for the moment, and once I am > > convinced that it really is working, I might consider doing futexes > > if running on Linux. > > Paul, this is why I created urcu-futex.h. It offers the futex API, but with a > polling or pthread-cond-based fallback for non-Linux platforms. So please feel > free to implement this using futex_noasync() (this is the version using a > pthread_cond() fallback, which is not signal-safe, but does not require > polling). OK, once I become more confident that I don't have bugs elsewhere, I will try out this approach. Thanx, Paul ^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu 2010-11-02 12:12 ` Paul E. McKenney @ 2010-11-11 8:44 ` Paolo Bonzini 2010-11-12 0:24 ` Paolo Bonzini 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2010-11-11 8:44 UTC (permalink / raw) On 11/02/2010 01:12 PM, Paul E. McKenney wrote: > OK, once I become more confident that I don't have bugs elsewhere, I will > try out this approach. I can also play with it myself since now your patch is in urcu.git. Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu 2010-11-11 8:44 ` Paolo Bonzini @ 2010-11-12 0:24 ` Paolo Bonzini 2010-11-16 0:16 ` Paul E. McKenney 0 siblings, 1 reply; 6+ messages in thread From: Paolo Bonzini @ 2010-11-12 0:24 UTC (permalink / raw) On 11/11/2010 09:44 AM, Paolo Bonzini wrote: > I can also play with it myself since now your patch is in urcu.git. Ahem, it's not, but that doesn't mean I cannot do the futex stuff. :) Paolo ^ permalink raw reply [flat|nested] 6+ messages in thread
* [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu 2010-11-12 0:24 ` Paolo Bonzini @ 2010-11-16 0:16 ` Paul E. McKenney 0 siblings, 0 replies; 6+ messages in thread From: Paul E. McKenney @ 2010-11-16 0:16 UTC (permalink / raw) On Fri, Nov 12, 2010 at 01:24:03AM +0100, Paolo Bonzini wrote: > On 11/11/2010 09:44 AM, Paolo Bonzini wrote: > >I can also play with it myself since now your patch is in urcu.git. > > Ahem, it's not, but that doesn't mean I cannot do the futex stuff. :) I would very much welcome that, by the way! ;-) Thanx, Paul ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-11-16 0:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20101029162534.GA11742@linux.vnet.ibm.com>
[not found] ` <4CCBDE0B.8060102@redhat.com>
2010-11-01 20:04 ` [ltt-dev] [PATCH RFC] call_rcu() interface for userspace-rcu Paul E. McKenney
2010-11-01 20:25 ` Mathieu Desnoyers
2010-11-02 12:12 ` Paul E. McKenney
2010-11-11 8:44 ` Paolo Bonzini
2010-11-12 0:24 ` Paolo Bonzini
2010-11-16 0:16 ` Paul E. McKenney
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox