Re: [PATCH] timekeeping: move multigrain ctime floor handling into timekeeper

[John Stultz]

On Wed, Sep 11, 2024 at 5:57 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
>
> The kernel test robot reported a performance regression in some
> will-it-scale tests due to the multigrain timestamp patches. The data
> showed that coarse_ctime() was slowing down current_time(), which is
> called frequently in the I/O path.

Maybe add a link to/sha for multigrain timestamp patches?

It might be helpful as well to further explain the overhead you're
seeing in detail?

> Add ktime_get_coarse_real_ts64_with_floor(), which returns either the
> coarse time or the floor as a realtime value. This avoids some of the
> conversion overhead of coarse_ctime(), and recovers some of the
> performance in these tests.
>
> The will-it-scale pipe1_threads microbenchmark shows these averages on
> my test rig:
>
>         v6.11-rc7:                      83830660 (baseline)
>         v6.11-rc7 + mgtime series:      77631748 (93% of baseline)
>         v6.11-rc7 + mgtime + this:      81620228 (97% of baseline)
>
> Reported-by: kernel test robot <oliver.sang@xxxxxxxxx>
> Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@xxxxxxxxx

Fixes: ?

> Suggested-by: Arnd Bergmann <arnd@xxxxxxxxxx>
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
> Arnd suggested moving this into the timekeeper when reviewing an earlier
> version of this series, and that turns out to be better for performance.
>
> I'm not sure how this should go in (if acceptable). The multigrain
> timestamp patches that this would affect are in Christian's tree, so
> that may be best if the timekeeper maintainers are OK with this
> approach.
> ---
>  fs/inode.c                  | 35 +++++++++--------------------------
>  include/linux/timekeeping.h |  2 ++
>  kernel/time/timekeeping.c   | 29 +++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+), 26 deletions(-)
>
> diff --git a/fs/inode.c b/fs/inode.c
> index 01f7df1973bd..47679a054472 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2255,25 +2255,6 @@ int file_remove_privs(struct file *file)
>  }
>  EXPORT_SYMBOL(file_remove_privs);
>
> -/**
> - * coarse_ctime - return the current coarse-grained time
> - * @floor: current (monotonic) ctime_floor value
> - *
> - * Get the coarse-grained time, and then determine whether to
> - * return it or the current floor value. Returns the later of the
> - * floor and coarse grained timestamps, converted to realtime
> - * clock value.
> - */
> -static ktime_t coarse_ctime(ktime_t floor)
> -{
> -       ktime_t coarse = ktime_get_coarse();
> -
> -       /* If coarse time is already newer, return that */
> -       if (!ktime_after(floor, coarse))
> -               return ktime_get_coarse_real();
> -       return ktime_mono_to_real(floor);
> -}

I'm guessing this is part of the patch set being worked on, but this
is a very unintuitive function.

You give it a CLOCK_MONOTONIC floor value, but it returns
CLOCK_REALTIME based time?

It looks like it's asking to be misused.

...
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 5391e4167d60..56b979471c6a 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -2394,6 +2394,35 @@ void ktime_get_coarse_real_ts64(struct timespec64 *ts)
>  }
>  EXPORT_SYMBOL(ktime_get_coarse_real_ts64);
>
> +/**
> + * ktime_get_coarse_real_ts64_with_floor - get later of coarse grained time or floor
> + * @ts: timespec64 to be filled
> + * @floor: monotonic floor value
> + *
> + * Adjust @floor to realtime and compare that to the coarse time. Fill
> + * @ts with the later of the two.
> + */
> +void ktime_get_coarse_real_ts64_with_floor(struct timespec64 *ts, ktime_t floor)

Maybe name 'floor' 'mono_floor' so it's very clear?

> +{
> +       struct timekeeper *tk = &tk_core.timekeeper;
> +       unsigned int seq;
> +       ktime_t f_real, offset, coarse;
> +
> +       WARN_ON(timekeeping_suspended);
> +
> +       do {
> +               seq = read_seqcount_begin(&tk_core.seq);
> +               *ts = tk_xtime(tk);
> +               offset = *offsets[TK_OFFS_REAL];
> +       } while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +       coarse = timespec64_to_ktime(*ts);
> +       f_real = ktime_add(floor, offset);
> +       if (ktime_after(f_real, coarse))
> +               *ts = ktime_to_timespec64(f_real);


I am still very wary of the function taking a CLOCK_MONOTONIC
comparator and returning a REALTIME value.
But I think I understand why you might want it: You want a ratchet to
filter inconsistencies from mixing fine and coarse (which very quickly
return the time in the recent past) grained timestamps, but you want
to avoid having a one way ratchet getting stuck if settimeofday() get
called.
So you implemented the ratchet against CLOCK_MONOTONIC, so
settimeofday offsets are ignored.

Is that close?

My confusion comes from the fact it seems like that would mean you
have to do all your timestamping with CLOCK_MONOTONIC (so you have a
useful floor value that you're keeping), so I'm not sure I understand
the utility of returning CLOCK_REALTIME values. I guess I don't quite
see the logic where the floor value is updated here, so I'm guessing.

Further, while this change from the earlier method avoids having to
make two calls taking the timekeeping seqlock, this still is going
from timespec->ktime->timespec still seems a little less than optimal
if this is a performance hotpath (the coarse clocks are based on
CLOCK_REALTIME timespecs because that was the legacy hotpath being
optimized for, so if we have to internalize this odd-seeming reatime
against monotonic usage model, we probably should better optimise
through the stack there).

thanks
-john