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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4e4b1096b322f2998b5e59a861c5fdad410c108a.camel@kernel.org>
Date: Fri, 13 Sep 2024 15:01:18 -0400
From: Jeff Layton <jlayton@...nel.org>
To: John Stultz <jstultz@...gle.com>
Cc: Jan Kara <jack@...e.cz>, Alexander Viro <viro@...iv.linux.org.uk>, 
 Christian Brauner
	 <brauner@...nel.org>, Thomas Gleixner <tglx@...utronix.de>, Stephen Boyd
	 <sboyd@...nel.org>, Arnd Bergmann <arnd@...nel.org>, Vadim Fedorenko
	 <vadim.fedorenko@...ux.dev>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, kernel test robot <oliver.sang@...el.com>
Subject: Re: [PATCH v2] timekeeping: move multigrain timestamp floor
 handling into timekeeper

On Fri, 2024-09-13 at 11:43 -0700, John Stultz wrote:
> On Fri, Sep 13, 2024 at 5:01 AM Jeff Layton <jlayton@...nel.org> 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@...nel.org>