On Fri, 16 Aug 2019 11:55:14 PDT (-0700), Atish Patra wrote:
On Fri, 2019-08-16 at 17:09 +0200, Daniel Lezcano wrote:
On 31/07/2019 03:24, Atish Patra wrote:
> There is only one clocksource in RISC-V. The boot cpu initializes
> that clocksource. No need to keep a percpu data structure.
That is not what is stated in the initial patch [1].
Can you clarify that ?
I think what I meant to say was "There is only one clocksource used in
RISC-V Linux" as it is guranteed that all the timers across all the
harts are synchronized within one tick of each other [2].
Apologies for not being verbose here.
However, reading the privilege specification(1.12-draft)
Section. 3.1.10 states that
"Accurate real-time clocks (RTCs) are relatively expensive to provide
(requiring a crystal or MEMS oscillator) and have to run even when the
rest of system is powered down, and so there is usually only one in a
system located in a different frequency/voltage domain from the
processors. Hence, the RTC must be shared by all the harts in a system"
This is different from the commit text in [1].
Perhaps I misunderstood something. @Palmer ?
This is one of those places the ISA has drifted around a bit: in the user ISA
there is a time CSR, and CSRs are all per-hart state so logically there is a
timer per hart. We used to actually build systems this way (with an SOC agent
what would actively increment each CSR whenever the RTC fired), but it ended up
being impractical for a bunch of reasons. There was never a way to actually
write these time CSRs from supervisor mode, but machine-mode software could
write them so it would have been possible to build system that had different
time values on different harts.
As a result we ended up with per-CPU timers in Linux, but they never actually
worked correctly: there's a bunch of per-CPU state in the driver, but nothing
to actually enforce that timer reads go to the correct hart. For example, get
the time on hart 0 you'd have to IPI over to that hart, do a local CSR read,
and then IPI the time back. As a result the per-CPU state never really made
any sense, but it kind of just hung around because it worked fine on the
systems we were building (which always had time synced up anyway) and was
closer to what the spec allowed -- we didn't IPI over because time was always
synchronized on systems that actually existed and the IPIs are super slow, but
the scaffolding stuck around.
As part of cleaning up the privileged ISA for ratification we decided to
mandate that the time CSRs on every hart are always within a single tick of
each other, effectively mandating a single time across the system. This was
partially motivated by Linux, but mostly by a new approach we were taking to
the hypervisor specification -- rather than a hypervisor mode, we decided to
just extend supervisor mode to support fast nested virtualization, which means
we now have "htimedelta" (a per-hart timer offset) rather than per-hart timers.
This is more efficient because the per-state stays constant so we don't need to
actively tick it, and since it makes the per-hart time state unnecessary we
decided to drop that extra state.
The change to global time on RISC-V systems rendered the per-CPU timers
defunct, but since they weren't really doing anything they just stuck around.
The cleanup seems perfectly reasonable to me, modulo the issue I've pointed out
below...
[2]
https://elixir.bootlin.com/linux/v5.3-rc4/source/drivers/clocksource/timer-riscv.c#L44
Thanks
-- Daniel
[1] https://lkml.org/lkml/2018/8/4/51
> Signed-off-by: Atish Patra <atish.patra@xxxxxxx>
> ---
> drivers/clocksource/timer-riscv.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/clocksource/timer-riscv.c
> b/drivers/clocksource/timer-riscv.c
> index 5e6038fbf115..09e031176bc6 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -55,7 +55,7 @@ static u64 riscv_sched_clock(void)
> return get_cycles64();
> }
>
> -static DEFINE_PER_CPU(struct clocksource, riscv_clocksource) = {
> +static struct clocksource riscv_clocksource = {
> .name = "riscv_clocksource",
> .rating = 300,
> .mask = CLOCKSOURCE_MASK(64),
> @@ -92,7 +92,6 @@ void riscv_timer_interrupt(void)
> static int __init riscv_timer_init_dt(struct device_node *n)
> {
> int cpuid, hartid, error;
> - struct clocksource *cs;
>
> hartid = riscv_of_processor_hartid(n);
> if (hartid < 0) {
> @@ -112,8 +111,7 @@ static int __init riscv_timer_init_dt(struct
> device_node *n)
>
> pr_info("%s: Registering clocksource cpuid [%d] hartid [%d]\n",
> __func__, cpuid, hartid);
> - cs = per_cpu_ptr(&riscv_clocksource, cpuid);
> - error = clocksource_register_hz(cs, riscv_timebase);
> + error = clocksource_register_hz(&riscv_clocksource,
> riscv_timebase);
Someone's client has mangled the patches, but I think there's an issue here:
we're still calling the init code for every "riscv" DT entry, but there's now
only a single "struct clocksource". This will result in a single clocksource
being initialized multiple times, which I assume is an issue.
> if (error) {
> pr_err("RISCV timer register failed [%d] for cpu =
> [%d]\n",
> error, cpuid);
>
