Messages in this thread Patch in this message | | | Date | Mon, 10 Jul 2006 09:51:18 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH 1/2] srcu-3: RCU variant permitting read-side blocking |
| |
On Mon, Jul 10, 2006 at 03:50:29AM +0400, Oleg Nesterov wrote: > On 07/06, Paul E. McKenney wrote: > > > > Updated patch adding a variant of RCU that permits sleeping in read-side > > critical sections. > > I do not see any problems with this patch, but I have a couple of > questions, so your help is needed again.
Thank you for looking it over!
> > +void synchronize_srcu(struct srcu_struct *sp) > > +{ > > + [... snip ...] > > + > > + synchronize_sched(); /* Force memory barrier on all CPUs. */ > > + > > + /* > > + * The preceding synchronize_sched() forces all srcu_read_unlock() > > + * primitives that were executing concurrently with the preceding > > + * for_each_possible_cpu() loop to have completed by this point. > > + * More importantly, it also forces the corresponding SRCU read-side > > + * critical sections to have also completed, and the corresponding > > + * references to SRCU-protected data items to be dropped. > > + */ > > + > > + mutex_unlock(&sp->mutex); > > +} > > Isn't it possible to unlock ->mutex earlier, before the last > synchronize_sched()?
It seems possible, but I would like to think carefully about this one first, and, if it still seems plausible, test it heavily. If I understand your line of reasoning, the thought is that the first synchronize_sched() at the beginning of synchronize_srcu() ensures that all of the counter updates pertaining to the last instance of synchronize_srcu() have been committed. The same reasoning might well cover the sp->completed fastpath as well.
In any case, this is a performance boost off the fastpath. A good boost, if it works, but I will be much more excited if you find a way of speeding up srcu_read_lock() or srcu_read_unlock(). ;-)
> Another question: what is the semantics of synchronize_sched() ? > > I am not talking about the current implementation, it is very clear. > The question is: what is the _definition_ of synchronize_sched() > (which must be valid for "any" RCU implementation) ? > > 1) The comment in include/linux/rcupdate.h states that "all preempt_disable > code sequences will have completed before this primitive returns". > > 2) kernel/srcu.c claims that this primitive "forces memory barrier on all > CPUs". (so the comment in rcupdate.h is not complete). > > (I understand this so that each cpu does something which implies mb() > semantics). > > As I see it, 1) + 2) is NOT enough for synchronize_srcu() to be correct > (the 2-nd and 3-rd synchronize_sched() calls). I think synchronize_sched() > should also guarantee the completion of mem ops on all CPUs before return, > not just mb() (which does not have any timing guaranties). > > Could you clarify this issue? > > (Again, I do not see any problems with the current RCU implementation).
However, this -does- seem to be to be a problem with the comment headers and the documentation. Does the following patch make things better?
David, would it be worthwhile adding this global-memory-barrier effect of synchronize_rcu(), synchronize_sched(), and synchronize_srcu() to Documentation/memory-barriers.txt?
Thanx, Paul
Signed-off-by: Paul E. McKenney <paulmck@us.ibm.com> ---
Documentation/RCU/checklist.txt | 4 ++++ include/linux/rcupdate.h | 3 +++ kernel/rcupdate.c | 3 +++ kernel/srcu.c | 3 ++- 4 files changed, 12 insertions(+), 1 deletion(-)
diff -urpNa -X dontdiff linux-2.6.17-srcu-LKML-4/Documentation/RCU/checklist.txt linux-2.6.17-srcu-LKML-5/Documentation/RCU/checklist.txt --- linux-2.6.17-srcu-LKML-4/Documentation/RCU/checklist.txt 2006-07-06 16:45:01.000000000 -0700 +++ linux-2.6.17-srcu-LKML-5/Documentation/RCU/checklist.txt 2006-07-10 09:43:19.000000000 -0700 @@ -221,3 +221,7 @@ over a rather long period of time, but i Note that, rcu_assign_pointer() and rcu_dereference() relate to SRCU just as they do to other forms of RCU. + +14. The synchronize_rcu(), synchronize_sched(), and synchronize_srcu() + primitives force at least one memory barrier to be executed on + each active CPU before they return. diff -urpNa -X dontdiff linux-2.6.17-srcu-LKML-4/include/linux/rcupdate.h linux-2.6.17-srcu-LKML-5/include/linux/rcupdate.h --- linux-2.6.17-srcu-LKML-4/include/linux/rcupdate.h 2006-06-17 18:49:35.000000000 -0700 +++ linux-2.6.17-srcu-LKML-5/include/linux/rcupdate.h 2006-07-10 09:48:51.000000000 -0700 @@ -251,6 +251,9 @@ extern int rcu_needs_cpu(int cpu); * guarantees that rcu_read_lock() sections will have completed. * In "classic RCU", these two guarantees happen to be one and * the same, but can differ in realtime RCU implementations. + * + * In addition, this primitive guarantees that every active CPU has + * executed at least one memory barrier before it returns. */ #define synchronize_sched() synchronize_rcu() diff -urpNa -X dontdiff linux-2.6.17-srcu-LKML-4/kernel/rcupdate.c linux-2.6.17-srcu-LKML-5/kernel/rcupdate.c --- linux-2.6.17-srcu-LKML-4/kernel/rcupdate.c 2006-06-17 18:49:35.000000000 -0700 +++ linux-2.6.17-srcu-LKML-5/kernel/rcupdate.c 2006-07-10 09:48:32.000000000 -0700 @@ -597,6 +597,9 @@ static void wakeme_after_rcu(struct rcu_ * sections are delimited by rcu_read_lock() and rcu_read_unlock(), * and may be nested. * + * This primitive also causes each active CPU to execute at least one + * memory barrier before it returns. + * * If your read-side code is not protected by rcu_read_lock(), do -not- * use synchronize_rcu(). */ diff -urpNa -X dontdiff linux-2.6.17-srcu-LKML-4/kernel/srcu.c linux-2.6.17-srcu-LKML-5/kernel/srcu.c --- linux-2.6.17-srcu-LKML-4/kernel/srcu.c 2006-07-06 16:50:23.000000000 -0700 +++ linux-2.6.17-srcu-LKML-5/kernel/srcu.c 2006-07-10 09:48:09.000000000 -0700 @@ -143,7 +143,8 @@ void srcu_read_unlock(struct srcu_struct * Flip the completed counter, and wait for the old count to drain to zero. * As with classic RCU, the updater must use some separate means of * synchronizing concurrent updates. Can block; must be called from - * process context. + * process context. Has the side-effect of forcing a memory barrier on + * each active CPU before returning. * * Note that it is illegal to call synchornize_srcu() from the corresponding * SRCU read-side critical section; doing so will result in deadlock. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |