Prev: MIPS TSC synchronization
Next: [tip:x86/mm] x86, numa, bootmem: Only free bootmem on NUMA failure path
From: Ingo Molnar on 22 Nov 2009 03:20

* Wu Zhangjin <wuzhangjin(a)gmail.com> wrote:

> +config HR_SCHED_CLOCK
> + bool "High Resolution sched_clock()"
> + depends on CSRC_R4K
> + default n
> + help
> + This option enables the MIPS c0 count based high resolution
> + sched_clock().
> +
> + If you need a ns precision timestamp, You are recommended to enable
> + this option. For example, If you are using the Ftrace subsystem to do
> + real time tracing, this option is needed.

s/You/you

> +
> + If unsure, disable it.
> +
> +config HR_SCHED_CLOCK_UPDATE
> + bool "Update sched_clock() automatically"
> + depends on HR_SCHED_CLOCK
> + default y
> + help
> + Because Some of the MIPS c0 count period is quite short and because
> + cnt32_to_63() needs to be called at least once per half period to
> + work properly, a kernel timer is needed to set up to ensure this
> + requirement is always met.

s/Some/some

Why is this a config option? I suspect that hardware that _needs_ this
keep-warm periodic tick we should enable it unconditionally and
automatically - otherwise the user can misconfigure the kernel with a
bad clock.


> +
> + If unusre, enable it.

s/unusre/unsure

> +
> +#
> # Timer Interrupt Frequency Configuration
> #
>
> diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
> index e95a3cd..4e52cca 100644
> --- a/arch/mips/kernel/csrc-r4k.c
> +++ b/arch/mips/kernel/csrc-r4k.c
> @@ -6,10 +6,64 @@
> * Copyright (C) 2007 by Ralf Baechle
> */
> #include <linux/clocksource.h>
> +#include <linux/cnt32_to_63.h>
> +#include <linux/timer.h>
> #include <linux/init.h>
>
> #include <asm/time.h>
>
> +/*
> + * MIPS' sched_clock implementation.

s/MIPS'/MIPS's

or

s/MIPS'/MIPS

> + *
> + * because cnt32_to_63() needs to be called at least once per half period to
> + * work properly, and some of the MIPS' frequency is very low, perhaps a kernel
> + * timer is needed to be set up to ensure this requirement is always met.
> + * please refer to arch/arm/plat-orion/time.c and include/linux/cnt32_to_63.h

s/please/Please

> + */
> +static unsigned long __maybe_unused tclk2ns_scale;
> +static unsigned long __maybe_unused tclk2ns_scale_factor;


__read_mostly?

