Date:	Fri, 13 Sep 2013 10:05:48 -0700
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Thomas Gleixner <tglx@...utronix.de>
CC:	Peter Zijlstra <peterz@...radead.org>,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	John Stultz <john.stultz@...aro.org>,
	Richard Cochran <richardcochran@...il.com>,
	Prarit Bhargava <prarit@...hat.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Ingo Molnar <mingo@...e.hu>, linux-kernel@...r.kernel.org,
	lttng-dev@...ts.lttng.org
Subject: Re: [RFC PATCH] Introduce timekeeper latch synchronization

On 13/09/13 09:13 AM, Thomas Gleixner wrote:
> On Thu, 12 Sep 2013, Mathieu Desnoyers wrote:
>> * Peter Zijlstra (peterz@...radead.org) wrote:
>> [...]
>>> Yep, that's good. I suppose if there's multiple use sites we can jump
>>> through another few hoops to get rid of the specific struct foo
>>> assumptions by storing sizeof() whatever we do use and playing pointer
>>> math games.
>>>
>>> But for now with the time stuff as only user this looks ok.
>> OK! Here is the full implementation of the idea against Linux
>> timekeeper, ntp, and PPS. It appears that ntp and PPS were relying on
>> the timekeeper seqlock too. And guess what, after booting my laptop with
>> this kernel there still no smoke coming out of it after a good 5 minutes
>> of testing. ;-)
>>
>> Comments are welcome.
> First of all this needs to be split into several patches.
How about:
- three patches refactoring data structures into objects (no
synchronization changes whatsoever). timekeeper, ntp and pps each done
in separate patches,
- one patch to introduce the new synchronization scheme along with the
usage site changes. This patch would include the removal of the
shadow_timekeeper variable, which is made pointless by the introduction
of this mixed-rcu-seqcount synchronization scheme.

is that enough, or you see a more fine-grained breakdown ?


>
>>  static void update_wall_time(void)
>  ...
>> -	write_seqcount_begin(&timekeeper_seq);
>>  	/* Update clock->cycle_last with the new value */
>>  	clock->cycle_last = tk->cycle_last;
>> -	/*
>> -	 * Update the real timekeeper.
>> -	 *
>> -	 * We could avoid this memcpy by switching pointers, but that
>> -	 * requires changes to all other timekeeper usage sites as
>> -	 * well, i.e. move the timekeeper pointer getter into the
>> -	 * spinlocked/seqcount protected sections. And we trade this
>> -	 * memcpy under the timekeeper_seq against one before we start
>> -	 * updating.
>> -	 */
>> -	memcpy(real_tk, tk, sizeof(*tk));
>> -	timekeeping_update(real_tk, action);
> So you're dropping the timekeeping_update() call here. I wonder how
> this works.

It doesn't :-)

There appears to be a couple of side-effects related to removal of the
call to timekeeping_update(). On the positive side, I now have much more
free time on my hands. However, people seem to complain that I arrive
late to meetings for some reasons ;-)

>
>> +	timekeeper_write_begin(&latch_timekeeper, &prev, &tk);
>> +	*tk = *prev;
> Do we really need to do the copy on all call sites, instead of doing
> it in timekeeper_write_begin() ?

Since we're indeed doing the copy on all call sites, I think it would be
good to move it into timekeeper_write_begin() as you suggest.

Initially, I kept the copy in the caller in case we could do without the
copy in some cases (in case the caller would overwrite the entire
structure without caring about prior content). But since we need to copy
timekeeper, ntp and pps structures, I doubt this is realistic. So let's
move this copy into timekeeper_write_begin().

Thanks !

Mathieu


>
> Thanks,
>
> 	tglx

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

Powered by blists - more mailing lists