[patch] RCU: introduce rcu_soon_pending() interface

[patch] RCU: introduce rcu_soon_pending() interface

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Introduce rcu_soon_pending() interface. This can be used to tell if there
will be a new rcu batch on a cpu soon by looking at the curlist pointer.
This can be used to avoid to enter a tickless idle state where the cpu
would miss that a new batch is ready when rcu_start_batch would be called
on a different cpu.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

See also here: http://www.ussg.iu.edu/hypermail/linux/kernel/0604.3/0057.html
Better solutions welcome :)

 include/linux/rcupdate.h |    1 +
 kernel/rcupdate.c        |    8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 5673008..80bcbce 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -132,6 +132,7 @@ static inline void rcu_bh_qsctr_inc(int 
 }
 
 extern int rcu_pending(int cpu);
+extern int rcu_soon_pending(int cpu);
 
 /**
  * rcu_read_lock - mark the beginning of an RCU read-side critical section.
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 13458bb..e8cf09f 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -485,6 +485,14 @@ int rcu_pending(int cpu)
 		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
 }
 
+int rcu_soon_pending(int cpu)
+{
+	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
+	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
+
+	return (!!rdp->curlist || !!rdp_bh->curlist);
+}
+
 void rcu_check_callbacks(int cpu, int user)
 {
 	if (user || 

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Andrew Morton]

Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
>
> @@ -485,6 +485,14 @@ int rcu_pending(int cpu)
>  		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
>  }
>  
> +int rcu_soon_pending(int cpu)
> +{
> +	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> +	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
> +
> +	return (!!rdp->curlist || !!rdp_bh->curlist);
> +}

This patch sets my nerves a-jangling.

What are the units of soonness?  It's awfully waffly.  Can we specify this
more tightly?

Neither rcu_pending() nor rcu_soon_pending() are commented or documented. 
Pity the poor user trying to work out what they do, and how they differ. 
They're global symbols and they form part of the RCU API - they should be
kernel docified, please.

There's probably a reason why neither of these symbols are exported to
modules.  Once they're actually documented I mught be able to work out what
that reason is ;)

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Heiko Carstens]

> > @@ -485,6 +485,14 @@ int rcu_pending(int cpu)
> >  		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
> >  }
> >  
> > +int rcu_soon_pending(int cpu)
> > +{
> > +	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> > +	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
> > +
> > +	return (!!rdp->curlist || !!rdp_bh->curlist);
> > +}
> 
> This patch sets my nerves a-jangling.
> 
> What are the units of soonness?  It's awfully waffly.  Can we specify this
> more tightly?
> 
> Neither rcu_pending() nor rcu_soon_pending() are commented or documented. 
> Pity the poor user trying to work out what they do, and how they differ. 
> They're global symbols and they form part of the RCU API - they should be
> kernel docified, please.
> 
> There's probably a reason why neither of these symbols are exported to
> modules.  Once they're actually documented I mught be able to work out what
> that reason is ;)

Maybe rcu_batch_pending() would be a better name for rcu_soon_pending(). Also
rcu_batch_in_work() would be a more descriptive name for rcu_pending() as far
as I can tell.
Actually I was hoping for a better solution from the rcu experts, since I
don't like this too, but couldn't find something better.

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Paul E. McKenney]

On Mon, Apr 24, 2006 at 04:09:43PM -0700, Andrew Morton wrote:
> Heiko Carstens <heiko.carstens@de.ibm.com> wrote:
> >
> > @@ -485,6 +485,14 @@ int rcu_pending(int cpu)
> >  		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
> >  }
> >  
> > +int rcu_soon_pending(int cpu)
> > +{
> > +	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> > +	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
> > +
> > +	return (!!rdp->curlist || !!rdp_bh->curlist);
> > +}
> 
> This patch sets my nerves a-jangling.
> 
> What are the units of soonness?  It's awfully waffly.  Can we specify this
> more tightly?
> 
> Neither rcu_pending() nor rcu_soon_pending() are commented or documented. 
> Pity the poor user trying to work out what they do, and how they differ. 
> They're global symbols and they form part of the RCU API - they should be
> kernel docified, please.

Please note that the rcu_pending() interface was never intended for
external use -- it is purely internal to the RCU infrastructure.
If there is a new external use for rcu_pending(), then it would need to
be documented.  But I would rather this one stay internal -- different
RCU implementations might need different things.

So, what are we trying to do here?

> There's probably a reason why neither of these symbols are exported to
> modules.  Once they're actually documented I mught be able to work out what
> that reason is ;)

The reason for rcu_pending() was that it is a private interface.

					Thanx, Paul

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Martin Schwidefsky]

On Tue, 2006-04-25 at 04:23 -0700, Paul E. McKenney wrote:
> > Neither rcu_pending() nor rcu_soon_pending() are commented or documented. 
> > Pity the poor user trying to work out what they do, and how they differ. 
> > They're global symbols and they form part of the RCU API - they should be
> > kernel docified, please.
> 
> Please note that the rcu_pending() interface was never intended for
> external use -- it is purely internal to the RCU infrastructure.
> If there is a new external use for rcu_pending(), then it would need to
> be documented.  But I would rather this one stay internal -- different
> RCU implementations might need different things.
> 
> So, what are we trying to do here?

We are trying to find out if an idle cpu needs to continue to tick every
HZ or if we can put it into deep sleep. If there is any kind of rcu work
pending that requires the idle cpu to call in later via a timer
interrupt, we need to know.

-- 
blue skies,
  Martin.

Martin Schwidefsky
Linux for zSeries Development & Services
IBM Deutschland Entwicklung GmbH

"Reality continues to ruin my life." - Calvin.

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Paul E. McKenney]

On Tue, Apr 25, 2006 at 07:27:21AM +0200, Heiko Carstens wrote:
> > > @@ -485,6 +485,14 @@ int rcu_pending(int cpu)
> > >  		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
> > >  }
> > >  
> > > +int rcu_soon_pending(int cpu)
> > > +{
> > > +	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> > > +	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
> > > +
> > > +	return (!!rdp->curlist || !!rdp_bh->curlist);
> > > +}
> > 
> > This patch sets my nerves a-jangling.
> > 
> > What are the units of soonness?  It's awfully waffly.  Can we specify this
> > more tightly?
> > 
> > Neither rcu_pending() nor rcu_soon_pending() are commented or documented. 
> > Pity the poor user trying to work out what they do, and how they differ. 
> > They're global symbols and they form part of the RCU API - they should be
> > kernel docified, please.
> > 
> > There's probably a reason why neither of these symbols are exported to
> > modules.  Once they're actually documented I mught be able to work out what
> > that reason is ;)
> 
> Maybe rcu_batch_pending() would be a better name for rcu_soon_pending(). Also
> rcu_batch_in_work() would be a more descriptive name for rcu_pending() as far
> as I can tell.
> Actually I was hoping for a better solution from the rcu experts, since I
> don't like this too, but couldn't find something better.

OK, got a look at your patch.

You are using this internally, as part of the RCU -implementation-.
You are determining whether this CPU will still be needed by RCU,
or whether it can be turned off.  So how 'bout calling the (internal)
API something like rcu_needs_cpu()?

int rcu_needs_cpu(int cpu)
{
	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);

	return (!!rdp->curlist || !!rdp_bh->curlist || rcu_pending(cpu));
}

Then you can drop the rcu_pending() check from your 390 patch.

Seem reasonable?

The meaning of rcu_pending() is "Does RCU have some work pending on
this CPU, so that there is a need to invoke rcu_check_callbacks() on
this CPU?"

							Thanx, Paul

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Paul E. McKenney]

On Tue, Apr 25, 2006 at 01:33:37PM +0200, Martin Schwidefsky wrote:
> On Tue, 2006-04-25 at 04:23 -0700, Paul E. McKenney wrote:
> > > Neither rcu_pending() nor rcu_soon_pending() are commented or documented. 
> > > Pity the poor user trying to work out what they do, and how they differ. 
> > > They're global symbols and they form part of the RCU API - they should be
> > > kernel docified, please.
> > 
> > Please note that the rcu_pending() interface was never intended for
> > external use -- it is purely internal to the RCU infrastructure.
> > If there is a new external use for rcu_pending(), then it would need to
> > be documented.  But I would rather this one stay internal -- different
> > RCU implementations might need different things.
> > 
> > So, what are we trying to do here?
> 
> We are trying to find out if an idle cpu needs to continue to tick every
> HZ or if we can put it into deep sleep. If there is any kind of rcu work
> pending that requires the idle cpu to call in later via a timer
> interrupt, we need to know.

OK, this confirms my guess that you are acting as part of the RCU
implementation rather than as a normal user of the RCU API.

						Thanx, Paul

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Heiko Carstens]

> OK, got a look at your patch.
> 
> You are using this internally, as part of the RCU -implementation-.
> You are determining whether this CPU will still be needed by RCU,
> or whether it can be turned off.  So how 'bout calling the (internal)
> API something like rcu_needs_cpu()?
> 
> int rcu_needs_cpu(int cpu)
> {
> 	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> 	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
> 
> 	return (!!rdp->curlist || !!rdp_bh->curlist || rcu_pending(cpu));
> }
> 
> Then you can drop the rcu_pending() check from your 390 patch.
> 
> Seem reasonable?

Looks fine to me! Will you post a patch or should I?

Re: [patch] RCU: introduce rcu_soon_pending() interface

[Paul E. McKenney]

On Tue, Apr 25, 2006 at 01:52:26PM +0200, Heiko Carstens wrote:
> > OK, got a look at your patch.
> > 
> > You are using this internally, as part of the RCU -implementation-.
> > You are determining whether this CPU will still be needed by RCU,
> > or whether it can be turned off.  So how 'bout calling the (internal)
> > API something like rcu_needs_cpu()?
> > 
> > int rcu_needs_cpu(int cpu)
> > {
> > 	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
> > 	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
> > 
> > 	return (!!rdp->curlist || !!rdp_bh->curlist || rcu_pending(cpu));
> > }
> > 
> > Then you can drop the rcu_pending() check from your 390 patch.
> > 
> > Seem reasonable?
> 
> Looks fine to me! Will you post a patch or should I?

Given that I am getting 24Kbps right now, could you please post it?

							Thanx, Paul

[patch] RCU: introduce rcu_needs_cpu() interface

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
From: "Paul E. McKenney" <paulmck@us.ibm.com>

Introduce rcu_needs_cpu() interface. This can be used to tell if there
will be a new rcu batch on a cpu soon by looking at the curlist pointer.
This can be used to avoid to enter a tickless idle state where the cpu
would miss that a new batch is ready when rcu_start_batch would be called
on a different cpu.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

This one replaces rcu-introduce-rcu_soon_pending-interface.patch .

 include/linux/rcupdate.h |    1 +
 kernel/rcupdate.c        |    8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 5673008..970284f 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -132,6 +132,7 @@ static inline void rcu_bh_qsctr_inc(int 
 }
 
 extern int rcu_pending(int cpu);
+extern int rcu_needs_cpu(int cpu);
 
 /**
  * rcu_read_lock - mark the beginning of an RCU read-side critical section.
diff --git a/kernel/rcupdate.c b/kernel/rcupdate.c
index 13458bb..651ac14 100644
--- a/kernel/rcupdate.c
+++ b/kernel/rcupdate.c
@@ -485,6 +485,14 @@ int rcu_pending(int cpu)
 		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
 }
 
+int rcu_needs_cpu(int cpu)
+{
+	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);
+	struct rcu_data *rdp_bh = &per_cpu(rcu_bh_data, cpu);
+
+	return (!!rdp->curlist || !!rdp_bh->curlist || rcu_pending(cpu));
+}
+
 void rcu_check_callbacks(int cpu, int user)
 {
 	if (user || 

[patch] s390: exploit rcu_needs_cpu() interface

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Exploit rcu_needs_cpu() interface to keep the cpu 'ticking' if necessary.

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

This one replaces s390-exploit-rcu_soon_pending-interface.patch .

 arch/s390/kernel/time.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index fea043b..029f099 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -249,18 +249,19 @@ static inline void stop_hz_timer(void)
 	unsigned long flags;
 	unsigned long seq, next;
 	__u64 timer, todval;
+	int cpu = smp_processor_id();
 
 	if (sysctl_hz_timer != 0)
 		return;
 
-	cpu_set(smp_processor_id(), nohz_cpu_mask);
+	cpu_set(cpu, nohz_cpu_mask);
 
 	/*
 	 * Leave the clock comparator set up for the next timer
 	 * tick if either rcu or a softirq is pending.
 	 */
