| Message ID | 1369570367-994-2-git-send-email-baruch@tkos.co.il (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/26/2013 05:12 AM, Baruch Siach wrote: > ARM is the only architecture providing sched_clock.h and setup_sched_clock(). > Implement sched_clock() for use by other architectures. > > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: John Stultz <john.stultz@linaro.org> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jamie Iles <jamie@jamieiles.com> > Cc: Dinh Nguyen <dinguyen@altera.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > arch/arm/Kconfig | 1 + > drivers/clocksource/Kconfig | 3 +++ > drivers/clocksource/dw_apb_timer_of.c | 18 ++++++++++++++++++ > 3 files changed, 22 insertions(+) > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig > index 1cacda4..7489604 100644 > --- a/arch/arm/Kconfig > +++ b/arch/arm/Kconfig > @@ -47,6 +47,7 @@ config ARM > select HAVE_OPROFILE if (HAVE_PERF_EVENTS) > select HAVE_PERF_EVENTS > select HAVE_REGS_AND_STACK_ACCESS_API > + select HAVE_SETUP_SCHED_CLOCK > select HAVE_SYSCALL_TRACEPOINTS > select HAVE_UID16 > select KTIME_SCALAR > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig > index e507ab7..0fd6d13 100644 > --- a/drivers/clocksource/Kconfig > +++ b/drivers/clocksource/Kconfig > @@ -1,3 +1,6 @@ > +config HAVE_SETUP_SCHED_CLOCK > + bool > + > config CLKSRC_OF > bool > > diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c > index 5a61ebc..9fdb4d3 100644 > --- a/drivers/clocksource/dw_apb_timer_of.c > +++ b/drivers/clocksource/dw_apb_timer_of.c > @@ -21,7 +21,11 @@ > #include <linux/of_address.h> > #include <linux/of_irq.h> > > +#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK > #include <asm/sched_clock.h> > +#else > +#include <linux/sched.h> > +#endif > > static void timer_get_base_and_rate(struct device_node *np, > void __iomem **base, u32 *rate) > @@ -73,6 +77,9 @@ static void add_clocksource(struct device_node *source_timer) > } > > static void __iomem *sched_io_base; > +#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > +static u64 sched_clock_mult __read_mostly; > +#endif > > static u32 read_sched_clock(void) > { > @@ -97,7 +104,11 @@ static void init_sched_clock(void) > timer_get_base_and_rate(sched_timer, &sched_io_base, &rate); > of_node_put(sched_timer); > > +#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK > setup_sched_clock(read_sched_clock, 32, rate); > +#else > + sched_clock_mult = NSEC_PER_SEC / rate; > +#endif > } Can you rework this to not use #ifdefs within the function? They make it annoying to read the code. Instead maybe have a local setup_sched_clock() function that sets the mult value for the !CONFIG_HAVE_SETUP_SCHED_CLOCK case? > > static const struct of_device_id osctimer_ids[] __initconst = { > @@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) > > init_sched_clock(); > } > + > +#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > +unsigned long long notrace sched_clock() > +{ > + return read_sched_clock() * sched_clock_mult; > +} > +#endif Also, can you try to condense the number of #ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK checks to one, and consolidate the needed functions all in that one conditional? thanks -john
Hi John, On Tue, May 28, 2013 at 01:24:17PM -0700, John Stultz wrote: > On 05/26/2013 05:12 AM, Baruch Siach wrote: > >ARM is the only architecture providing sched_clock.h and setup_sched_clock(). > >Implement sched_clock() for use by other architectures. > > > >Cc: Ingo Molnar <mingo@redhat.com> > >Cc: Peter Zijlstra <peterz@infradead.org> > >Cc: John Stultz <john.stultz@linaro.org> > >Cc: Thomas Gleixner <tglx@linutronix.de> > >Cc: Jamie Iles <jamie@jamieiles.com> > >Cc: Dinh Nguyen <dinguyen@altera.com> > >Signed-off-by: Baruch Siach <baruch@tkos.co.il> > >--- [snip] > >@@ -73,6 +77,9 @@ static void add_clocksource(struct device_node > > *source_timer) > > } > > static void __iomem *sched_io_base; > >+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > >+static u64 sched_clock_mult __read_mostly; > >+#endif > > static u32 read_sched_clock(void) > > { > >@@ -97,7 +104,11 @@ static void init_sched_clock(void) > > timer_get_base_and_rate(sched_timer, &sched_io_base, &rate); > > of_node_put(sched_timer); > >+#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK > > setup_sched_clock(read_sched_clock, 32, rate); > >+#else > >+ sched_clock_mult = NSEC_PER_SEC / rate; > >+#endif > > } > > Can you rework this to not use #ifdefs within the function? They > make it annoying to read the code. > > Instead maybe have a local setup_sched_clock() function that sets > the mult value for the !CONFIG_HAVE_SETUP_SCHED_CLOCK case? > > > static const struct of_device_id osctimer_ids[] __initconst = { > >@@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) > > init_sched_clock(); > > } > >+ > >+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > >+unsigned long long notrace sched_clock() > >+{ > >+ return read_sched_clock() * sched_clock_mult; > >+} > >+#endif > > Also, can you try to condense the number of #ifndef > CONFIG_HAVE_SETUP_SCHED_CLOCK checks to one, and consolidate the > needed functions all in that one conditional? Thanks for your comments. I'll rework the patch and resubmit. I've just noticed that I have a bigger problem. read_sched_clock() returns u32, not u64. This means that in a rate of, say, 100MHz it will wrap around after a little more than 40 seconds. Would it make sense to put ARM's 32 bin sched_clock extension code (arch/arm/kernel/sched_clock.c) is a common place (maybe drivers/clocksource), and use that? There seems to be nothing ARM specific in this code, after all. baruch
On 05/29/2013 10:32 PM, Baruch Siach wrote: > Hi John, > > On Tue, May 28, 2013 at 01:24:17PM -0700, John Stultz wrote: >> On 05/26/2013 05:12 AM, Baruch Siach wrote: >>> static const struct of_device_id osctimer_ids[] __initconst = { >>> @@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) >>> init_sched_clock(); >>> } >>> + >>> +#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK >>> +unsigned long long notrace sched_clock() >>> +{ >>> + return read_sched_clock() * sched_clock_mult; >>> +} >>> +#endif >> Also, can you try to condense the number of #ifndef >> CONFIG_HAVE_SETUP_SCHED_CLOCK checks to one, and consolidate the >> needed functions all in that one conditional? > Thanks for your comments. I'll rework the patch and resubmit. > > I've just noticed that I have a bigger problem. read_sched_clock() returns > u32, not u64. This means that in a rate of, say, 100MHz it will wrap around > after a little more than 40 seconds. Would it make sense to put ARM's 32 bin > sched_clock extension code (arch/arm/kernel/sched_clock.c) is a common place > (maybe drivers/clocksource), and use that? There seems to be nothing ARM > specific in this code, after all. Yea, working out an actual generic sched_clock implementation is something I'd like to see done. Though I'd really rather we not toss yet another chunk of infrastructure in the drivers/clocksource directory. Instead we should probably have a kernel/time/sched_clock.c. Then its just the issue of tying that and the clocksource code together so you just register existing capable clocksources with a SCHED_CLOCK flag or via a different registration hook. I don't think it will be an easy job, but if you want to give it a shot, I'd be quite interested in reviewing the patches! thanks -john
Hi John, On Thu, May 30, 2013 at 09:11:17AM -0700, John Stultz wrote: > On 05/29/2013 10:32 PM, Baruch Siach wrote: > >On Tue, May 28, 2013 at 01:24:17PM -0700, John Stultz wrote: > >>On 05/26/2013 05:12 AM, Baruch Siach wrote: > >>> static const struct of_device_id osctimer_ids[] __initconst = { > >>>@@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) > >>> init_sched_clock(); > >>> } > >>>+ > >>>+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > >>>+unsigned long long notrace sched_clock() > >>>+{ > >>>+ return read_sched_clock() * sched_clock_mult; > >>>+} > >>>+#endif > >>Also, can you try to condense the number of #ifndef > >>CONFIG_HAVE_SETUP_SCHED_CLOCK checks to one, and consolidate the > >>needed functions all in that one conditional? > >Thanks for your comments. I'll rework the patch and resubmit. > > > >I've just noticed that I have a bigger problem. read_sched_clock() returns > >u32, not u64. This means that in a rate of, say, 100MHz it will wrap around > >after a little more than 40 seconds. Would it make sense to put ARM's 32 bin > >sched_clock extension code (arch/arm/kernel/sched_clock.c) is a common place > >(maybe drivers/clocksource), and use that? There seems to be nothing ARM > >specific in this code, after all. > > Yea, working out an actual generic sched_clock implementation is > something I'd like to see done. > > Though I'd really rather we not toss yet another chunk of > infrastructure in the drivers/clocksource directory. Instead we > should probably have a kernel/time/sched_clock.c. > > Then its just the issue of tying that and the clocksource code > together so you just register existing capable clocksources with a > SCHED_CLOCK flag or via a different registration hook. > > I don't think it will be an easy job, but if you want to give it a > shot, I'd be quite interested in reviewing the patches! Although I'd very much like to contribute in this area, I'm afraid I don't have time for this now. My problem is much narrower in scope, though, and I think it can be solved without a generic sched_clock implementation at first step. All I want is to use a 32 bit counter for sched_clock. The code enabling this is currently only available for ARM (arch/arm/kernel/sched_clock.c). Should this part of the code move to kernel/time/sched_clock.c? baruch
Hi! > > >@@ -73,6 +77,9 @@ static void add_clocksource(struct device_node > > > *source_timer) > > > } > > > static void __iomem *sched_io_base; > > >+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > > >+static u64 sched_clock_mult __read_mostly; > > >+#endif > > > static u32 read_sched_clock(void) > > > { > > >@@ -97,7 +104,11 @@ static void init_sched_clock(void) > > > timer_get_base_and_rate(sched_timer, &sched_io_base, &rate); > > > of_node_put(sched_timer); > > >+#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK > > > setup_sched_clock(read_sched_clock, 32, rate); > > >+#else > > >+ sched_clock_mult = NSEC_PER_SEC / rate; > > >+#endif > > > } > > > > Can you rework this to not use #ifdefs within the function? They > > make it annoying to read the code. > > > > Instead maybe have a local setup_sched_clock() function that sets > > the mult value for the !CONFIG_HAVE_SETUP_SCHED_CLOCK case? > > > > > static const struct of_device_id osctimer_ids[] __initconst = { > > >@@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) > > > init_sched_clock(); > > > } > > >+ > > >+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > > >+unsigned long long notrace sched_clock() > > >+{ > > >+ return read_sched_clock() * sched_clock_mult; > > >+} > > >+#endif > > > > Also, can you try to condense the number of #ifndef > > CONFIG_HAVE_SETUP_SCHED_CLOCK checks to one, and consolidate the > > needed functions all in that one conditional? > > Thanks for your comments. I'll rework the patch and resubmit. > > I've just noticed that I have a bigger problem. read_sched_clock() returns > u32, not u64. This means that in a rate of, say, 100MHz it will wrap around > after a little more than 40 seconds. Would it make sense to put ARM's 32 bin > sched_clock extension code (arch/arm/kernel/sched_clock.c) is a common place > (maybe drivers/clocksource), and use that? There seems to be nothing ARM > specific in this code, after all. Also note that there are two conflicting changes to this area pending. It seems one in soc-next arm tree will prevail. Please take a look... Pavel
Hi Pavel, On Thu, Jun 20, 2013 at 04:44:33PM +0200, Pavel Machek wrote: > > > >@@ -73,6 +77,9 @@ static void add_clocksource(struct device_node > > > > *source_timer) > > > > } > > > > static void __iomem *sched_io_base; > > > >+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > > > >+static u64 sched_clock_mult __read_mostly; > > > >+#endif > > > > static u32 read_sched_clock(void) > > > > { > > > >@@ -97,7 +104,11 @@ static void init_sched_clock(void) > > > > timer_get_base_and_rate(sched_timer, &sched_io_base, &rate); > > > > of_node_put(sched_timer); > > > >+#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK > > > > setup_sched_clock(read_sched_clock, 32, rate); > > > >+#else > > > >+ sched_clock_mult = NSEC_PER_SEC / rate; > > > >+#endif > > > > } > > > > > > Can you rework this to not use #ifdefs within the function? They > > > make it annoying to read the code. > > > > > > Instead maybe have a local setup_sched_clock() function that sets > > > the mult value for the !CONFIG_HAVE_SETUP_SCHED_CLOCK case? > > > > > > > static const struct of_device_id osctimer_ids[] __initconst = { > > > >@@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) > > > > init_sched_clock(); > > > > } > > > >+ > > > >+#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK > > > >+unsigned long long notrace sched_clock() > > > >+{ > > > >+ return read_sched_clock() * sched_clock_mult; > > > >+} > > > >+#endif > > > > > > Also, can you try to condense the number of #ifndef > > > CONFIG_HAVE_SETUP_SCHED_CLOCK checks to one, and consolidate the > > > needed functions all in that one conditional? > > > > Thanks for your comments. I'll rework the patch and resubmit. > > > > I've just noticed that I have a bigger problem. read_sched_clock() returns > > u32, not u64. This means that in a rate of, say, 100MHz it will wrap around > > after a little more than 40 seconds. Would it make sense to put ARM's 32 bin > > sched_clock extension code (arch/arm/kernel/sched_clock.c) is a common place > > (maybe drivers/clocksource), and use that? There seems to be nothing ARM > > specific in this code, after all. > > Also note that there are two conflicting changes to this area > pending. It seems one in soc-next arm tree will prevail. > > Please take a look... Now that generic sched_clock implementation is available to all architectures (38ff87f77 "sched_clock: Make ARM's sched_clock generic for all architectures" in the timers/core branch of the tip tree) this patch is not needed anymore. Thanks for the head up. baruch
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 1cacda4..7489604 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -47,6 +47,7 @@ config ARM select HAVE_OPROFILE if (HAVE_PERF_EVENTS) select HAVE_PERF_EVENTS select HAVE_REGS_AND_STACK_ACCESS_API + select HAVE_SETUP_SCHED_CLOCK select HAVE_SYSCALL_TRACEPOINTS select HAVE_UID16 select KTIME_SCALAR diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig index e507ab7..0fd6d13 100644 --- a/drivers/clocksource/Kconfig +++ b/drivers/clocksource/Kconfig @@ -1,3 +1,6 @@ +config HAVE_SETUP_SCHED_CLOCK + bool + config CLKSRC_OF bool diff --git a/drivers/clocksource/dw_apb_timer_of.c b/drivers/clocksource/dw_apb_timer_of.c index 5a61ebc..9fdb4d3 100644 --- a/drivers/clocksource/dw_apb_timer_of.c +++ b/drivers/clocksource/dw_apb_timer_of.c @@ -21,7 +21,11 @@ #include <linux/of_address.h> #include <linux/of_irq.h> +#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK #include <asm/sched_clock.h> +#else +#include <linux/sched.h> +#endif static void timer_get_base_and_rate(struct device_node *np, void __iomem **base, u32 *rate) @@ -73,6 +77,9 @@ static void add_clocksource(struct device_node *source_timer) } static void __iomem *sched_io_base; +#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK +static u64 sched_clock_mult __read_mostly; +#endif static u32 read_sched_clock(void) { @@ -97,7 +104,11 @@ static void init_sched_clock(void) timer_get_base_and_rate(sched_timer, &sched_io_base, &rate); of_node_put(sched_timer); +#ifdef CONFIG_HAVE_SETUP_SCHED_CLOCK setup_sched_clock(read_sched_clock, 32, rate); +#else + sched_clock_mult = NSEC_PER_SEC / rate; +#endif } static const struct of_device_id osctimer_ids[] __initconst = { @@ -124,3 +135,10 @@ void __init dw_apb_timer_init(void) init_sched_clock(); } + +#ifndef CONFIG_HAVE_SETUP_SCHED_CLOCK +unsigned long long notrace sched_clock() +{ + return read_sched_clock() * sched_clock_mult; +} +#endif
ARM is the only architecture providing sched_clock.h and setup_sched_clock(). Implement sched_clock() for use by other architectures. Cc: Ingo Molnar <mingo@redhat.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: John Stultz <john.stultz@linaro.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jamie Iles <jamie@jamieiles.com> Cc: Dinh Nguyen <dinguyen@altera.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- arch/arm/Kconfig | 1 + drivers/clocksource/Kconfig | 3 +++ drivers/clocksource/dw_apb_timer_of.c | 18 ++++++++++++++++++ 3 files changed, 22 insertions(+)