> +
> +#ifdef CONFIG_HR_SCHED_CLOCK
> +unsigned long long notrace sched_clock(void)
> +{
> + unsigned long long v = cnt32_to_63(read_c0_count());
> + return (v * tclk2ns_scale) >> tclk2ns_scale_factor;
> +}
> +#endif
> +
> +static void __init __maybe_unused setup_sched_clock(struct clocksource *cs)
> +{
> + unsigned long long v;
> +
> + v = cs->mult;
> + /*
> + * We want an even value to automatically clear the top bit
> + * returned by cnt32_to_63() without an additional run time
> + * instruction. So if the LSB is 1 then round it up.
> + */
> + if (v & 1)
> + v++;
>
> + tclk2ns_scale = v;
> + tclk2ns_scale_factor = cs->shift;
> +}
> +
> +static struct timer_list __maybe_unused cnt32_to_63_keepwarm_timer;
> +
> +static void __maybe_unused cnt32_to_63_keepwarm(unsigned long data)
> +{
> + mod_timer(&cnt32_to_63_keepwarm_timer, round_jiffies(jiffies + data));
> + (void) sched_clock();
>
> +}
> +
> +static void __maybe_unused setup_sched_clock_update(unsigned long tclk)
> +{
> + unsigned long data;
> +
> + data = (0xffffffffUL / tclk / 2 - 2) * HZ;
> + setup_timer(&cnt32_to_63_keepwarm_timer, cnt32_to_63_keepwarm, data);
> + mod_timer(&cnt32_to_63_keepwarm_timer, round_jiffies(jiffies + data));
> +}
> +
> static cycle_t c0_hpt_read(struct clocksource *cs)
> {
> return read_c0_count();
> @@ -32,7 +86,13 @@ int __init init_r4k_clocksource(void)
>
> clocksource_set_clock(&clocksource_mips, mips_hpt_frequency);
>
> +#ifdef CONFIG_HR_SCHED_CLOCK
> + setup_sched_clock(&clocksource_mips);
> +#endif
> clocksource_register(&clocksource_mips);
>
> +#ifdef CONFIG_HR_SCHED_CLOCK_UPDATE
> + setup_sched_clock_update(mips_hpt_frequency);
> +#endif
> return 0;

You could make the functions inline and move the #ifdef into those
functions.

That wold also remove those __maybe_unused tags.

Thanks,

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Wu Zhangjin on 22 Nov 2009 06:10
[...]
> > +
> > + If unsure, disable it.
> > +
> > +config HR_SCHED_CLOCK_UPDATE
> > + bool "Update sched_clock() automatically"
> > + depends on HR_SCHED_CLOCK
> > + default y
> > + help
> > + Because Some of the MIPS c0 count period is quite short and because
> > + cnt32_to_63() needs to be called at least once per half period to
> > + work properly, a kernel timer is needed to set up to ensure this
> > + requirement is always met.
>
> s/Some/some
>

Will apply all of the above feedbacks and the later relative ones,
thanks!

> Why is this a config option? I suspect that hardware that _needs_ this
> keep-warm periodic tick we should enable it unconditionally and
> automatically - otherwise the user can misconfigure the kernel with a
> bad clock.
>

It will be really hard to let the users make decision, so I tell them to
enable it if not sure. perhaps it's better to remove this option.

but one issue I have found here, if enabled this keep-warm by default, I
can not always get the result of function graph tracer, not sure what
the exact reason is. and even if get the result, lots of them are the
results about timer(that mod_timer() and sched_clock() is called
frequently?).

and as we can see from arch/arm/mach-pxa/time.c, there is also no
keep_warm there, which means that if the frequency of that MIPS is very
low, it will need enough seconds to make that 32bit long c0 count
overrall. so, they will not need this keep-warm. perhaps we can use that
mips_hpt_frequency variable to determine setup that keep-warm or not.
but it's hard to design the number of frequency.

[...]
> > + */
> > +static unsigned long __maybe_unused tclk2ns_scale;
> > +static unsigned long __maybe_unused tclk2ns_scale_factor;
>
>
> __read_mostly?
>

Yes.

[...]
> > +static void __maybe_unused setup_sched_clock_update(unsigned long tclk)
> > +{
> > + unsigned long data;
> > +
> > + data = (0xffffffffUL / tclk / 2 - 2) * HZ;

Because the MIPS c0 count's frequency is half of the cpu frequency(Hi,
Ralf, does every MIPS c0 count meet this feature?), so, the above line
should be:

data = (0xffffffffUL / tclk - 2) * HZ;

> > + setup_timer(&cnt32_to_63_keepwarm_timer, cnt32_to_63_keepwarm, data);
> > + mod_timer(&cnt32_to_63_keepwarm_timer, round_jiffies(jiffies + data));
> > +}
[...]
> >
> > +#ifdef CONFIG_HR_SCHED_CLOCK
> > + setup_sched_clock(&clocksource_mips);
> > +#endif
> > clocksource_register(&clocksource_mips);
> >
> > +#ifdef CONFIG_HR_SCHED_CLOCK_UPDATE
> > + setup_sched_clock_update(mips_hpt_frequency);
> > +#endif
> > return 0;
>
> You could make the functions inline and move the #ifdef into those
> functions.
>
> That wold also remove those __maybe_unused tags.
>

will apply it, thanks ;)

