Resend, see the previous dicsussion:
http://marc.theaimsgroup.com/?t=110442762800019
Manfred Spraul has another (better) idea to speedup qs detection, but
that would be a more radical change.
Let's suppose that cpu is running idle thread or user level process, and
the grace period was started. Currently we need 2 local timer interrupts
to happen before this cpu can signal the end of it's grace period. This is
because rcu_check_quiescent_state() will reset ->passed_quiesc before it
sets ->qs_pending = 1.
I think it is better to set ->qs_pending = 1 directly in __rcu_pending():
int __rcu_pending()
{
if (qs_pending) return 1;
if (rdp->quiescbatch != rcp->cur) {
passed_quiesc = 0;
barrier();
qs_pending = 1;
return 1;
}
... other checks ...
}
void rcu_check_quiescent_state()
{
if (!qs_pending) return;
barrier();
if (!passed_quiesc) return;
cpu_quiet();
qs_pending = 0;
}
This way the grace period for that cpu will be completed after the 1st irq.
Signed-off-by: Oleg Nesterov <[email protected]>
--- 2.6.15/kernel/rcupdate.c~5_SPEEDUP 2006-01-08 23:49:31.000000000 +0300
+++ 2.6.15/kernel/rcupdate.c 2006-01-09 00:26:44.000000000 +0300
@@ -292,28 +292,25 @@ static void cpu_quiet(int cpu, struct rc
static void rcu_check_quiescent_state(struct rcu_ctrlblk *rcp,
struct rcu_data *rdp)
{
- if (rdp->quiescbatch != rcp->cur) {
- /* start new grace period: */
- rdp->qs_pending = 1;
- rdp->passed_quiesc = 0;
- rdp->quiescbatch = rcp->cur;
- return;
- }
-
/* Grace period already completed for this cpu?
* qs_pending is checked instead of the actual bitmap to avoid
* cacheline trashing.
*/
if (!rdp->qs_pending)
return;
+ /*
+ * Protect against the race with __rcu_pending() from local interrupt.
+ * We should read ->passed_quiesc after we checked ->qs_pending != 0.
+ * These vars are cpu-local, no need to use memory barriers.
+ */
+ barrier();
- /*
+ /*
* Was there a quiescent state since the beginning of the grace
* period? If no, then exit and wait for the next call.
*/
if (!rdp->passed_quiesc)
return;
- rdp->qs_pending = 0;
spin_lock(&rcp->lock);
/*
@@ -324,6 +321,8 @@ static void rcu_check_quiescent_state(st
cpu_quiet(rdp->cpu, rcp);
spin_unlock(&rcp->lock);
+
+ rdp->qs_pending = 0;
}
@@ -435,6 +434,20 @@ static void rcu_process_callbacks(unsign
static int __rcu_pending(struct rcu_ctrlblk *rcp, struct rcu_data *rdp)
{
+ /* The rcu core waits for a quiescent state from the cpu */
+ if (rdp->qs_pending)
+ return 1;
+
+ if (rdp->quiescbatch != rcp->cur) {
+ /* start new grace period: */
+ rdp->quiescbatch = rcp->cur;
+ rdp->passed_quiesc = 0;
+ /* see the comment in rcu_check_quiescent_state() */
+ barrier();
+ rdp->qs_pending = 1;
+ return 1;
+ }
+
/* This cpu has pending rcu entries and the grace period
* for them has completed.
*/
@@ -445,10 +458,6 @@ static int __rcu_pending(struct rcu_ctrl
if (!rdp->curlist && rdp->nxtlist)
return 1;
- /* The rcu core waits for a quiescent state from the cpu */
- if (rdp->quiescbatch != rcp->cur || rdp->qs_pending)
- return 1;
-
/* nothing to do */
return 0;
}
Oleg Nesterov wrote:
>
> I think it is better to set ->qs_pending = 1 directly in __rcu_pending():
This patch has a bug. I am sending a trivial fix, but now I am not
sure myself that 1 timer tick saved worth the code uglification.
[PATCH 6/5] rcu: start new grace period from rcu_pending() fix
We should not miss __rcu_pending(&rcu_bh_ctrlblk) in rcu_pending().
Signed-off-by: Oleg Nesterov <[email protected]>
--- 2.6.15/kernel/rcupdate.c~6_FIX 2006-01-09 00:26:44.000000000 +0300
+++ 2.6.15/kernel/rcupdate.c 2006-01-09 19:19:27.000000000 +0300
@@ -464,7 +464,7 @@ static int __rcu_pending(struct rcu_ctrl
int rcu_pending(int cpu)
{
- return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) ||
+ return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) |
__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
}
On Mon, Jan 09, 2006 at 05:32:55PM +0300, Oleg Nesterov wrote:
> Oleg Nesterov wrote:
> >
> > I think it is better to set ->qs_pending = 1 directly in __rcu_pending():
>
> This patch has a bug. I am sending a trivial fix, but now I am not
> sure myself that 1 timer tick saved worth the code uglification.
This is indeed an accident waiting to happen -- someone is bound to
replace the "|" with an "||", a change that is too easy for someone
to miss. Once Vatsa is satisfied with the CPU-hotplug aspects of
this set of patches, if __rcu_pending() still has side-effects, I would
suggest something like the following:
int rcu_pending(int cpu)
{
int retval = 0;
if (__rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)))
retval = 1;
if (__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu)))
retval = 1;
return retval;
}
A few more lines, but the intent is much more clear. And I bet that
gcc generates reasonable code in either case.
Or maybe this is just me...
Thanx, Paul
> [PATCH 6/5] rcu: start new grace period from rcu_pending() fix
>
> We should not miss __rcu_pending(&rcu_bh_ctrlblk) in rcu_pending().
>
> Signed-off-by: Oleg Nesterov <[email protected]>
>
> --- 2.6.15/kernel/rcupdate.c~6_FIX 2006-01-09 00:26:44.000000000 +0300
> +++ 2.6.15/kernel/rcupdate.c 2006-01-09 19:19:27.000000000 +0300
> @@ -464,7 +464,7 @@ static int __rcu_pending(struct rcu_ctrl
>
> int rcu_pending(int cpu)
> {
> - return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) ||
> + return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) |
> __rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
> }
>
"Paul E. McKenney" wrote:
>
> On Mon, Jan 09, 2006 at 05:32:55PM +0300, Oleg Nesterov wrote:
> > Oleg Nesterov wrote:
> > >
> > > I think it is better to set ->qs_pending = 1 directly in __rcu_pending():
> >
> > This patch has a bug. I am sending a trivial fix, but now I am not
> > sure myself that 1 timer tick saved worth the code uglification.
>
> This is indeed an accident waiting to happen -- someone is bound to
> replace the "|" with an "||", a change that is too easy for someone
> to miss. Once Vatsa is satisfied with the CPU-hotplug aspects of
> this set of patches, if __rcu_pending() still has side-effects, I would
> suggest something like the following:
>
> int rcu_pending(int cpu)
> {
> int retval = 0;
>
> if (__rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)))
> retval = 1;
> if (__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu)))
> retval = 1;
> return retval;
> }
>
> A few more lines, but the intent is much more clear. And I bet that
> gcc generates reasonable code in either case.
>
> Or maybe this is just me...
No, me too. For some reasons I can't re-send the patch today, will do
tomorrow.
However, I am not sure anymore that this patch is a good idea. Exactly
because it adds side-effects to rcu_pending().
So, unless somebody on CC: list thinks it may be useful - let's forget
it.
Oleg.