linux-kernel - Re: [RFC][PATCH 1/2] time: Add ktime_get_mono_raw_fast

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: <20150220201137.GE23367@worktop.ger.corp.intel.com>
Date:	Fri, 20 Feb 2015 21:11:37 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	John Stultz <john.stultz@...aro.org>
Cc:	Paweł Moll <pawel.moll@....com>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Ingo Molnar <mingo@...nel.org>,
	Stephane Eranian <eranian@...gle.com>,
	lkml <linux-kernel@...r.kernel.org>,
	Arnaldo Carvalho de Melo <acme@...nel.org>,
	David Ahern <dsahern@...il.com>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Jiri Olsa <jolsa@...hat.com>,
	Namhyung Kim <namhyung@...il.com>,
	Paul Mackerras <paulus@...ba.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Steven Rostedt <rostedt@...dmis.org>,
	Sonny Rao <sonnyrao@...omium.org>,
	"ak@...ux.intel.com" <ak@...ux.intel.com>, vincent.weaver@...ne.edu
Subject: Re: [RFC][PATCH 1/2] time: Add ktime_get_mono_raw_fast_ns()

On Fri, Feb 20, 2015 at 11:49:49AM -0800, John Stultz wrote:
> On Fri, Feb 20, 2015 at 6:29 AM, Peter Zijlstra <peterz@...radead.org> wrote:
> > +u64 notrace ktime_get_mono_raw_fast_ns(void)
> > +{
> > +       struct tk_read_base *tkr;
> > +       unsigned int seq;
> > +       u64 now;
> > +
> > +       do {
> > +               seq = raw_read_seqcount(&tk_fast_mono_raw.seq);
> > +               tkr = tk_fast_mono_raw.base + (seq & 0x01);
> > +               now = ktime_to_ns(tkr->base_mono) + timekeeping_get_ns(tkr);
> 
> 
> So this doesn't look right. I think you want to use tk->base_raw and
> timekeeping_get_ns_raw() here?

No, the problem is that timekeeping_get_ns_raw() dereferences
tkr->clock. The idea was to, like ktime_get_mono_fast_ns() only touch
the _1_ cacheline.

Clearly I've messed it up, but I didn't want it to go dereference
tkr->clock and pull all kinds of stuff from that second line.

> > +       tkr = tk->tkr;
> > +       tkr.mult  = tk->tkr.clock->mult;
> > +       tkr.shift = tk->tkr.clock->shift;
> > +       update_fast_timekeeper(&tk_fast_mono_raw, &tkr);
> 
> So this is sort of sneaky and subtle here, which will surely cause
> problems later on. You're copying the original mult/shift pair into a
> copy of the tkr, so you get similar results from timekeeping_get_ns()
> as you'd want from timekeeping_get_ns_raw().  This results in multiple
> ways of getting the raw clock.
> 
> I think it would be better to either add a new tkr structure for the
> raw clock in the timekeeper, so you can directly copy it over,

OK, this then.

> or
> extend the tkr structure so it can contain the raw values as well.

Can't it would push tk_fast over the _1_ cacheline.

> Also, there's no real reason to have fast/non-fast versions of the raw
> clock. Since it isn't affected by frequency changes, it can't have the
> inconsistency issues the monotonic clock can see (which are documented
> in the comment near ktime_get_mono_fast_ns()). So we can probably
> condense these and avoid duplicative code.

The typical timekeeping_get_ns_raw() still got a seqcount around it
which can fail from NMI (inf loop for all).

So we do nee the tk_fast dual copy seqcount thing just the same, also as
per the above, cacheline cacheline cacheline :)

But yes, I think you're right in that we should be able to condense that
somewhat.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/