Message-ID: <88c8b17b1fe7db8215ec4a7aebc4893d8c15574b.camel@kernel.org>
Date: Wed, 11 Sep 2024 16:19:23 -0400
From: Jeff Layton <jlayton@...nel.org>
To: John Stultz <jstultz@...gle.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner
 <brauner@...nel.org>, Jan Kara <jack@...e.cz>, Thomas Gleixner
 <tglx@...utronix.de>, Stephen Boyd <sboyd@...nel.org>, Arnd Bergmann
 <arnd@...nel.org>, linux-fsdevel@...r.kernel.org,
 linux-kernel@...r.kernel.org,  kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH] timekeeping: move multigrain ctime floor handling into
 timekeeper

On Wed, 2024-09-11 at 12:55 -0700, John Stultz wrote:
> On Wed, Sep 11, 2024 at 5:57 AM Jeff Layton <jlayton@...nel.org> 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?
> 

Sure. This is the latest posting:

https://lore.kernel.org/linux-fsdevel/20240715-mgtime-v6-0-48e5d34bd2ba@kernel.org/

The patches are in the vfs.mgtime branch of Christian's public tree as
well.

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

I changed current_time() to call a new coarse_ctime() function. That
function just calls ktime_* functions, but it makes 2 trips through
seqcount loops. Each of those implies a smp_mb() call.

This patch gets that down to a single seqcount loop.

> > 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@...el.com>
> > Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@intel.com
> 
> Fixes: ?

Sure. But as I said, this is not in mainline yet:

    Fixes: a037d5e7f81b ("fs: add infrastructure for multigrain timestamps")

> 
> > Suggested-by: Arnd Bergmann <arnd@...nel.org>
> > Signed-off-by: Jeff Layton <jlayton@...nel.org>
> > ---
> > 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.
> 

I get your point, but I think it's unavoidable here, unfortunately.

> ...
> > 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?
> 

Sure. Will do.

> > +{
> > +       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?
> 

Bingo.

> 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.
> 

The floor value is updated in inode_set_ctime_current() in the
multigrain series. The comments over that hopefully describe how it
works, but basically, once we determine that we need a fine-grained
timestamp, we fetch a new fine-grained value and try to swap it into
ctime_floor. After that, we convert it to a realtime value and try to
swap the nsec field into the inode's ctime.

The conversion is a bit expensive, but the multigrain series takes
great pains to only update the ctime_floor as a last resort. It's a
global value, so we _really_ don't want to write to it any more than
necessary.

> 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).
> 

The floor is tracked as a ktime_t, as we need to be able to swap it
into place with a cmpxchg() operation. I did originally try to use
timespec64's for everything, but it was too hard to keep everything
consistent without resorting to locking.

That said, I'm open to suggestions to make this better. I did (briefly)
look at whether moving the floor tracking into the timekeeper wholesale
would be better, but it didn't seem to be.

Thanks for taking a look!
-- 
Jeff Layton <jlayton@...nel.org>

Powered by blists - more mailing lists