Re: [GIT pull] timers fixes for 2.6.31

[Martin Schwidefsky <schwidefsky@de.ibm.com>]

On Thu, 6 Aug 2009 15:36:36 -0700
john stultz <johnstul@us.ibm.com> wrote:

> On Fri, Jul 31, 2009 at 5:37 AM, Ingo Molnar<mingo@elte.hu> wrote:
> > [ Markus, John: the fix looks right for .31 but i think there's two
> >  small structural problems with this code, which might have
> >  contributed to the bug to happen to begin with:
> >
> >  Firstly, ->mult_orig is a slight misnomer - if it was named
> >  properly we wouldnt even need the comments to explain how to use
> >  and update it. It's the unadjusted multiplicator while _orig
> >  patterns in the kernel generally suggest some sort of save/restore
> >  pattern (which this is not).
> >
> >  I'd suggest to rename it to ->mult_unadjusted, ->mult_raw or
> >  ->mult_static instead. This field has not gotten into many
> >  clocksource drivers yet so it's easy to do. For .32 obviously.
> >
> >  Secondly, the broader design question is: why are clocksource
> >  drivers mucking around with NTP details? Whether NTP is running
> >  should be a transparent detail to drivers and if such details are
> >  visible in low level driver (which they are in
> >  arch/arm/plat-omap/common.c et al) that's sign of uncleanliness.
> >  Mind improving that? (for .32 too)
> 
> Well, the clocksources aren't really mucking with NTP details, but NTP
> mucks with the clocksource.  And so this was a way to try to preserve
> the original mult value,  and I'll admit was a bit stapled on.  This
> mostly comes from how the clocksource structure has swelled over time
> to include quite a bit of timekeeping specific information.
> 
> Splitting this up has been on my list for awhile, but I've never
> gotten to it. However, Martin Schwidefsky has been doing a great job
> with his latest patches cleaning up some of uglier parts of the
> timekeeping/clocksource interactions. There is still lots of  changes
> I'd like to see, but they will take time as we still haven't gotten
> all the arches over to GENERIC_TIME, so some cruft has to stick
> around.   But Martin's patches are a great big step towards it and
> hopefully his patches will be ready for .32.

The question what we do with the mult_orig on clocksource
enable/disable is one of the remaining open issues with my patch set.
With the NTP adjusted multiplicator in the struct timekeeper we don't
have a convenient place to keep the NTP adjusted multiplier while a
clocksource is disabled. My initial thought was: why do we store the
old multiplier at all? Its not that the clocksource changes all the
time and NTP will catch up again if it is changed. Just start from
scratch with the initial multiplicator.

-- 
blue skies,
   Martin.

"Reality continues to ruin my life." - Calvin.