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

[John Stultz]

On Fri, Sep 13, 2024 at 5:01 AM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> On Fri, 2024-09-13 at 13:26 +0200, Jan Kara wrote:
> > So what would be the difference if we did instead:
> >
> >       old = atomic64_read(&mg_floor);
> >
> > and not bother with the cookie? AFAIU this could result in somewhat more
> > updates to mg_floor (the contention on the mg_floor cacheline would be the
> > same but there would be more invalidates of the cacheline). OTOH these
> > updates can happen only if max(current_coarse_time, mg_floor) ==
> > inode->i_ctime which is presumably rare? What is your concern that I'm
> > missing?
> >
>
> My main concern is the "somewhat more updates to mg_floor". mg_floor is
> a global variable, so one of my main goals is to minimize the updates
> to it. There is no correctness issue in doing what you're saying above
> (AFAICT anyway), but the window of time between when we fetch the
> current floor and try to do the swap will be smaller, and we'll end up
> doing more swaps as a result.

Would it be worth quantifying that cost?

> Do you have any objection to adding the cookie to this API?

My main concern is it is just a bit subtle. I found it hard to grok
(though I can be pretty dim sometimes, so maybe that doesn't count for
much :)
It seems if it were misused, the fine-grained accessor could
constantly return coarse grained results when called repeatedly with a
very stale cookie.

Further, the point about avoiding "too many" mg_floor writes is a
little fuzzy. It feels almost like folks would need to use the cookie
update as a tuning knob to balance the granularity of their timestamps
against the cost of the global mg_floor writes. So this probably needs
some clear comments to make it more obvious.

thanks
-john