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

[Jeff Layton]

On Fri, 2024-09-13 at 11:43 -0700, John Stultz wrote:
> 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?
> 

There's a patch in the larger set that adds some percpu counters to
count events. One of them was successful floor value swaps. I dropped
that particular counter from the v7 set, but we could resurrect it.


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

Fair points. I don't have any hard numbers around it. I'm mainly just
trying to do what I can to keep the floor swaps to an absolute minimum.
This is a global value after all so we really are better off avoiding
cache invalidations.

That said, passing the cookie like this would only open the window a
small amount. I can certainly drop that part of the interface. In the
big scheme of things I doubt it'll make much difference in performance,
and if it does we can always bring it back.

If that sounds OK, I'll send a v8 (after some testing). I have some
comment updates I'd like to add as well.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>