Regards,
Wu Zhangjin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Wu Zhangjin on 22 Nov 2009 08:20
On Sun, 2009-11-22 at 12:35 +0000, Ralf Baechle wrote:
> On Sun, Nov 22, 2009 at 07:08:05PM +0800, Wu Zhangjin wrote:
>
> > > > + data = (0xffffffffUL / tclk / 2 - 2) * HZ;
> >
> > Because the MIPS c0 count's frequency is half of the cpu frequency(Hi,
> > Ralf, does every MIPS c0 count meet this feature?), so, the above line
> > should be:
>
> There are processors which have no cp0 counter at all; these are mostly
> very old pre-R4000 era 32-bit MIPS I and MIPS II cores.
>
> Of those which have a cp0 counter most will clock it at "half the maximum
> instruction issue rate" and a few at the full rate. Finally for a few
> such as the RM52xx either half or the full count the rate is selectable by
> the reset initialization bitstream fed into the processor. Too make this
> feature suck nicely there is no way for software to find out which rate
> was selected so software must know that or calibrate against a timer of
> known frequency.
>
> Platform-specific code does this by setting mips_hpt_frequency to the
> count rate before calling init_r4k_clocksource; it's also the value being
> passed into setup_sched_clock_update() so you don't need to count for the
> half / full clock rate thing there.
>
> I don't see why you need the -2 in your formula so the whole thing can
> be simplified to:
>
> data = 0x80000000 / tclk * HZ;
>

Sorry, I have mixed the mips_hpt_frequency with the cpu frequency,
mips_hpt_frequency is exactly the frequency of the timer. so, there is
no need to consider the relation between it and the cpu frequency here.
therefore, my old formula should be okay, that -2 is used to ensure data
is smaller than half of the period of the timer.

Best Regards,
Wu Zhangjin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 22 Nov 2009 13:10

* Sergei Shtylyov <sshtylyov(a)ru.mvista.com> wrote:

> Hello.
>
> Ingo Molnar wrote:
>
> >>+config HR_SCHED_CLOCK
> >>+ bool "High Resolution sched_clock()"
> >>+ depends on CSRC_R4K
> >>+ default n
> >>+ help
> >>+ This option enables the MIPS c0 count based high resolution
> >>+ sched_clock().
> >>+
> >>+ If you need a ns precision timestamp, You are recommended to enable
> >>+ this option. For example, If you are using the Ftrace subsystem to do
>
> s/If/if/
>
> >>diff --git a/arch/mips/kernel/csrc-r4k.c b/arch/mips/kernel/csrc-r4k.c
> >>index e95a3cd..4e52cca 100644
> >>--- a/arch/mips/kernel/csrc-r4k.c
> >>+++ b/arch/mips/kernel/csrc-r4k.c
> >>@@ -6,10 +6,64 @@
> >> * Copyright (C) 2007 by Ralf Baechle
> >> */
> >> #include <linux/clocksource.h>
> >>+#include <linux/cnt32_to_63.h>
> >>+#include <linux/timer.h>
> >> #include <linux/init.h>
> >> #include <asm/time.h>
> >>+/*
> >>+ * MIPS' sched_clock implementation.
> >
> >s/MIPS'/MIPS's
>
> MIPS's is not really a proper English. :-)

AFAIK 'MIPS' is not the plural of 'MIP' (but an acronym ending with
'S'), hence the possessive form would be MIPS's.

It doesnt matter much i guess ;-)

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Ingo Molnar on 23 Nov 2009 03:50

* Ralf Baechle <ralf(a)linux-mips.org> wrote:

> On Sun, Nov 22, 2009 at 07:06:16PM +0100, Ingo Molnar wrote:
>
> > > MIPS's is not really a proper English. :-)
> >
> > AFAIK 'MIPS' is not the plural of 'MIP' (but an acronym ending with
> > 'S'), hence the possessive form would be MIPS's.
>
> MIPS Technologies' IP lawyers insist that MIPS is a proper name and
> not an acronym - this position has certain advantages in trademark
> law.

That too seems to support my point that "MIPS's" is the right spelling.

Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2
Prev: MIPS TSC synchronization
Next: [tip:x86/mm] x86, numa, bootmem: Only free bootmem on NUMA failure path