Hi Thomas,

On Fri, Feb 08, 2019 at 08:28:07PM +0100, Thomas Gleixner wrote:

On Fri, 8 Feb 2019, Will Deacon wrote:

quoted

On Fri, Dec 07, 2018 at 05:53:21PM +0000, Will Deacon wrote:

quoted

Anyway, moving the counter read into the protected region is a little fiddly
because the memory barriers we have in there won't give us the ordering we
need. We'll instead need to do something nasty, like create a dependency
from the counter read to the read of the seqlock:

Maybe the untested crufty hack below, although this will be a nightmare to
implement in C.

We discussed this in person this week, but you couldn't recall the details
off the top of your head so I'm replying here. Please could you clarify what
your concern was with the existing code, and whether or not I've got the
wrong end of the stick?

If you just collect the variables under the seqcount protection and have
the readout of the timer outside of it then you are not guaranteeing
consistent state.

The problem is:

	do {
		seq = read_seqcount_begin(d->seq);
		last = d->cycle_last;
		mult = d->mult;
		shift = d->shift;
		ns = d->ns_base;
	while (read_seqcount_retry(d->seq, seq));

	ns += ((read_clock() - last) * mult);
	ns >>= shift;

So on the first glance this looks consistent because you collect all data
and then do the read and calc outside the loop.

But if 'd->mult' gets updated _before_ read_clock() then you can run into a
situation where time goes backwards with the next read.

Here is the flow you need for that:

t1 = gettime()
     {
	collect_data()

     ---> Interrupt, updates mult (mult becomes smaller)

     	  This can expand over a very long time when the task is scheduled
     	  out here and there are multiple updates in between. The farther
     	  out the read is delayed, the more likely the problem is going to
     	  observable.

     	read_clock_and_calc()
     }

t2 = gettime()
     {
	collect_data()
     	read_clock_and_calc()
     }

This second read uses updated data, i.e. the smaller mult. So depending on
the distance of the two reads and the difference of the mult, the caller
can observe t2 < t1, which is a NONO. You can observe it on a single
CPU. No virt, SMP, migration needed at all.

Thank you for this explanation, I agree that we have a bug here (and have
done since day 1 on arm64!). I've replied separately on ktime_get().

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel