| Message ID | 20090602071718.GA17710@linux-sh.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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()? -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 :-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <lethal@linux-sh.org> 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 <peterz@infradead.org> " > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = {
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 <lethal@linux-sh.org> --- 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(-) -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html