[PATCH 5/5][RFC] rcu: start new grace period from rcu_pending()

[PATCH 5/5][RFC] rcu: start new grace period from rcu_pending()

[Oleg Nesterov]

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 <oleg@tv-sign.ru>

--- 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;
 }

Re: [PATCH 5/5][RFC] rcu: start new grace period from rcu_pending()

[Oleg Nesterov]

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 <oleg@tv-sign.ru>

--- 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));
 }

Re: [PATCH 5/5][RFC] rcu: start new grace period from rcu_pending()

[Paul E. McKenney]

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 <oleg@tv-sign.ru>
> 
> --- 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));
>  }
> 

Re: [PATCH 5/5][RFC] rcu: start new grace period from rcu_pending()

[Oleg Nesterov]

"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.