As there was no additional feedback on the most recent version of this
patch, I'm resubmitting it for inclusion. As far as I know there are no
more outstanding concerns.
--
sched: sched_clock() clocksource handling.
There are presently a number of issues and limitations with how the
clocksource and sched_clock() interaction works today. Configurations
tend to be grouped in to one of the following:
- Platform provides a clocksource unsuitable for sched_clock()
and prefers to use the generic jiffies-backed implementation.
- Platform provides its own clocksource and sched_clock() that
wraps in to it.
- Platform uses a generic clocksource (ie, drivers/clocksource/)
combined with the generic jiffies-backed sched_clock().
- Platform supports multiple sched_clock()-capable clocksources.
This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address
these issues, which can be set for any sched_clock()-capable clocksource.
The generic sched_clock() implementation is likewise switched over to
always read from a designated sched_clocksource, which is default
initialized to the jiffies clocksource and updated based on the
availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses
the generic cyc2ns() logic on the clocksource ->read(), most of the
platform-specific sched_clock() implementations can subsequently be
killed off.
Signed-off-by: Paul Mundt <[email protected]>
---
include/linux/clocksource.h | 4 +++-
kernel/sched_clock.c | 13 +++++++++++--
kernel/time/clocksource.c | 19 +++++++++++++++++++
kernel/time/jiffies.c | 2 +-
4 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c56457c..2109940 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -202,7 +202,8 @@ struct clocksource {
#endif
};
-extern struct clocksource *clock; /* current clocksource */
+extern struct clocksource *clock; /* current clocksource */
+extern struct clocksource *sched_clocksource; /* sched_clock() clocksource */
/*
* Clock source flags bits::
@@ -212,6 +213,7 @@ extern struct clocksource *clock; /* current clocksource */
#define CLOCK_SOURCE_WATCHDOG 0x10
#define CLOCK_SOURCE_VALID_FOR_HRES 0x20
+#define CLOCK_SOURCE_USE_FOR_SCHED_CLOCK 0x40
/* simplify initialization of mask field */
#define CLOCKSOURCE_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)
diff --git a/kernel/sched_clock.c b/kernel/sched_clock.c
index e1d16c9..b51d48d 100644
--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -30,6 +30,8 @@
#include <linux/percpu.h>
#include <linux/ktime.h>
#include <linux/sched.h>
+#include <linux/clocksource.h>
+#include <linux/rcupdate.h>
/*
* Scheduler clock - returns current time in nanosec units.
@@ -38,8 +40,15 @@
*/
unsigned long long __attribute__((weak)) sched_clock(void)
{
- return (unsigned long long)(jiffies - INITIAL_JIFFIES)
- * (NSEC_PER_SEC / HZ);
+ unsigned long long time;
+ struct clocksource *clock;
+
+ rcu_read_lock();
+ clock = rcu_dereference(sched_clocksource);
+ time = cyc2ns(clock, clocksource_read(clock));
+ rcu_read_unlock();
+
+ return time;
}
static __read_mostly int sched_clock_running;
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 80189f6..3795954 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -25,6 +25,7 @@
*/
#include <linux/clocksource.h>
+#include <linux/rcupdate.h>
#include <linux/sysdev.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -109,6 +110,7 @@ EXPORT_SYMBOL(timecounter_cyc2time);
/* XXX - Would like a better way for initializing curr_clocksource */
extern struct clocksource clocksource_jiffies;
+struct clocksource *sched_clocksource = &clocksource_jiffies;
/*[Clocksource internal variables]---------
* curr_clocksource:
@@ -362,6 +364,9 @@ static struct clocksource *select_clocksource(void)
if (next == curr_clocksource)
return NULL;
+ if (next->flags & CLOCK_SOURCE_USE_FOR_SCHED_CLOCK)
+ sched_clocksource = next;
+
return next;
}
@@ -440,7 +445,21 @@ void clocksource_unregister(struct clocksource *cs)
list_del(&cs->list);
if (clocksource_override == cs)
clocksource_override = NULL;
+
next_clocksource = select_clocksource();
+
+ /*
+ * If select_clocksource() fails to find another suitable
+ * clocksource for sched_clocksource and we are unregistering
+ * it, switch back to jiffies.
+ */
+ if (sched_clocksource == cs) {
+ rcu_assign_pointer(sched_clocksource, &clocksource_jiffies);
+ spin_unlock_irqrestore(&clocksource_lock, flags);
+ synchronize_rcu();
+ return;
+ }
+
spin_unlock_irqrestore(&clocksource_lock, flags);
}
diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
index c3f6c30..727d881 100644
--- a/kernel/time/jiffies.c
+++ b/kernel/time/jiffies.c
@@ -52,7 +52,7 @@
static cycle_t jiffies_read(struct clocksource *cs)
{
- return (cycle_t) jiffies;
+ return (cycle_t) (jiffies - INITIAL_JIFFIES);
}
struct clocksource clocksource_jiffies = {
On Tue, 2009-06-02 at 16:17 +0900, Paul Mundt wrote:
> @@ -362,6 +364,9 @@ static struct clocksource *select_clocksource(void)
> if (next == curr_clocksource)
> return NULL;
>
> + if (next->flags & CLOCK_SOURCE_USE_FOR_SCHED_CLOCK)
> + sched_clocksource = next;
> +
> return next;
> }
>
> @@ -440,7 +445,21 @@ void clocksource_unregister(struct clocksource *cs)
> list_del(&cs->list);
> if (clocksource_override == cs)
> clocksource_override = NULL;
> +
> next_clocksource = select_clocksource();
> +
> + /*
> + * If select_clocksource() fails to find another suitable
> + * clocksource for sched_clocksource and we are unregistering
> + * it, switch back to jiffies.
> + */
> + if (sched_clocksource == cs) {
> + rcu_assign_pointer(sched_clocksource, &clocksource_jiffies);
> + spin_unlock_irqrestore(&clocksource_lock, flags);
> + synchronize_rcu();
> + return;
> + }
> +
> spin_unlock_irqrestore(&clocksource_lock, flags);
> }
What if there's multiple CLOCK_SOURCE_USER_FOR_SCHED_CLOCK [ damn, thats
a long name to type :-) ] ?
That is, should we have logic in select_clocksource that does:
if ((next->flags & ..) && next->prio > sched_clocksource->prio)
or whatever, so that it picks the best one?
Same for unregister, should we re-evaluate all clocksources before
falling back to basic?
On Tue, Jun 02, 2009 at 09:25:02AM +0200, Peter Zijlstra wrote:
> On Tue, 2009-06-02 at 16:17 +0900, Paul Mundt wrote:
>
> > @@ -362,6 +364,9 @@ static struct clocksource *select_clocksource(void)
> > if (next == curr_clocksource)
> > return NULL;
> >
> > + if (next->flags & CLOCK_SOURCE_USE_FOR_SCHED_CLOCK)
> > + sched_clocksource = next;
> > +
> > return next;
> > }
> >
> > @@ -440,7 +445,21 @@ void clocksource_unregister(struct clocksource *cs)
> > list_del(&cs->list);
> > if (clocksource_override == cs)
> > clocksource_override = NULL;
> > +
> > next_clocksource = select_clocksource();
> > +
> > + /*
> > + * If select_clocksource() fails to find another suitable
> > + * clocksource for sched_clocksource and we are unregistering
> > + * it, switch back to jiffies.
> > + */
> > + if (sched_clocksource == cs) {
> > + rcu_assign_pointer(sched_clocksource, &clocksource_jiffies);
> > + spin_unlock_irqrestore(&clocksource_lock, flags);
> > + synchronize_rcu();
> > + return;
> > + }
> > +
> > spin_unlock_irqrestore(&clocksource_lock, flags);
> > }
>
>
> What if there's multiple CLOCK_SOURCE_USER_FOR_SCHED_CLOCK [ damn, thats
> a long name to type :-) ] ?
>
> That is, should we have logic in select_clocksource that does:
>
> if ((next->flags & ..) && next->prio > sched_clocksource->prio)
>
> or whatever, so that it picks the best one?
>
> Same for unregister, should we re-evaluate all clocksources before
> falling back to basic?
We already do via select_clocksource(), if we are unregistering the
current one then a new one with the flag set is selected. Before that,
the override is likewise given preference, and we fall back on jiffies if
there is nothing else. I suppose we could try and find the "best" one,
but I think the override and manual clocksource selection should be fine
for this.
Now that you mention it though, the sched_clocksource() assignment within
select_clocksource() happens underneath the clocksource_lock, but is not
using rcu_assign_pointer(). If the assignment there needs to use
rcu_assign_pointer() then presumably all of the unlock paths that do
select_clocksource() will have to synchronize_rcu()?
On Tue, 2009-06-02 at 16:35 +0900, Paul Mundt wrote:
>
> We already do via select_clocksource(), if we are unregistering the
> current one then a new one with the flag set is selected. Before that,
> the override is likewise given preference, and we fall back on jiffies if
> there is nothing else. I suppose we could try and find the "best" one,
> but I think the override and manual clocksource selection should be fine
> for this.
Ah, ok. So unregister calls select_clocksource again? That does leave us
a small window with jiffies, but I guess that's ok.
> Now that you mention it though, the sched_clocksource() assignment within
> select_clocksource() happens underneath the clocksource_lock, but is not
> using rcu_assign_pointer().
Right, that would want fixing indeed.
> If the assignment there needs to use
> rcu_assign_pointer() then presumably all of the unlock paths that do
> select_clocksource() will have to synchronize_rcu()?
No, you only have to do sync_rcu() when stuff that could have referenced
is going away and you cannot use call_rcu().
So when selecting a new clocksource, you don't need synchonization
because stuff doesn't go away (I think :-)
On Tue, Jun 02, 2009 at 09:41:35AM +0200, Peter Zijlstra wrote:
> On Tue, 2009-06-02 at 16:35 +0900, Paul Mundt wrote:
> >
> > We already do via select_clocksource(), if we are unregistering the
> > current one then a new one with the flag set is selected. Before that,
> > the override is likewise given preference, and we fall back on jiffies if
> > there is nothing else. I suppose we could try and find the "best" one,
> > but I think the override and manual clocksource selection should be fine
> > for this.
>
> Ah, ok. So unregister calls select_clocksource again? That does leave us
> a small window with jiffies, but I guess that's ok.
>
A synchronize_rcu() would fix that up, but I think a small window with
jiffies is less painful than sorting out RCU ordering and synchronization
for a corner case of a corner case ;-)
> > Now that you mention it though, the sched_clocksource() assignment within
> > select_clocksource() happens underneath the clocksource_lock, but is not
> > using rcu_assign_pointer().
>
> Right, that would want fixing indeed.
>
> > If the assignment there needs to use
> > rcu_assign_pointer() then presumably all of the unlock paths that do
> > select_clocksource() will have to synchronize_rcu()?
>
> No, you only have to do sync_rcu() when stuff that could have referenced
> is going away and you cannot use call_rcu().
>
> So when selecting a new clocksource, you don't need synchonization
> because stuff doesn't go away (I think :-)
Ok, that keeps things more simplified then. How does this look?
---
include/linux/clocksource.h | 4 +++-
kernel/sched_clock.c | 13 +++++++++++--
kernel/time/clocksource.c | 19 +++++++++++++++++++
kernel/time/jiffies.c | 2 +-
4 files changed, 34 insertions(+), 4 deletions(-)
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c56457c..2109940 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -202,7 +202,8 @@ struct clocksource {
#endif
};
-extern struct clocksource *clock; /* current clocksource */
+extern struct clocksource *clock; /* current clocksource */
+extern struct clocksource *sched_clocksource; /* sched_clock() clocksource */
/*
* Clock source flags bits::
@@ -212,6 +213,7 @@ extern struct clocksource *clock; /* current clocksource */
#define CLOCK_SOURCE_WATCHDOG 0x10
#define CLOCK_SOURCE_VALID_FOR_HRES 0x20
+#define CLOCK_SOURCE_USE_FOR_SCHED_CLOCK 0x40
/* simplify initialization of mask field */
#define CLOCKSOURCE_MASK(bits) (cycle_t)((bits) < 64 ? ((1ULL<<(bits))-1) : -1)
diff --git a/kernel/sched_clock.c b/kernel/sched_clock.c
index e1d16c9..b51d48d 100644
--- a/kernel/sched_clock.c
+++ b/kernel/sched_clock.c
@@ -30,6 +30,8 @@
#include <linux/percpu.h>
#include <linux/ktime.h>
#include <linux/sched.h>
+#include <linux/clocksource.h>
+#include <linux/rcupdate.h>
/*
* Scheduler clock - returns current time in nanosec units.
@@ -38,8 +40,15 @@
*/
unsigned long long __attribute__((weak)) sched_clock(void)
{
- return (unsigned long long)(jiffies - INITIAL_JIFFIES)
- * (NSEC_PER_SEC / HZ);
+ unsigned long long time;
+ struct clocksource *clock;
+
+ rcu_read_lock();
+ clock = rcu_dereference(sched_clocksource);
+ time = cyc2ns(clock, clocksource_read(clock));
+ rcu_read_unlock();
+
+ return time;
}
static __read_mostly int sched_clock_running;
diff --git a/kernel/time/clocksource.c b/kernel/time/clocksource.c
index 80189f6..f7243f2 100644
--- a/kernel/time/clocksource.c
+++ b/kernel/time/clocksource.c
@@ -25,6 +25,7 @@
*/
#include <linux/clocksource.h>
+#include <linux/rcupdate.h>
#include <linux/sysdev.h>
#include <linux/init.h>
#include <linux/module.h>
@@ -109,6 +110,7 @@ EXPORT_SYMBOL(timecounter_cyc2time);
/* XXX - Would like a better way for initializing curr_clocksource */
extern struct clocksource clocksource_jiffies;
+struct clocksource *sched_clocksource = &clocksource_jiffies;
/*[Clocksource internal variables]---------
* curr_clocksource:
@@ -362,6 +364,9 @@ static struct clocksource *select_clocksource(void)
if (next == curr_clocksource)
return NULL;
+ if (next->flags & CLOCK_SOURCE_USE_FOR_SCHED_CLOCK)
+ rcu_assign_pointer(sched_clocksource, next);
+
return next;
}
@@ -440,7 +445,21 @@ void clocksource_unregister(struct clocksource *cs)
list_del(&cs->list);
if (clocksource_override == cs)
clocksource_override = NULL;
+
next_clocksource = select_clocksource();
+
+ /*
+ * If select_clocksource() fails to find another suitable
+ * clocksource for sched_clocksource and we are unregistering
+ * it, switch back to jiffies.
+ */
+ if (sched_clocksource == cs) {
+ rcu_assign_pointer(sched_clocksource, &clocksource_jiffies);
+ spin_unlock_irqrestore(&clocksource_lock, flags);
+ synchronize_rcu();
+ return;
+ }
+
spin_unlock_irqrestore(&clocksource_lock, flags);
}
diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
index c3f6c30..727d881 100644
--- a/kernel/time/jiffies.c
+++ b/kernel/time/jiffies.c
@@ -52,7 +52,7 @@
static cycle_t jiffies_read(struct clocksource *cs)
{
- return (cycle_t) jiffies;
+ return (cycle_t) (jiffies - INITIAL_JIFFIES);
}
struct clocksource clocksource_jiffies = {
On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> On Tue, Jun 02, 2009 at 09:41:35AM +0200, Peter Zijlstra wrote:
> > On Tue, 2009-06-02 at 16:35 +0900, Paul Mundt wrote:
> > >
> > > We already do via select_clocksource(), if we are unregistering the
> > > current one then a new one with the flag set is selected. Before that,
> > > the override is likewise given preference, and we fall back on jiffies if
> > > there is nothing else. I suppose we could try and find the "best" one,
> > > but I think the override and manual clocksource selection should be fine
> > > for this.
> >
> > Ah, ok. So unregister calls select_clocksource again? That does leave us
> > a small window with jiffies, but I guess that's ok.
> >
> A synchronize_rcu() would fix that up, but I think a small window with
> jiffies is less painful than sorting out RCU ordering and synchronization
> for a corner case of a corner case ;-)
>
> > > Now that you mention it though, the sched_clocksource() assignment within
> > > select_clocksource() happens underneath the clocksource_lock, but is not
> > > using rcu_assign_pointer().
> >
> > Right, that would want fixing indeed.
> >
> > > If the assignment there needs to use
> > > rcu_assign_pointer() then presumably all of the unlock paths that do
> > > select_clocksource() will have to synchronize_rcu()?
> >
> > No, you only have to do sync_rcu() when stuff that could have referenced
> > is going away and you cannot use call_rcu().
> >
> > So when selecting a new clocksource, you don't need synchonization
> > because stuff doesn't go away (I think :-)
>
> Ok, that keeps things more simplified then. How does this look?
Looks fine to me,
Acked-by: Peter Zijlstra <[email protected]>
On Tue, Jun 02, 2009 at 10:00:03AM +0200, Peter Zijlstra wrote:
> On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> > On Tue, Jun 02, 2009 at 09:41:35AM +0200, Peter Zijlstra wrote:
> > > On Tue, 2009-06-02 at 16:35 +0900, Paul Mundt wrote:
> > > >
> > > > We already do via select_clocksource(), if we are unregistering the
> > > > current one then a new one with the flag set is selected. Before that,
> > > > the override is likewise given preference, and we fall back on jiffies if
> > > > there is nothing else. I suppose we could try and find the "best" one,
> > > > but I think the override and manual clocksource selection should be fine
> > > > for this.
> > >
> > > Ah, ok. So unregister calls select_clocksource again? That does leave us
> > > a small window with jiffies, but I guess that's ok.
> > >
> > A synchronize_rcu() would fix that up, but I think a small window with
> > jiffies is less painful than sorting out RCU ordering and synchronization
> > for a corner case of a corner case ;-)
> >
> > > > Now that you mention it though, the sched_clocksource() assignment within
> > > > select_clocksource() happens underneath the clocksource_lock, but is not
> > > > using rcu_assign_pointer().
> > >
> > > Right, that would want fixing indeed.
> > >
> > > > If the assignment there needs to use
> > > > rcu_assign_pointer() then presumably all of the unlock paths that do
> > > > select_clocksource() will have to synchronize_rcu()?
> > >
> > > No, you only have to do sync_rcu() when stuff that could have referenced
> > > is going away and you cannot use call_rcu().
> > >
> > > So when selecting a new clocksource, you don't need synchonization
> > > because stuff doesn't go away (I think :-)
> >
> > Ok, that keeps things more simplified then. How does this look?
>
>
> Looks fine to me,
>
> Acked-by: Peter Zijlstra <[email protected]>
Thanks for the help!
On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> unsigned long long __attribute__((weak)) sched_clock(void)
> {
> - return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> - * (NSEC_PER_SEC / HZ);
> + unsigned long long time;
> + struct clocksource *clock;
> +
> + rcu_read_lock();
> + clock = rcu_dereference(sched_clocksource);
> + time = cyc2ns(clock, clocksource_read(clock));
> + rcu_read_unlock();
> +
> + return time;
> }
My concerns with the locking here still stand. Nothing you've said or
done bolsters the clocksource in modules argument. I think what your
planning for sh clocksources seems very inelegant. I would imagine a
better solution is out there. I'd prefer if you just leave sched_clock
alone.
Daniel
On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> @@ -212,6 +213,7 @@ extern struct clocksource *clock; /* current clocksource */
>
> #define CLOCK_SOURCE_WATCHDOG 0x10
> #define CLOCK_SOURCE_VALID_FOR_HRES 0x20
> +#define CLOCK_SOURCE_USE_FOR_SCHED_CLOCK 0x40
One more thing though, as John suggested, this bit might want a proper
comment explaining when a clocksource should and should not set this
bit.
On Tue, Jun 02, 2009 at 04:17:18PM +0900, Paul Mundt wrote:
> sched: sched_clock() clocksource handling.
>
> There are presently a number of issues and limitations with how the
> clocksource and sched_clock() interaction works today. Configurations
> tend to be grouped in to one of the following:
>
> - Platform provides a clocksource unsuitable for sched_clock()
> and prefers to use the generic jiffies-backed implementation.
>
> - Platform provides its own clocksource and sched_clock() that
> wraps in to it.
>
> - Platform uses a generic clocksource (ie, drivers/clocksource/)
> combined with the generic jiffies-backed sched_clock().
>
> - Platform supports multiple sched_clock()-capable clocksources.
>
> This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address
> these issues, which can be set for any sched_clock()-capable clocksource.
>
> The generic sched_clock() implementation is likewise switched over to
> always read from a designated sched_clocksource, which is default
> initialized to the jiffies clocksource and updated based on the
> availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses
> the generic cyc2ns() logic on the clocksource ->read(), most of the
> platform-specific sched_clock() implementations can subsequently be
> killed off.
80ea3bac3a47bc73efa334d0dd57099d0ff14216 ("ARM: OMAP: sched_clock()
corrected") seems to have switched omap from cyc2ns() to an open coded
calculation based on mult_orig because cyc2ns() uses the NTP-adjusted
mult instead. Does that apply here or is that change incorrect?
Rabin
On Tue, 2009-06-02 at 19:47 +0530, Rabin Vincent wrote:
> On Tue, Jun 02, 2009 at 04:17:18PM +0900, Paul Mundt wrote:
> > sched: sched_clock() clocksource handling.
> >
> > There are presently a number of issues and limitations with how the
> > clocksource and sched_clock() interaction works today. Configurations
> > tend to be grouped in to one of the following:
> >
> > - Platform provides a clocksource unsuitable for sched_clock()
> > and prefers to use the generic jiffies-backed implementation.
> >
> > - Platform provides its own clocksource and sched_clock() that
> > wraps in to it.
> >
> > - Platform uses a generic clocksource (ie, drivers/clocksource/)
> > combined with the generic jiffies-backed sched_clock().
> >
> > - Platform supports multiple sched_clock()-capable clocksources.
> >
> > This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address
> > these issues, which can be set for any sched_clock()-capable clocksource.
> >
> > The generic sched_clock() implementation is likewise switched over to
> > always read from a designated sched_clocksource, which is default
> > initialized to the jiffies clocksource and updated based on the
> > availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses
> > the generic cyc2ns() logic on the clocksource ->read(), most of the
> > platform-specific sched_clock() implementations can subsequently be
> > killed off.
>
> 80ea3bac3a47bc73efa334d0dd57099d0ff14216 ("ARM: OMAP: sched_clock()
> corrected") seems to have switched omap from cyc2ns() to an open coded
> calculation based on mult_orig because cyc2ns() uses the NTP-adjusted
> mult instead. Does that apply here or is that change incorrect?
Hmm, that would be a problem, because changing the mult can cause non
monotonic behaviour.
On Tue, 2 Jun 2009, Peter Zijlstra wrote:
> On Tue, 2009-06-02 at 16:35 +0900, Paul Mundt wrote:
> >
> > We already do via select_clocksource(), if we are unregistering the
> > current one then a new one with the flag set is selected. Before that,
> > the override is likewise given preference, and we fall back on jiffies if
> > there is nothing else. I suppose we could try and find the "best" one,
> > but I think the override and manual clocksource selection should be fine
> > for this.
>
> Ah, ok. So unregister calls select_clocksource again? That does leave us
> a small window with jiffies, but I guess that's ok.
>
> > Now that you mention it though, the sched_clocksource() assignment within
> > select_clocksource() happens underneath the clocksource_lock, but is not
> > using rcu_assign_pointer().
>
> Right, that would want fixing indeed.
>
> > If the assignment there needs to use
> > rcu_assign_pointer() then presumably all of the unlock paths that do
> > select_clocksource() will have to synchronize_rcu()?
>
> No, you only have to do sync_rcu() when stuff that could have referenced
> is going away and you cannot use call_rcu().
>
> So when selecting a new clocksource, you don't need synchonization
> because stuff doesn't go away (I think :-)
Hmm, no. In the unregister case stuff _IS_ going away. That's why you
unregister in the first place, right ?
Thanks,
tglx
On Tue, 2 Jun 2009, Daniel Walker wrote:
> On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> > unsigned long long __attribute__((weak)) sched_clock(void)
> > {
> > - return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> > - * (NSEC_PER_SEC / HZ);
> > + unsigned long long time;
> > + struct clocksource *clock;
> > +
> > + rcu_read_lock();
> > + clock = rcu_dereference(sched_clocksource);
> > + time = cyc2ns(clock, clocksource_read(clock));
> > + rcu_read_unlock();
> > +
> > + return time;
> > }
>
> My concerns with the locking here still stand. Nothing you've said or
> done bolsters the clocksource in modules argument. I think what your
Can you please stop to belabor modules? Again, it is totally
_irrelevant_ whether the clocksource is in a module or not.
unregister_clocksource() can be called from compiled in code as well
to replace a clocksource which is physically shut down. We have to
deal with that modules or not.
Thanks,
tglx
On Tue, 2009-06-02 at 16:17 +0900, Paul Mundt wrote:
> As there was no additional feedback on the most recent version of this
> patch, I'm resubmitting it for inclusion. As far as I know there are no
> more outstanding concerns.
>
> --
>
> sched: sched_clock() clocksource handling.
>
> There are presently a number of issues and limitations with how the
> clocksource and sched_clock() interaction works today. Configurations
> tend to be grouped in to one of the following:
>
> - Platform provides a clocksource unsuitable for sched_clock()
> and prefers to use the generic jiffies-backed implementation.
>
> - Platform provides its own clocksource and sched_clock() that
> wraps in to it.
>
> - Platform uses a generic clocksource (ie, drivers/clocksource/)
> combined with the generic jiffies-backed sched_clock().
>
> - Platform supports multiple sched_clock()-capable clocksources.
>
> This patch adds a new CLOCK_SOURCE_USE_FOR_SCHED_CLOCK flag to address
> these issues, which can be set for any sched_clock()-capable clocksource.
>
> The generic sched_clock() implementation is likewise switched over to
> always read from a designated sched_clocksource, which is default
> initialized to the jiffies clocksource and updated based on the
> availability of CLOCK_SOURCE_USE_FOR_SCHED_CLOCK sources. As this uses
> the generic cyc2ns() logic on the clocksource ->read(), most of the
> platform-specific sched_clock() implementations can subsequently be
> killed off.
>
> Signed-off-by: Paul Mundt <[email protected]>
Yea, I think this is nicer then your earlier patch. Thanks for reworking
it. Although there are a few concerns though that came up from an email
with Peter:
From: Peter Zijlstra <[email protected]>
"
> 2) How long does it have to be monotonic for?
Forever? (per cpu)
> Is it ok if it wraps every few seconds?
No, if it wraps it needs to wrap on u64.
"
> /*
> * Scheduler clock - returns current time in nanosec units.
> @@ -38,8 +40,15 @@
> */
> unsigned long long __attribute__((weak)) sched_clock(void)
> {
> - return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> - * (NSEC_PER_SEC / HZ);
> + unsigned long long time;
> + struct clocksource *clock;
> +
> + rcu_read_lock();
> + clock = rcu_dereference(sched_clocksource);
> + time = cyc2ns(clock, clocksource_read(clock));
> + rcu_read_unlock();
> +
> + return time;
> }
So in the above, cyc2ns could overflow prior to a u64 wrap.
cyc2ns does the following:
(cycles * cs->mult) >> cs->shift;
The (cycles*cs->mult) bit may overflow for large cycle values, and its
likely that could be fairly quickly, as ideally we have a large shift
value to keep the precision high so mult will also be large.
I just went through some of the math here with Jon Hunter in this
thread: http://lkml.org/lkml/2009/5/15/466
None the less, either sched_clock will have to handle overflows or we'll
need to do something like the timekeeping code where there's an periodic
accumulation step that keeps the unaccumulated cycles small.
That said, the x86 sched_clock() uses cycles_2_ns() which is similar
(but with a smaller scale value). So its likely it would also overflow
prior to the u64 boundary as well.
> diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
> index c3f6c30..727d881 100644
> --- a/kernel/time/jiffies.c
> +++ b/kernel/time/jiffies.c
> @@ -52,7 +52,7 @@
>
> static cycle_t jiffies_read(struct clocksource *cs)
> {
> - return (cycle_t) jiffies;
> + return (cycle_t) (jiffies - INITIAL_JIFFIES);
> }
Also, on 32bit systems this will may overflow ~monthly. However, this
isn't different then the existing sched_clock() implementation, so
either its been already handled and sched_clock is more robust then I
thought or there's a bug there.
thanks
-john
On Tue, Jun 02, 2009 at 04:49:26AM -0700, Daniel Walker wrote:
> On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> > unsigned long long __attribute__((weak)) sched_clock(void)
> > {
> > - return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> > - * (NSEC_PER_SEC / HZ);
> > + unsigned long long time;
> > + struct clocksource *clock;
> > +
> > + rcu_read_lock();
> > + clock = rcu_dereference(sched_clocksource);
> > + time = cyc2ns(clock, clocksource_read(clock));
> > + rcu_read_unlock();
> > +
> > + return time;
> > }
>
> My concerns with the locking here still stand. Nothing you've said or
> done bolsters the clocksource in modules argument. I think what your
> planning for sh clocksources seems very inelegant. I would imagine a
> better solution is out there. I'd prefer if you just leave sched_clock
> alone.
>
This is the first I've heard you mention locking concerns, and as usual
there is not enough technical content (or any, really) to go on to even
reply to this. Whether you consider my solution for sh clocksources
elegant or not is irrelevant, as I wasn't soliciting feedback, and it's a
problem that has to be dealt with regardless of whether it's a pretty one
or not.
If at such a time you wish to post something bordering on a real
technical concern, we can continue this thread of conversation, until
then I'll be sure to drop you from future versions of the patch. If you
want to hand-wave, do it somewhere else, thanks.
On Tue, Jun 02, 2009 at 10:17:23PM +0200, Thomas Gleixner wrote:
> On Tue, 2 Jun 2009, Peter Zijlstra wrote:
>
> > On Tue, 2009-06-02 at 16:35 +0900, Paul Mundt wrote:
> > >
> > > We already do via select_clocksource(), if we are unregistering the
> > > current one then a new one with the flag set is selected. Before that,
> > > the override is likewise given preference, and we fall back on jiffies if
> > > there is nothing else. I suppose we could try and find the "best" one,
> > > but I think the override and manual clocksource selection should be fine
> > > for this.
> >
> > Ah, ok. So unregister calls select_clocksource again? That does leave us
> > a small window with jiffies, but I guess that's ok.
> >
> > > Now that you mention it though, the sched_clocksource() assignment within
> > > select_clocksource() happens underneath the clocksource_lock, but is not
> > > using rcu_assign_pointer().
> >
> > Right, that would want fixing indeed.
> >
> > > If the assignment there needs to use
> > > rcu_assign_pointer() then presumably all of the unlock paths that do
> > > select_clocksource() will have to synchronize_rcu()?
> >
> > No, you only have to do sync_rcu() when stuff that could have referenced
> > is going away and you cannot use call_rcu().
> >
> > So when selecting a new clocksource, you don't need synchonization
> > because stuff doesn't go away (I think :-)
>
> Hmm, no. In the unregister case stuff _IS_ going away. That's why you
> unregister in the first place, right ?
>
sched_clocksource will be updated in the unregister path, the clocksource
being unregistered will go away, and we use RCU synchronization to handle
the case where the current clocksource assigned to sched_clocksource is
being unregistered. So while the clocksource is going away,
sched_clocksource is not.
On Tue, 2009-06-02 at 15:24 -0700, john stultz wrote:
> > /*
> > * Scheduler clock - returns current time in nanosec units.
> > @@ -38,8 +40,15 @@
> > */
> > unsigned long long __attribute__((weak)) sched_clock(void)
> > {
> > - return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> > - * (NSEC_PER_SEC / HZ);
> > + unsigned long long time;
> > + struct clocksource *clock;
> > +
> > + rcu_read_lock();
> > + clock = rcu_dereference(sched_clocksource);
> > + time = cyc2ns(clock, clocksource_read(clock));
> > + rcu_read_unlock();
> > +
> > + return time;
> > }
>
> So in the above, cyc2ns could overflow prior to a u64 wrap.
>
>
> cyc2ns does the following:
> (cycles * cs->mult) >> cs->shift;
>
> The (cycles*cs->mult) bit may overflow for large cycle values, and its
> likely that could be fairly quickly, as ideally we have a large shift
> value to keep the precision high so mult will also be large.
>
> I just went through some of the math here with Jon Hunter in this
> thread: http://lkml.org/lkml/2009/5/15/466
>
> None the less, either sched_clock will have to handle overflows or we'll
> need to do something like the timekeeping code where there's an periodic
> accumulation step that keeps the unaccumulated cycles small.
>
> That said, the x86 sched_clock() uses cycles_2_ns() which is similar
> (but with a smaller scale value). So its likely it would also overflow
> prior to the u64 boundary as well.
>
>
> > diff --git a/kernel/time/jiffies.c b/kernel/time/jiffies.c
> > index c3f6c30..727d881 100644
> > --- a/kernel/time/jiffies.c
> > +++ b/kernel/time/jiffies.c
> > @@ -52,7 +52,7 @@
> >
> > static cycle_t jiffies_read(struct clocksource *cs)
> > {
> > - return (cycle_t) jiffies;
> > + return (cycle_t) (jiffies - INITIAL_JIFFIES);
> > }
>
> Also, on 32bit systems this will may overflow ~monthly. However, this
> isn't different then the existing sched_clock() implementation, so
> either its been already handled and sched_clock is more robust then I
> thought or there's a bug there.
I suspect you just found two bugs.. I thought to remember the jiffies
based sched clock used to use jiffies_64, but I might be mistaken.
As to the x86 sched_clock wrapping before 64bits, how soon would that
be? The scheduler code really assumes it wraps on 64bit and I expect it
to do something mighty odd when it wraps sooner (although I'd have to
audit the code to see exactly what).
Aah, I think the filtering in kernel/sched_clock.c fixes it up. The wrap
will be visible as a large backward motion which will be discarded.
On Wed, 2009-06-03 at 12:36 +0900, Paul Mundt wrote:
> On Tue, Jun 02, 2009 at 04:49:26AM -0700, Daniel Walker wrote:
> > On Tue, 2009-06-02 at 16:54 +0900, Paul Mundt wrote:
> > > unsigned long long __attribute__((weak)) sched_clock(void)
> > > {
> > > - return (unsigned long long)(jiffies - INITIAL_JIFFIES)
> > > - * (NSEC_PER_SEC / HZ);
> > > + unsigned long long time;
> > > + struct clocksource *clock;
> > > +
> > > + rcu_read_lock();
> > > + clock = rcu_dereference(sched_clocksource);
> > > + time = cyc2ns(clock, clocksource_read(clock));
> > > + rcu_read_unlock();
> > > +
> > > + return time;
> > > }
> >
> > My concerns with the locking here still stand. Nothing you've said or
> > done bolsters the clocksource in modules argument. I think what your
> > planning for sh clocksources seems very inelegant. I would imagine a
> > better solution is out there. I'd prefer if you just leave sched_clock
> > alone.
> >
> This is the first I've heard you mention locking concerns, and as usual
> there is not enough technical content (or any, really) to go on to even
> reply to this. Whether you consider my solution for sh clocksources
> elegant or not is irrelevant, as I wasn't soliciting feedback, and it's a
> problem that has to be dealt with regardless of whether it's a pretty one
> or not.
I think maybe your not reading my emails .. I said there is unnessesary
locking in sched_clock, and that it's fixing a problem that doesn't
exist (i.e. clocksources in modules) .. You claim you want clocksources
in modules because you have a useful case in sh, which I consider not
very useful .. And your refusing to remove the locking, or that's how it
seems. So I'd prefer you don't submit this code any longer. I don't
think you know what your doing in this case.
This is not hand waving, and it is technical.
Daniel