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 <[email protected]>
---
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
