fs/inode.c | 35 +++++++++-------------------------- include/linux/timekeeping.h | 2 ++ kernel/time/timekeeping.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 26 deletions(-)
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.
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@intel.com>
Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@intel.com
Suggested-by: Arnd Bergmann <arnd@kernel.org>
Signed-off-by: Jeff Layton <jlayton@kernel.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);
-}
-
/**
* current_time - Return FS time (possibly fine-grained)
* @inode: inode.
@@ -2285,10 +2266,10 @@ static ktime_t coarse_ctime(ktime_t floor)
struct timespec64 current_time(struct inode *inode)
{
ktime_t floor = atomic64_read(&ctime_floor);
- ktime_t now = coarse_ctime(floor);
- struct timespec64 now_ts = ktime_to_timespec64(now);
+ struct timespec64 now_ts;
u32 cns;
+ ktime_get_coarse_real_ts64_with_floor(&now_ts, floor);
if (!is_mgtime(inode))
goto out;
@@ -2745,7 +2726,7 @@ EXPORT_SYMBOL(timestamp_truncate);
*
* Set the inode's ctime to the current value for the inode. Returns the
* current value that was assigned. If this is not a multigrain inode, then we
- * just set it to whatever the coarse_ctime is.
+ * set it to the later of the coarse time and floor value.
*
* If it is multigrain, then we first see if the coarse-grained timestamp is
* distinct from what we have. If so, then we'll just use that. If we have to
@@ -2756,15 +2737,15 @@ EXPORT_SYMBOL(timestamp_truncate);
*/
struct timespec64 inode_set_ctime_current(struct inode *inode)
{
- ktime_t now, floor = atomic64_read(&ctime_floor);
+ ktime_t floor = atomic64_read(&ctime_floor);
struct timespec64 now_ts;
u32 cns, cur;
- now = coarse_ctime(floor);
+ ktime_get_coarse_real_ts64_with_floor(&now_ts, floor);
/* Just return that if this is not a multigrain fs */
if (!is_mgtime(inode)) {
- now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
+ now_ts = timestamp_truncate(now_ts, inode);
inode_set_ctime_to_ts(inode, now_ts);
goto out;
}
@@ -2777,6 +2758,7 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
cns = smp_load_acquire(&inode->i_ctime_nsec);
if (cns & I_CTIME_QUERIED) {
ktime_t ctime = ktime_set(inode->i_ctime_sec, cns & ~I_CTIME_QUERIED);
+ ktime_t now = timespec64_to_ktime(now_ts);
if (!ktime_after(now, ctime)) {
ktime_t old, fine;
@@ -2797,10 +2779,11 @@ struct timespec64 inode_set_ctime_current(struct inode *inode)
else
fine = old;
now = ktime_mono_to_real(fine);
+ now_ts = ktime_to_timespec64(now);
}
}
mgtime_counter_inc(mg_ctime_updates);
- now_ts = timestamp_truncate(ktime_to_timespec64(now), inode);
+ now_ts = timestamp_truncate(now_ts, inode);
cur = cns;
/* No need to cmpxchg if it's exactly the same */
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index fc12a9ba2c88..9b3c957ab260 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -44,6 +44,7 @@ extern void ktime_get_ts64(struct timespec64 *ts);
extern void ktime_get_real_ts64(struct timespec64 *tv);
extern void ktime_get_coarse_ts64(struct timespec64 *ts);
extern void ktime_get_coarse_real_ts64(struct timespec64 *ts);
+extern void ktime_get_coarse_real_ts64_with_floor(struct timespec64 *ts, ktime_t floor);
void getboottime64(struct timespec64 *ts);
@@ -68,6 +69,7 @@ enum tk_offsets {
extern ktime_t ktime_get(void);
extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
extern ktime_t ktime_get_coarse_with_offset(enum tk_offsets offs);
+extern ktime_t ktime_get_coarse_with_floor_and_offset(enum tk_offsets offs, ktime_t floor);
extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
extern ktime_t ktime_get_raw(void);
extern u32 ktime_get_resolution_ns(void);
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)
+{
+ 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);
+}
+EXPORT_SYMBOL_GPL(ktime_get_coarse_real_ts64_with_floor);
+
void ktime_get_coarse_ts64(struct timespec64 *ts)
{
struct timekeeper *tk = &tk_core.timekeeper;
---
base-commit: 962e66693d6214b1d48f32f68ed002170a98f2c0
change-id: 20240910-mgtime-e244049f2aea
Best regards,
--
Jeff Layton <jlayton@kernel.org>On Wed, Sep 11, 2024 at 08:56:56AM GMT, Jeff Layton 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. > > 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@intel.com> > Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@intel.com > Suggested-by: Arnd Bergmann <arnd@kernel.org> > Signed-off-by: Jeff Layton <jlayton@kernel.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. We will need this as otherwise we can't really merge the multigrain timestamp work with known performance regressions?
On Thu, 2024-09-12 at 14:31 +0200, Christian Brauner wrote: > On Wed, Sep 11, 2024 at 08:56:56AM GMT, Jeff Layton 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. > > > > 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@intel.com> > > Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@intel.com > > Suggested-by: Arnd Bergmann <arnd@kernel.org> > > Signed-off-by: Jeff Layton <jlayton@kernel.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. > > We will need this as otherwise we can't really merge the multigrain > timestamp work with known performance regressions? Yes, I think we'll need something here. Arnd suggested an alternative way to do this that might be even better. I'm not 100% sure that it'll work though since the approach is a bit different. I'd still like to see this go in for v6.12, so what I'd probably prefer is to take this patch initially (with the variable name change that John suggested), and then we can work on the alternative approach in the meantime Would that be acceptable? -- Jeff Layton <jlayton@kernel.org>
On Thu, Sep 12, 2024 at 08:39:32AM GMT, Jeff Layton wrote: > On Thu, 2024-09-12 at 14:31 +0200, Christian Brauner wrote: > > On Wed, Sep 11, 2024 at 08:56:56AM GMT, Jeff Layton 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. > > > > > > 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@intel.com> > > > Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@intel.com > > > Suggested-by: Arnd Bergmann <arnd@kernel.org> > > > Signed-off-by: Jeff Layton <jlayton@kernel.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. > > > > We will need this as otherwise we can't really merge the multigrain > > timestamp work with known performance regressions? > > Yes, I think we'll need something here. Arnd suggested an alternative > way to do this that might be even better. I'm not 100% sure that it'll > work though since the approach is a bit different. > > I'd still like to see this go in for v6.12, so what I'd probably prefer > is to take this patch initially (with the variable name change that > John suggested), and then we can work on the alternative approach in > the meantime > > Would that be acceptable? It would be ok with me but we should get a nodd from the time keeper folks.
On Wed, Sep 11, 2024 at 5:57 AM Jeff Layton <jlayton@kernel.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?
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@intel.com>
> Closes: https://lore.kernel.org/oe-lkp/202409091303.31b2b713-oliver.sang@intel.com
Fixes: ?
> Suggested-by: Arnd Bergmann <arnd@kernel.org>
> Signed-off-by: Jeff Layton <jlayton@kernel.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.
...
> 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
On Wed, 2024-09-11 at 12:55 -0700, John Stultz wrote:
> On Wed, Sep 11, 2024 at 5:57 AM Jeff Layton <jlayton@kernel.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@intel.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@kernel.org>
> > Signed-off-by: Jeff Layton <jlayton@kernel.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@kernel.org>
© 2016 - 2024 Red Hat, Inc.