Re: [PATCH v8 01/11] timekeeping: move multigrain timestamp floor handling into timekeeper

[Thomas Gleixner <tglx@linutronix.de>]

On Sat, Sep 14 2024 at 13:07, Jeff Layton wrote:
> For multigrain timestamps, we must keep track of the latest timestamp

What is a multgrain timestamp? Can you please describe the concept
behind it? I'm not going to chase random documentation or whatever
because change logs have to self contained.

And again 'we' do nothing. Describe the problem in technical terms and
do not impersonate code.

> To maximize the window of this occurring when multiple tasks are racing
> to update the floor, ktime_get_coarse_real_ts64_mg returns a cookie
> value that represents the state of the floor tracking word, and
> ktime_get_real_ts64_mg accepts a cookie value that it uses as the "old"
> value when calling cmpxchg().

Clearly:

> +void ktime_get_real_ts64_mg(struct timespec64 *ts, u64 cookie)

Can you please get your act together?

> +/**
> + * ktime_get_coarse_real_ts64_mg - get later of coarse grained time or floor
> + * @ts: timespec64 to be filled
> + *
> + * Adjust floor to realtime and compare it to the coarse time. Fill
> + * @ts with the latest one.

This explains nothing.

>      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;
> +
> +	WARN_ON(timekeeping_suspended);
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		*ts = tk_xtime(tk);
> +		offset = *offsets[TK_OFFS_REAL];

Why this indirection? What's wrong with using
tk_core.timekeeper.offs_real directly?

> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	coarse = timespec64_to_ktime(*ts);
> +	f_real = ktime_add(floor, offset);

How is any of this synchronized against concurrent updates of the floor
value or the offset? I'm failing to see anything which keeps this
consistent. If this is magically consistent then it wants a big fat
comment in the code which explains it.

> +void ktime_get_real_ts64_mg(struct timespec64 *ts, u64 cookie)

What is this cookie argument for and how does that match the
declaration?

> +extern void ktime_get_real_ts64_mg(struct timespec64 *ts);

This does not even build.

> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	ktime_t old = atomic64_read(&mg_floor);
> +	ktime_t offset, mono;
> +	unsigned int seq;
> +	u64 nsecs;
> +
> +	WARN_ON(timekeeping_suspended);

WARN_ON_ONCE() if at all.

> +	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 = *offsets[TK_OFFS_REAL];
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	mono = ktime_add_ns(mono, nsecs);
> +
> +	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
> +		 * fetched. "old" has now been updated with the
> +		 * current value of mg_floor, so use that to return
> +		 * the current coarse floor value.

'Something has changed' is a truly understandable technical
explanation.

I'm not going to accept this voodoo which makes everyone scratch his
head who wasn't involved in this.

Thanks,

        tglx