-	if (rcu_pending(smp_processor_id()) || local_softirq_pending()) {
-		cpu_clear(smp_processor_id(), nohz_cpu_mask);
+	if (rcu_needs_cpu(cpu) || local_softirq_pending()) {
+		cpu_clear(cpu, nohz_cpu_mask);
 		return;
 	}
 

[patch] RCU: add comments to rcu_pending/rcu_needs_cpu

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

Add some comments to rcu_pending() and rcu_needs_cpu().

Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

Wording might be poor, but probably better than no comments at all.

 kernel/rcupdate.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff -purN a/kernel/rcupdate.c b/kernel/rcupdate.c
--- a/kernel/rcupdate.c	2006-04-27 09:30:23.000000000 +0200
+++ b/kernel/rcupdate.c	2006-04-27 09:56:51.000000000 +0200
@@ -479,12 +479,30 @@ static int __rcu_pending(struct rcu_ctrl
 	return 0;
 }
 
+/**
+ * rcu_pending - Check for pending RCU work on cpu.
+ * @cpu: cpu to check.
+ *
+ * Does RCU have some work pending on the specified cpu, so that there is a
+ * need to invoke rcu_check_callbacks() on the cpu?
+ */
 int rcu_pending(int cpu)
 {
 	return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) ||
 		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
 }
 
