[GIT pull] timers fixes for 2.6.31

[GIT pull] timers fixes for 2.6.31

[Thomas Gleixner]

Linus,

Please pull the latest timers-fixes-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus

Thanks,

	tglx

------------------>
Magnus Damm (1):
      clocksource: save mult_orig in clocksource_disable()


 include/linux/clocksource.h |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c56457c..aff9f01 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -293,7 +293,11 @@ static inline int clocksource_enable(struct clocksource *cs)
 	if (cs->enable)
 		ret = cs->enable(cs);
 
-	/* save mult_orig on enable */
+	/* The frequency may have changed while the clocksource
+	 * was disabled. If so the code in ->enable() must update
+	 * the mult value to reflect the new frequency. Make sure
+	 * mult_orig follows this change.
+	 */
 	cs->mult_orig = cs->mult;
 
 	return ret;
@@ -309,6 +313,12 @@ static inline int clocksource_enable(struct clocksource *cs)
  */
 static inline void clocksource_disable(struct clocksource *cs)
 {
+	/* Save mult_orig in mult so clocksource_enable() can
+	 * restore the value regardless if ->enable() updates
+	 * the value of mult or not.
+	 */
+	cs->mult = cs->mult_orig;
+
 	if (cs->disable)
 		cs->disable(cs);
 }

Re: [GIT pull] timers fixes for 2.6.31

[Linus Torvalds]

On Thu, 30 Jul 2009, Thomas Gleixner wrote:
> 
> Please pull the latest timers-fixes-for-linus git tree from:
> 
>    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus
> 
> Thanks,
> 
> 	tglx
> 
> ------------------>
> Magnus Damm (1):
>       clocksource: save mult_orig in clocksource_disable()
> 
>  include/linux/clocksource.h |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)

Hmm. You seem to have forgotten to push this out. I realize you're off to 
vacation, do you have time to check that?

		Linus

Re: [GIT pull] timers fixes for 2.6.31

[Peter Zijlstra]

On Thu, 2009-07-30 at 16:49 -0700, Linus Torvalds wrote:
> 
> On Thu, 30 Jul 2009, Thomas Gleixner wrote:
> > 
> > Please pull the latest timers-fixes-for-linus git tree from:
> > 
> >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> > ------------------>
> > Magnus Damm (1):
> >       clocksource: save mult_orig in clocksource_disable()
> > 
> >  include/linux/clocksource.h |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> Hmm. You seem to have forgotten to push this out. I realize you're off to 
> vacation, do you have time to check that?

I can't find this merge in Linus' tree as of this writing, so I'm
assuming tglx vanished. Peter can you put that commit in the mentioned
tip branch?

Alternatively, I can stick it in a git tree for Linus.

Re: [GIT pull] timers fixes for 2.6.31

[Ingo Molnar]

* Linus Torvalds <torvalds@linux-foundation.org> wrote:

> 
> 
> On Thu, 30 Jul 2009, Thomas Gleixner wrote:
> > 
> > Please pull the latest timers-fixes-for-linus git tree from:
> > 
> >    git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus
> > 
> > Thanks,
> > 
> > 	tglx
> > 
> > ------------------>
> > Magnus Damm (1):
> >       clocksource: save mult_orig in clocksource_disable()
> > 
> >  include/linux/clocksource.h |   12 +++++++++++-
> >  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> Hmm. You seem to have forgotten to push this out. I realize you're 
> off to vacation, do you have time to check that?

I've pushed out the fix. I did two minor (non-functional) edits to 
the commit: fixed the comment style to match that of the rest of 
clocksource.h and improved the commit log a tiny bit. Find the 
updated pull request below.

[ 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)

  Also, a commit message quality nit: the commit description lacks 
  details about precisely what bug was hit in practice. While this 
  fix is eligible even without that info, it's not good to omit it. 
]

Linus, please pull the latest timers-fixes-for-linus git tree from:

   git://git.kernel.org/pub/scm/linux/kernel/git/tip/linux-2.6-tip.git timers-fixes-for-linus

 Thanks,

	Ingo

------------------>
Magnus Damm (1):
      clocksource: Save mult_orig in clocksource_disable()


 include/linux/clocksource.h |   14 +++++++++++++-
 1 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index c56457c..1219be4 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -293,7 +293,12 @@ static inline int clocksource_enable(struct clocksource *cs)
 	if (cs->enable)
 		ret = cs->enable(cs);
 
-	/* save mult_orig on enable */
+	/*
+	 * The frequency may have changed while the clocksource
+	 * was disabled. If so the code in ->enable() must update
+	 * the mult value to reflect the new frequency. Make sure
+	 * mult_orig follows this change.
+	 */
 	cs->mult_orig = cs->mult;
 
 	return ret;
@@ -309,6 +314,13 @@ static inline int clocksource_enable(struct clocksource *cs)
  */
 static inline void clocksource_disable(struct clocksource *cs)
 {
+	/*
+	 * Save mult_orig in mult so clocksource_enable() can
+	 * restore the value regardless if ->enable() updates
+	 * the value of mult or not.
+	 */
+	cs->mult = cs->mult_orig;
+
 	if (cs->disable)
 		cs->disable(cs);
 }

Re: [GIT pull] timers fixes for 2.6.31

[H. Peter Anvin]

On 07/31/2009 05:37 AM, Ingo Molnar wrote:
> 
> I've pushed out the fix. I did two minor (non-functional) edits to 
> the commit: fixed the comment style to match that of the rest of 
> clocksource.h and improved the commit log a tiny bit. Find the 
> updated pull request below.
> 

OK, won't worry about it, then.

	-hpa

-- 
H. Peter Anvin, Intel Open Source Technology Center
I work for Intel.  I don't speak on their behalf.

Re: [GIT pull] timers fixes for 2.6.31

[john stultz]

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.

thanks
-john

Re: [GIT pull] timers fixes for 2.6.31

[Martin Schwidefsky]

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.

Re: [GIT pull] timers fixes for 2.6.31

[john stultz]

On Fri, 2009-08-07 at 10:18 +0200, Martin Schwidefsky wrote:
> On Thu, 6 Aug 2009 15:36:36 -0700
> john stultz <johnstul@us.ibm.com> wrote:
>  
> > 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.

Indeed, we don't need to store the adjusted multiplier while a
clocksource is not in use. We just have to make sure every time it is
used, we start with the same original multiplier, so we have a constant
base from which the NTP freq adjustment can be made upon.

I think you're approach (keeping the clocksource mult untouched, and
keeping the adjusted mult in the timekeeper structure)is fine.

thanks
-john