Re: [RFC PATCH Mach] Add a mach host operation which returns elapsed time since bootup

[Zhaoming Luo]

Thank you for the review.

On 12/18/24 10:02 AM, Diego Nieto Cid wrote:
> El mar, 17 dic 2024 a las 22:21, Zhaoming Luo (<zhmingluo@163.com>) escribió:
> > The precision of this implmentation is 10ms. Not sure how to do with the
> > possible data race.
> >
> > Signed-off-by: Zhaoming Luo <zhmingluo@163.com>
> > ---
> >   include/mach/mach_host.defs |  7 +++++++
> >   kern/mach_clock.c           | 20 ++++++++++++++++++++
> >   2 files changed, 27 insertions(+)
> >
> > diff --git a/include/mach/mach_host.defs b/include/mach/mach_host.defs
> > index 8fd9d6b3..67f72cda 100644
> > --- a/include/mach/mach_host.defs
> > +++ b/include/mach/mach_host.defs
> > @@ -386,3 +386,10 @@ routine    host_adjust_time64(
> >   routine        host_get_kernel_version(
> >                  host            : host_t;
> >          out     kernel_version  : new_kernel_version_t);
> > +
> > +/*
> > + *     Get the elapsed time on this host.
> > + */
> > +routine        host_get_elapsed_time(
> > +               host            : host_t;
> > +       out     elapsed_time    : time_value_t);
>
> If I'm not mistaken, this is most commonly called `uptime` (like in
> `host_get_uptime` or similar)
OK
> > +
> > +       read_elapsed_ticks = elapsed_ticks;
> > +       elapsed_usec = read_elapsed_ticks * tick;
> > +       elapsed_time->seconds = elapsed_usec / MICROSECONDS_IN_ONE_SECOND;
> > +       elapsed_time->microseconds = elapsed_usec % MICROSECONDS_IN_ONE_SECOND;
>
> `read_elapsed_ticks` is only used to calculate elapsed_usec and thus
> may be inlined, removing the otherwise unused variable
Do you mean something like the following?
```
elapsed_usec = elapsed_ticks * tick;
elapsed_time->seconds = elapsed_usec / MICROSECONDS_IN_ONE_SECOND;
elapsed_time->microseconds = elapsed_usec % MICROSECONDS_IN_ONE_SECOND;
```
`read_elapsed_ticks` is read first because I hope it can reduce the 
possibility of data races. The `elapsed_tick` may be incremented in the 
timer interrupt[0].
> As for the data races, I believe you can use atomic builtins[1], like below:
>
>      elapsed_usec = __sync_fetch_and_add(&elapsed_tikcs, 0) * tick
>
> Or you could also use the lock used to increment `elapsed_ticks`:
>
>       s = simple_lock_irq(&timer_lock);
>      elapsed_usec = elapsed_tikcs * tick;
>      simple_unlock_irq(s, &timer_lock);
>
> I'm not sure which works best.
I don't think the latter will work. The timer interrupt also calls 
simple_lock_irq()[2]. It may cause deadlock.
> Cheers
>
> ---
> [1] https://gcc.gnu.org/onlinedocs/gcc-4.1.2/gcc/Atomic-Builtins.html

[0]:https://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/kern/
mach_clock.c#n221
[2]:https://git.savannah.gnu.org/cgit/hurd/gnumach.git/tree/kern/mach_clock.c#n219
--
Zhaoming Luo