+/**
+ * rcu_needs_cpu - Determine if cpu will still be needed by RCU.
+ * @cpu: cpu to check.
+ *
+ * Determine whether the specified cpu will still be needed by RCU, or whether
+ * it can be turned off (e.g. by entering a tickless idle state).
+ * Note the difference to rcu_pending() which checks if there is some work to
+ * do that can be done immediately. While this function in addition checks if
+ * there would be some work to do if e.g. a different cpu finished working on
+ * the current batch.
+ */
 int rcu_needs_cpu(int cpu)
 {
 	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);

Re: [patch] RCU: add comments to rcu_pending/rcu_needs_cpu

[Paul E. McKenney]

On Thu, Apr 27, 2006 at 10:11:56AM +0200, Heiko Carstens wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> Add some comments to rcu_pending() and rcu_needs_cpu().
> 
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
> 
> Wording might be poor, but probably better than no comments at all.

But these are internal interfaces for RCU, so doesn't seem like they
should go into docbook.  Quite different than (say) rcu_read_lock()
or call_rcu().

How about something like the following instead?

						Thanx, Paul

Signed-off-by: <paulmck@us.ibm.com>
---

diff -urpNa -X dontdiff linux-2.6.17-rc3-rnc/kernel/rcupdate.c linux-2.6.17-rc3-rnc.pem/kernel/rcupdate.c
--- linux-2.6.17-rc3-rnc/kernel/rcupdate.c	2006-05-01 14:42:30.000000000 -0700
+++ linux-2.6.17-rc3-rnc.pem/kernel/rcupdate.c	2006-05-01 14:48:19.000000000 -0700
@@ -479,12 +479,25 @@ static int __rcu_pending(struct rcu_ctrl
 	return 0;
 }
 
+/*
+ * Check to see if there is any immediate RCU-related work to be done
+ * by the current CPU, returning 1 if so.  This function is part of the
+ * RCU implementation; it is -not- an exported member of the RCU API.
+ */
+
 int rcu_pending(int cpu)
 {
 	return __rcu_pending(&rcu_ctrlblk, &per_cpu(rcu_data, cpu)) ||
 		__rcu_pending(&rcu_bh_ctrlblk, &per_cpu(rcu_bh_data, cpu));
 }
 
+/* 
+ * Check to see if any future RCU-related work will need to be done
+ * by the current CPU, even if none need be done immediately, returning
+ * 1 if so.  This function is part of the RCU implementation; it is -not-
+ * an exported member of the RCU API.
+ */
+
 int rcu_needs_cpu(int cpu)
 {
 	struct rcu_data *rdp = &per_cpu(rcu_data, cpu);

Re: [patch] RCU: add comments to rcu_pending/rcu_needs_cpu

[Heiko Carstens]

> > Add some comments to rcu_pending() and rcu_needs_cpu().
> 
> But these are internal interfaces for RCU, so doesn't seem like they
> should go into docbook.  Quite different than (say) rcu_read_lock()
> or call_rcu().
> 
> How about something like the following instead?

Fine with me :)