On Tue, Oct 01 2024 at 06:58, Jeff Layton wrote:
V8 ? I remember that I reviewed v* already :)
Also the sentence after the susbsystem prefix starts with an uppercase
letter.
> Multigrain timestamps allow the kernel to use fine-grained timestamps
> when an inode's attributes is being actively observed via ->getattr().
> With this support, it's possible for a file to get a fine-grained
> timestamp, and another modified after it to get a coarse-grained stamp
> that is earlier than the fine-grained time. If this happens then the
> files can appear to have been modified in reverse order, which breaks
> VFS ordering guarantees.
>
> To prevent this, maintain a floor value for multigrain timestamps.
> Whenever a fine-grained timestamp is handed out, record it, and when
> coarse-grained stamps are handed out, ensure they are not earlier than
> that value. If the coarse-grained timestamp is earlier than the
> fine-grained floor, return the floor value instead.
>
> Add a static singleton atomic64_t into timekeeper.c that we can use to
s/we can use/is used/
> keep track of the latest fine-grained time ever handed out. This is
> tracked as a monotonic ktime_t value to ensure that it isn't affected by
> clock jumps. Because it is updated at different times than the rest of
> the timekeeper object, the floor value is managed independently of the
> timekeeper via a cmpxchg() operation, and sits on its own cacheline.
>
> This patch also adds two new public interfaces:
git grep 'This patch' Docuementation/process/
> - ktime_get_coarse_real_ts64_mg() fills a timespec64 with the later of the
> coarse-grained clock and the floor time
>
> - ktime_get_real_ts64_mg() gets the fine-grained clock value, and tries
> to swap it into the floor. A timespec64 is filled with the result.
>
> Since the floor is global, take care to avoid updating it unless it's
> absolutely necessary. If we do the cmpxchg and find that the value has
We do nothing. Please describe your changes in passive voice and do not
impersonate code.
> been updated since we fetched it, then we discard the fine-grained time
> that was fetched in favor of the recent update.
This still is confusing. Something like this:
The floor value is global and updated via a single try_cmpxchg(). If
that fails then the operation raced with a concurrent update. It does
not matter whether the new value is later than the value, which the
failed cmpxchg() tried to write, or not. Add sensible technical
explanation.
> +/*
> + * Multigrain timestamps require that we keep track of the latest fine-grained
> + * timestamp that has been issued, and never return a coarse-grained timestamp
> + * that is earlier than that value.
> + *
> + * mg_floor represents the latest fine-grained time that we have handed out as
> + * a timestamp on the system. Tracked as a monotonic ktime_t, and converted to
> + * the realtime clock on an as-needed basis.
> + *
> + * This ensures that we never issue a timestamp earlier than one that has
> + * already been issued, as long as the realtime clock never experiences a
> + * backward clock jump. If the realtime clock is set to an earlier time, then
> + * realtime timestamps can appear to go backward.
> + */
> +static __cacheline_aligned_in_smp atomic64_t mg_floor;
> +
> static inline void tk_normalize_xtime(struct timekeeper *tk)
> {
> while (tk->tkr_mono.xtime_nsec >= ((u64)NSEC_PER_SEC << tk->tkr_mono.shift)) {
> @@ -2394,6 +2410,86 @@ void ktime_get_coarse_real_ts64(struct timespec64 *ts)
> }
> EXPORT_SYMBOL(ktime_get_coarse_real_ts64);
>
> +/**
> + * ktime_get_coarse_real_ts64_mg - return latter of coarse grained time or floor
> + * @ts: timespec64 to be filled
> + *
> + * Fetch the global mg_floor value, convert it to realtime and
> + * compare it to the current coarse-grained time. Fill @ts with
> + * whichever is latest. Note that this is a filesystem-specific
> + * interface and should be avoided outside of that context.
> + */
> +void ktime_get_coarse_real_ts64_mg(struct timespec64 *ts)
> +{
> + struct timekeeper *tk = &tk_core.timekeeper;
> + u64 floor = atomic64_read(&mg_floor);
> + ktime_t f_real, offset, coarse;
> + unsigned int seq;
> +
> + do {
> + seq = read_seqcount_begin(&tk_core.seq);
> + *ts = tk_xtime(tk);
> + offset = tk_core.timekeeper.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_mg);
> +
> +/**
> + * ktime_get_real_ts64_mg - attempt to update floor value and return result
> + * @ts: pointer to the timespec to be set
> + *
> + * Get a monotonic fine-grained time value and attempt to swap it into the
> + * floor. If it succeeds then accept the new floor value. If it fails
> + * then another task raced in during the interim time and updated the floor.
> + * That value is just as valid, so accept that value in this case.
Why? 'just as valid' does not tell me anything.
> + * @ts will be filled with the resulting floor value, regardless of
> + * the outcome of the swap. Note that this is a filesystem specific interface
> + * and should be avoided outside of that context.
> + */
> +void ktime_get_real_ts64_mg(struct timespec64 *ts)
> +{
> + struct timekeeper *tk = &tk_core.timekeeper;
> + ktime_t old = atomic64_read(&mg_floor);
> + ktime_t offset, mono;
> + unsigned int seq;
> + u64 nsecs;
> +
> + do {
> + seq = read_seqcount_begin(&tk_core.seq);
> +
> + ts->tv_sec = tk->xtime_sec;
> + mono = tk->tkr_mono.base;
> + nsecs = timekeeping_get_ns(&tk->tkr_mono);
> + offset = tk_core.timekeeper.offs_real;
> + } while (read_seqcount_retry(&tk_core.seq, seq));
> +
> + mono = ktime_add_ns(mono, nsecs);
> +
> + /*
> + * Attempt to update the floor with the new time value. Accept the
> + * resulting floor value regardless of the outcome of the swap.
> + */
> + if (atomic64_try_cmpxchg(&mg_floor, &old, mono)) {
> + ts->tv_nsec = 0;
> + timespec64_add_ns(ts, nsecs);
> + } else {
> + /*
> + * Something has changed mg_floor since "old" was
I complained about 'something has changed ...' before. Can we please
have proper technical explanations?
> + * fetched. "old" has now been updated with the
> + * current value of mg_floor, so use that to return
> + * the current coarse floor value.
This still does not explain why the new floor value is valid even when
it is before the value in @mono.
Thanks,
tglx
