[PATCH] rcu: eliminate rcu_data.last_qsctr

[PATCH] rcu: eliminate rcu_data.last_qsctr

[Oleg Nesterov]

last_qsctr is used in rcu_check_quiescent_state() exclusively.
We can reset qsctr at the start of the grace period, and then
just test qsctr against 0.

On top of the 'rcu: eliminate rcu_ctrlblk.lock', see
http://marc.theaimsgroup.com/?l=linux-kernel&m=110156786721526

Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>

--- 2.6.10-rc2/include/linux/rcupdate.h~	2004-11-27 21:32:49.000000000 +0300
+++ 2.6.10-rc2/include/linux/rcupdate.h	2004-11-28 23:15:20.510856936 +0300
@@ -88,8 +88,6 @@ struct rcu_data {
 	/* 1) quiescent state handling : */
 	long		quiescbatch;     /* Batch # for grace period */
 	long		qsctr;		 /* User-mode/idle loop etc. */
-	long            last_qsctr;	 /* value of qsctr at beginning */
-					 /* of rcu grace period */
 	int		qs_pending;	 /* core waits for quiesc state */
 
 	/* 2) batch handling */
--- 2.6.10-rc2/kernel/rcupdate.c~	2004-11-28 23:13:03.324712400 +0300
+++ 2.6.10-rc2/kernel/rcupdate.c	2004-11-28 23:15:20.511856784 +0300
@@ -215,9 +215,9 @@ static void rcu_check_quiescent_state(st
 			struct rcu_state *rsp, struct rcu_data *rdp)
 {
 	if (rdp->quiescbatch != rcp->cur) {
-		/* new grace period: record qsctr value. */
+		/* new grace period: reset qsctr value. */
 		rdp->qs_pending = 1;
-		rdp->last_qsctr = rdp->qsctr;
+		rdp->qsctr = 0;
 		rdp->quiescbatch = rcp->cur;
 		return;
 	}
@@ -229,12 +229,7 @@ static void rcu_check_quiescent_state(st
 	if (!rdp->qs_pending)
 		return;
 
-	/* 
-	 * Races with local timer interrupt - in the worst case
-	 * we may miss one quiescent state of that CPU. That is
-	 * tolerable. So no need to disable interrupts.
-	 */
-	if (rdp->qsctr == rdp->last_qsctr)
+	if (rdp->qsctr == 0)
 		return;
 	rdp->qs_pending = 0;

Re: [PATCH] rcu: eliminate rcu_data.last_qsctr

[Manfred Spraul]

Oleg Nesterov wrote:

>last_qsctr is used in rcu_check_quiescent_state() exclusively.
>We can reset qsctr at the start of the grace period, and then
>just test qsctr against 0.
>
>On top of the 'rcu: eliminate rcu_ctrlblk.lock', see
>http://marc.theaimsgroup.com/?l=linux-kernel&m=110156786721526
>
>Signed-off-by: Oleg Nesterov <oleg@tv-sign.ru>
>
>  
>
Compile and boot tested with 2.6.10-rc2 on a 2-cpu system.

Signed-Off-By: Manfred Spraul <manfred@colorfullife.com>

Re: [PATCH] rcu: eliminate rcu_data.last_qsctr

[Manfred Spraul]

Oleg Nesterov wrote:

>last_qsctr is used in rcu_check_quiescent_state() exclusively.
>We can reset qsctr at the start of the grace period, and then
>just test qsctr against 0.
>
>  
>
It seems the patch got lost, I've updated it a bit and resent it to Andrew.

But: I think there is the potential for an even larger cleanup, although 
this would be more a rewrite:
Get rid of rcu_check_quiescent_state and instead use something like this 
in rcu_qsctr_inc:

static inline void rcu_qsctr_inc(int cpu)
{
        struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
        if (rdp->quiescbatch != rcp->cur) {
             /* a new grace period is running. And we are at a quiescent
              * point, so complete it
              */
             spin_lock(&rsp->lock);
             rdp->quiescbatch = rcp->cur;
             cpu_quiet(rdp->cpu, rcp, rsp);
            spin_unlock(&rsp->lock);
     }
}

It's just an idea, it needs testing on big systems - does reading from 
the global rcp from every schedule call cause any problems? The cache 
line is virtually read-only, so it shouldn't cause trashing, but who knows?

--
    Manfred

Re: [PATCH] rcu: eliminate rcu_data.last_qsctr

[Paul E. McKenney]

On Wed, Dec 29, 2004 at 04:37:31PM +0100, Manfred Spraul wrote:
> Oleg Nesterov wrote:
> 
> >last_qsctr is used in rcu_check_quiescent_state() exclusively.
> >We can reset qsctr at the start of the grace period, and then
> >just test qsctr against 0.
> >
> > 
> >
> It seems the patch got lost, I've updated it a bit and resent it to Andrew.
> 
> But: I think there is the potential for an even larger cleanup, although 
> this would be more a rewrite:
> Get rid of rcu_check_quiescent_state and instead use something like this 
> in rcu_qsctr_inc:
> 
> static inline void rcu_qsctr_inc(int cpu)
> {
>        struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
>        if (rdp->quiescbatch != rcp->cur) {
>             /* a new grace period is running. And we are at a quiescent
>              * point, so complete it
>              */
>             spin_lock(&rsp->lock);
>             rdp->quiescbatch = rcp->cur;
>             cpu_quiet(rdp->cpu, rcp, rsp);
>            spin_unlock(&rsp->lock);
>     }
> }
> 
> It's just an idea, it needs testing on big systems - does reading from 
> the global rcp from every schedule call cause any problems? The cache 
> line is virtually read-only, so it shouldn't cause trashing, but who knows?

Hello, Manfred,

The main concern I have with this is not cache thrashing of rcp->cur,
but shrinking the grace periods on large systems, which can result in
extra overhead per callback, since the shorter grace periods will tend
to have fewer callbacks.  We saw this problem on some of the early
RCU-infrastructure patches.

Another approach would be to conditionally compile the two versions,
though that might make the code more complex.

						Thanx, Paul