[PATCH] clocksource: shift helper

[PATCH] clocksource: shift helper

[Daniel Walker]

This is a little helper I pulled from the mips tree. I've seen a couple 
of bogus shift values, and this helper calculates the shift. This
should move the shift selection away from the user, and systematically
pick the value base on the hz.

I also modified the acpi_pm timer to use this new interface. The original
shift was 22 , and after this patch it's increased to 23. Which should make
the clocksource slightly more accurate, but shouldn't have much of a visible
effect.

Signed-off-by: Daniel Walker <dwalker@mvista.com>

---
 drivers/clocksource/acpi_pm.c |    5 ++++-
 include/linux/acpi_pmtmr.h    |    5 +++--
 include/linux/clocksource.h   |   21 +++++++++++++++++++++
 3 files changed, 28 insertions(+), 3 deletions(-)

Index: linux-2.6.25/drivers/clocksource/acpi_pm.c
===================================================================
--- linux-2.6.25.orig/drivers/clocksource/acpi_pm.c
+++ linux-2.6.25/drivers/clocksource/acpi_pm.c
@@ -72,7 +72,7 @@ static struct clocksource clocksource_ac
 	.read		= acpi_pm_read,
 	.mask		= (cycle_t)ACPI_PM_MASK,
 	.mult		= 0, /*to be calculated*/
-	.shift		= 22,
+	.shift		= 0,
 	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
 
 };
@@ -183,6 +183,9 @@ static int __init init_acpi_pm_clocksour
 	if (!pmtmr_ioport)
 		return -ENODEV;
 
+	clocksource_acpi_pm.shift =
+		clocksource_hz2shift(ACPI_PM_BITS, PMTMR_TICKS_PER_SEC);
+
 	clocksource_acpi_pm.mult = clocksource_hz2mult(PMTMR_TICKS_PER_SEC,
 						clocksource_acpi_pm.shift);
 
Index: linux-2.6.25/include/linux/acpi_pmtmr.h
===================================================================
--- linux-2.6.25.orig/include/linux/acpi_pmtmr.h
+++ linux-2.6.25/include/linux/acpi_pmtmr.h
@@ -7,10 +7,11 @@
 #define PMTMR_TICKS_PER_SEC 3579545
 
 /* limit it to 24 bits */
-#define ACPI_PM_MASK CLOCKSOURCE_MASK(24)
+#define ACPI_PM_BITS	24
+#define ACPI_PM_MASK CLOCKSOURCE_MASK(ACPI_PM_BITS)
 
 /* Overrun value */
-#define ACPI_PM_OVRRUN	(1<<24)
+#define ACPI_PM_OVRRUN	(1 << ACPI_PM_BITS)
 
 #ifdef CONFIG_X86_PM_TIMER
 
Index: linux-2.6.25/include/linux/clocksource.h
===================================================================
--- linux-2.6.25.orig/include/linux/clocksource.h
+++ linux-2.6.25/include/linux/clocksource.h
@@ -157,6 +157,27 @@ static inline u32 clocksource_hz2mult(u3
 }
 
 /**
+ * clocksource_hz2shift - calculates shift from hz and # of bits
+ * @bits:		Number of bits this clocksource uses
+ * @hz:			Clocksource frequency in Hz
+ *
+ * Helper functions that calculates the best shift value
+ * based on the hz and # of bits of any given clock.
+ */
+static inline u32 clocksource_hz2shift(u32 bits, u32 hz)
+{
+	u64 temp;
+
+	for (; bits > 0; bits--) {
+		temp = (u64) NSEC_PER_SEC << bits;
+		do_div(temp, hz);
+		if ((temp >> 32) == 0)
+			break;
+	}
+	return bits;
+}
+
+/**
  * clocksource_read: - Access the clocksource's current cycle value
  * @cs:		pointer to clocksource being read
  *
-- 

-- 

Re: [PATCH] clocksource: shift helper

[Andrew Morton]

On Thu, 01 May 2008 10:31:23 -0700
Daniel Walker <dwalker@mvista.com> wrote:

> This is a little helper I pulled from the mips tree. I've seen a couple 
> of bogus shift values, and this helper calculates the shift. This
> should move the shift selection away from the user, and systematically
> pick the value base on the hz.
> 
> I also modified the acpi_pm timer to use this new interface. The original
> shift was 22 , and after this patch it's increased to 23. Which should make
> the clocksource slightly more accurate, but shouldn't have much of a visible
> effect.
> 
> ...
>
> ===================================================================
> --- linux-2.6.25.orig/drivers/clocksource/acpi_pm.c
> +++ linux-2.6.25/drivers/clocksource/acpi_pm.c
> @@ -72,7 +72,7 @@ static struct clocksource clocksource_ac
>  	.read		= acpi_pm_read,
>  	.mask		= (cycle_t)ACPI_PM_MASK,
>  	.mult		= 0, /*to be calculated*/
> -	.shift		= 22,
> +	.shift		= 0,
>  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
>  
>  };
> @@ -183,6 +183,9 @@ static int __init init_acpi_pm_clocksour
>  	if (!pmtmr_ioport)
>  		return -ENODEV;
>  
> +	clocksource_acpi_pm.shift =
> +		clocksource_hz2shift(ACPI_PM_BITS, PMTMR_TICKS_PER_SEC);
> +
>  	clocksource_acpi_pm.mult = clocksource_hz2mult(PMTMR_TICKS_PER_SEC,
>  						clocksource_acpi_pm.shift);
>  
> Index: linux-2.6.25/include/linux/acpi_pmtmr.h
> ===================================================================
> --- linux-2.6.25.orig/include/linux/acpi_pmtmr.h
> +++ linux-2.6.25/include/linux/acpi_pmtmr.h
> @@ -7,10 +7,11 @@
>  #define PMTMR_TICKS_PER_SEC 3579545
>  
>  /* limit it to 24 bits */
> -#define ACPI_PM_MASK CLOCKSOURCE_MASK(24)
> +#define ACPI_PM_BITS	24
> +#define ACPI_PM_MASK CLOCKSOURCE_MASK(ACPI_PM_BITS)
>  
>  /* Overrun value */
> -#define ACPI_PM_OVRRUN	(1<<24)
> +#define ACPI_PM_OVRRUN	(1 << ACPI_PM_BITS)
>  
>  #ifdef CONFIG_X86_PM_TIMER
>  
> Index: linux-2.6.25/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6.25.orig/include/linux/clocksource.h
> +++ linux-2.6.25/include/linux/clocksource.h
> @@ -157,6 +157,27 @@ static inline u32 clocksource_hz2mult(u3
>  }
>  
>  /**
> + * clocksource_hz2shift - calculates shift from hz and # of bits
> + * @bits:		Number of bits this clocksource uses
> + * @hz:			Clocksource frequency in Hz
> + *
> + * Helper functions that calculates the best shift value
> + * based on the hz and # of bits of any given clock.
> + */
> +static inline u32 clocksource_hz2shift(u32 bits, u32 hz)
> +{
> +	u64 temp;
> +
> +	for (; bits > 0; bits--) {
> +		temp = (u64) NSEC_PER_SEC << bits;
> +		do_div(temp, hz);
> +		if ((temp >> 32) == 0)
> +			break;
> +	}
> +	return bits;
> +}

If we expect this to have more than one callsite then it would be best to
uninline it.

Unless we always expect it to be called from __init code, in which case
it's best to inline it ;)

Re: [PATCH] clocksource: shift helper

[Daniel Walker]

On Thu, 2008-05-01 at 12:32 -0700, Andrew Morton wrote:

> > +static inline u32 clocksource_hz2shift(u32 bits, u32 hz)
> > +{
> > +	u64 temp;
> > +
> > +	for (; bits > 0; bits--) {
> > +		temp = (u64) NSEC_PER_SEC << bits;
> > +		do_div(temp, hz);
> > +		if ((temp >> 32) == 0)
> > +			break;
> > +	}
> > +	return bits;
> > +}
> 
> If we expect this to have more than one callsite then it would be best to
> uninline it.
> 
> Unless we always expect it to be called from __init code, in which case
> it's best to inline it ;)

I expect it would always get called from __init flagged functions. The
clocksource can't get registed/used with out the shift and mult values. 

If we did uninline this one, we would have to do the other helpers too.
I imagine the use case is the same for all of them ..

Daniel

Re: [PATCH] clocksource: shift helper

[Ralf Baechle]

On Thu, May 01, 2008 at 12:48:25PM -0700, Daniel Walker wrote:

> > If we expect this to have more than one callsite then it would be best to
> > uninline it.
> > 
> > Unless we always expect it to be called from __init code, in which case
> > it's best to inline it ;)
> 
> I expect it would always get called from __init flagged functions. The
> clocksource can't get registed/used with out the shift and mult values. 
> 
> If we did uninline this one, we would have to do the other helpers too.
> I imagine the use case is the same for all of them ..

I could imagine this being called for hotpluggable CPUs or nodes, so rather
__cpuinit than __init.

  Ralf

Re: [PATCH] clocksource: shift helper

[Thomas Gleixner]

On Thu, 1 May 2008, Daniel Walker wrote:

> This is a little helper I pulled from the mips tree.
... 
> Signed-off-by: Daniel Walker <dwalker@mvista.com>

And who is the original author of this little helper in the MIPS tree?

Thanks,

	tglx

Re: [PATCH] clocksource: shift helper

[Daniel Walker]

On Tue, 2008-05-06 at 02:52 +0200, Thomas Gleixner wrote:
> On Thu, 1 May 2008, Daniel Walker wrote:
> 
> > This is a little helper I pulled from the mips tree.
> ... 
> > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> 
> And who is the original author of this little helper in the MIPS tree?

commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
Author: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

Daniel

Re: [PATCH] clocksource: shift helper

[Thomas Gleixner]

On Tue, 6 May 2008, Daniel Walker wrote:
> On Tue, 2008-05-06 at 02:52 +0200, Thomas Gleixner wrote:
> > On Thu, 1 May 2008, Daniel Walker wrote:
> > 
> > > This is a little helper I pulled from the mips tree.
> > ... 
> > > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> > 
> > And who is the original author of this little helper in the MIPS tree?
> 
> commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
> Author: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

Wouldn't it be appropriate to assign the authorship in the usual form
i.e.:

From: Atsushi Nemoto <anemo@mba.ocn.ne.jp> 

on top of the patch and keep the original changelog message intact
including the Signed-off-by of the author and Ralf ?

Aside of the broken S-O-B chain this is a blatant usurpation of
somebody else's work.

You answered my question about the real author without hesitation, so
I guess it was not your intention to claim authorship of someone
else's work, but this is on the verge of plagiarism and might bring
you into serious trouble.

You should know the proper procedures by now and this is exactly the
kind of sloppiness that you constantly show and what makes it almost
impossible to do any cooperative work with you.

Thanks,

	tglx

Re: [PATCH] clocksource: shift helper

[Daniel Walker]

On Tue, 2008-05-06 at 22:25 +0200, Thomas Gleixner wrote:
> On Tue, 6 May 2008, Daniel Walker wrote:
> > On Tue, 2008-05-06 at 02:52 +0200, Thomas Gleixner wrote:
> > > On Thu, 1 May 2008, Daniel Walker wrote:
> > > 
> > > > This is a little helper I pulled from the mips tree.
> > > ... 
> > > > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> > > 
> > > And who is the original author of this little helper in the MIPS tree?
> > 
> > commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
> > Author: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> 
> Wouldn't it be appropriate to assign the authorship in the usual form
> i.e.:
> 
> From: Atsushi Nemoto <anemo@mba.ocn.ne.jp> 
> 
> on top of the patch and keep the original changelog message intact
> including the Signed-off-by of the author and Ralf ?
> 
> Aside of the broken S-O-B chain this is a blatant usurpation of
> somebody else's work.
> 

I took a function and modified it for my usage, which happens all the
time.

I'd be fine if Atsushi wants to sign off on the patch.

Daniel

Re: [PATCH] clocksource: shift helper

[Thomas Gleixner]

On Tue, 6 May 2008, Daniel Walker wrote:
> On Tue, 2008-05-06 at 22:25 +0200, Thomas Gleixner wrote:
> > On Tue, 6 May 2008, Daniel Walker wrote:
> > > On Tue, 2008-05-06 at 02:52 +0200, Thomas Gleixner wrote:
> > > > On Thu, 1 May 2008, Daniel Walker wrote:
> > > > 
> > > > > This is a little helper I pulled from the mips tree.
> > > > ... 
> > > > > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> > > > 
> > > > And who is the original author of this little helper in the MIPS tree?
> > > 
> > > commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
> > > Author: Atsushi Nemoto <anemo@mba.ocn.ne.jp>
> > 
> > Wouldn't it be appropriate to assign the authorship in the usual form
> > i.e.:
> > 
> > From: Atsushi Nemoto <anemo@mba.ocn.ne.jp> 
> > 
> > on top of the patch and keep the original changelog message intact
> > including the Signed-off-by of the author and Ralf ?
> > 
> > Aside of the broken S-O-B chain this is a blatant usurpation of
> > somebody else's work.
> > 
> 
> I took a function and modified it for my usage, which happens all the
> time.

And your description led me to the assumption that the code was pulled
from the MIPS git tree and not from the MIPS architecture code in
mainline. Everyone else I talked to was having the same impression.

I was wrong, but can you understand that your sloppy wording "pulled
from the MIPS tree" along with the fact that you just copied code
instead of moving it into the generic space and cleanup the MIPS code
where it is duplicated from is causing such a misunderstanding ?

I'm fine with the change itself, but it needs to move the code out of
MIPS in the first place and not just duplicating code for no good
reason.

Thanks,
	tglx

Re: [PATCH] clocksource: shift helper

[Daniel Walker]

On Tue, 2008-05-06 at 22:53 +0200, Thomas Gleixner wrote:
> 
> And your description led me to the assumption that the code was pulled
> from the MIPS git tree and not from the MIPS architecture code in
> mainline. Everyone else I talked to was having the same impression.
> 
> I was wrong, but can you understand that your sloppy wording "pulled
> from the MIPS tree" along with the fact that you just copied code
> instead of moving it into the generic space and cleanup the MIPS code
> where it is duplicated from is causing such a misunderstanding ?

You could have asked me if it was mainline or not. I can see how words
could be misunderstood .

> I'm fine with the change itself, but it needs to move the code out of
> MIPS in the first place and not just duplicating code for no good
> reason.

I can work with Atsushi, but I still think the generic version should be
merged. We at least would need it in order to modify the mips code.

Daniel

Re: [PATCH] clocksource: shift helper

[Andrew Morton]

On Tue, 06 May 2008 14:01:15 -0700
Daniel Walker <dwalker@mvista.com> wrote:

> your sloppy wording "pulled from the MIPS tree"

Let it just be said: the amount of time which is lost due to poor patch
descriptions exceeds the amount of time which was saved in not
bothering to write good patch descriptions.

That it all.

Re: [PATCH] clocksource: shift helper

[Thomas Gleixner]

On Tue, 6 May 2008, Andrew Morton wrote:
> On Tue, 06 May 2008 14:01:15 -0700
> Daniel Walker <dwalker@mvista.com> wrote:
> 
> > your sloppy wording "pulled from the MIPS tree"
> 
> Let it just be said: the amount of time which is lost due to poor patch
> descriptions exceeds the amount of time which was saved in not
> bothering to write good patch descriptions.
> 
> That it all.

Amen to that.
 

Re: [PATCH] clocksource: shift helper

[Thomas Gleixner]

On Tue, 6 May 2008, Daniel Walker wrote:
> On Tue, 2008-05-06 at 22:53 +0200, Thomas Gleixner wrote:
> > 
> > And your description led me to the assumption that the code was pulled
> > from the MIPS git tree and not from the MIPS architecture code in
> > mainline. Everyone else I talked to was having the same impression.
> > 
> > I was wrong, but can you understand that your sloppy wording "pulled
> > from the MIPS tree" along with the fact that you just copied code
> > instead of moving it into the generic space and cleanup the MIPS code
> > where it is duplicated from is causing such a misunderstanding ?
> 
> You could have asked me if it was mainline or not. I can see how words
> could be misunderstood .

You - the patch submitter - have to provide useful information to
those who review the patches and not the reviewers are supposed to
decode your sloppy commit log.

"... which I pulled from the MIPS tree"

Why should I assume that this is from mainline ?

> > I'm fine with the change itself, but it needs to move the code out of
> > MIPS in the first place and not just duplicating code for no good
> > reason.
> 
> I can work with Atsushi, but I still think the generic version should be
> merged. We at least would need it in order to modify the mips code.

You can think what you want, it's not the way it works.

You duplicate code, so the first thing to do is to replace the code
which you copied and make sure it is still fully functional. Then you
can add a second user to make the point that the generic code move is
actually useful. btw, pmtimer is the worst example for a good use case
as everything there is constant so we can do it at compile time.

Thanks,
	tglx

Re: [PATCH] clocksource: shift helper

[Daniel Walker]

On Tue, 2008-05-06 at 23:19 +0200, Thomas Gleixner wrote:

> You - the patch submitter - have to provide useful information to
> those who review the patches and not the reviewers are supposed to
> decode your sloppy commit log.
> 
> "... which I pulled from the MIPS tree"
> 
> Why should I assume that this is from mainline ?

I'm not going to get into a long argument over this..

> > > I'm fine with the change itself, but it needs to move the code out of
> > > MIPS in the first place and not just duplicating code for no good
> > > reason.
> > 
> > I can work with Atsushi, but I still think the generic version should be
> > merged. We at least would need it in order to modify the mips code.
> 
> You can think what you want, it's not the way it works.
> 
> You duplicate code, so the first thing to do is to replace the code
> which you copied and make sure it is still fully functional. Then you
> can add a second user to make the point that the generic code move is
> actually useful. btw, pmtimer is the worst example for a good use case
> as everything there is constant so we can do it at compile time.

I can provide a patch that replaces the mips code instead of acpi_pm ..
Is that what your asking for?

Daniel

Re: [PATCH] clocksource: shift helper

[Atsushi Nemoto]

On Tue, 06 May 2008 09:39:14 -0700, Daniel Walker <dwalker@mvista.com> wrote:
> > > This is a little helper I pulled from the mips tree.
> > ... 
> > > Signed-off-by: Daniel Walker <dwalker@mvista.com>
> > 
> > And who is the original author of this little helper in the MIPS tree?
> 
> commit 16b7b2ac0148e839da86af8747b6fa4aad43a9b7
> Author: Atsushi Nemoto <anemo@mba.ocn.ne.jp>

I think Ralf's version is better starting point.

commit 93c846f9047f392fc2335668a5234edfbddb7cdc
Author: Ralf Baechle <ralf@linux-mips.org>
Date:   Fri Oct 19 08:13:08 2007 +0100

    [MIPS] time: Helpers to compute clocksource/event shift and mult values.

These helpers are live in arch/mips/kernel/time.c now.

void __init clocksource_set_clock(struct clocksource *cs, unsigned int clock)
{
	u64 temp;
	u32 shift;

	/* Find a shift value */
	for (shift = 32; shift > 0; shift--) {
		temp = (u64) NSEC_PER_SEC << shift;
		do_div(temp, clock);
		if ((temp >> 32) == 0)
			break;
	}
	cs->shift = shift;
	cs->mult = (u32) temp;
}

BTW, comparing this with clocksource_hz2mult(), inserting

	temp += clock / 2;

to just before do_div() line might be better.

---
Atsushi Nemoto

Re: [PATCH] clocksource: shift helper

[Roman Zippel]

Hi,

On Thursday 1. May 2008, Daniel Walker wrote:

> This is a little helper I pulled from the mips tree. I've seen a couple
> of bogus shift values, and this helper calculates the shift. This
> should move the shift selection away from the user, and systematically
> pick the value base on the hz.
>
> I also modified the acpi_pm timer to use this new interface. The original
> shift was 22 , and after this patch it's increased to 23. Which should make
> the clocksource slightly more accurate, but shouldn't have much of a
> visible effect.

What is this calculation based on?

Unless I miss something even 22 is far more than enough. acpi_pm has a 
resolution of 279ns, thus the clock can't be more accurate than half of this 
on average and increasing the shift doesn't change much directly about it.
What the shift value does influence is the size of the adjustment step the 
clock code can do during an update, so with shift=22 and HZ=100 the possible 
adjustment step would be 0.008ns (freq/2^shift/ntp_hz). The problem is that 
this value is also used for how soon the clock is being adjusted, this means 
the clock code is constantly busy adjusting for a very small error.
So from a clock adjustment perspective a shift value close to log2
(freq/res/10^9/ntp_hz) (with res=1/freq and being limited to 1ns<=res<=1us) 
would provide sufficient accuracy, while keeping the need for adjustment low, 
this means with the above example a shift value of much more than 8 doesn't 
improve accuracy significantly.

That said, I agree that a dynamic calculation of the shift value is a good 
thing, but I don't think that the method in the patch is a good one.

bye, Roman

Re: [PATCH] clocksource: shift helper

[Daniel Walker]

On Wed, 2008-05-07 at 05:57 +0200, Roman Zippel wrote:
> Hi,
> 
> On Thursday 1. May 2008, Daniel Walker wrote:
> 
> > This is a little helper I pulled from the mips tree. I've seen a couple
> > of bogus shift values, and this helper calculates the shift. This
> > should move the shift selection away from the user, and systematically
> > pick the value base on the hz.
> >
> > I also modified the acpi_pm timer to use this new interface. The original
> > shift was 22 , and after this patch it's increased to 23. Which should make
> > the clocksource slightly more accurate, but shouldn't have much of a
> > visible effect.
> 
> What is this calculation based on?

It's based on the clocksource_hz2mult() calculation. It tests multiple
shifts until the largest workable one is found.

> Unless I miss something even 22 is far more than enough. acpi_pm has a 
> resolution of 279ns, thus the clock can't be more accurate than half of this 
> on average and increasing the shift doesn't change much directly about it.
> What the shift value does influence is the size of the adjustment step the 
> clock code can do during an update, so with shift=22 and HZ=100 the possible 
> adjustment step would be 0.008ns (freq/2^shift/ntp_hz). The problem is that 
> this value is also used for how soon the clock is being adjusted, this means 
> the clock code is constantly busy adjusting for a very small error.
> So from a clock adjustment perspective a shift value close to log2
> (freq/res/10^9/ntp_hz) (with res=1/freq and being limited to 1ns<=res<=1us) 
> would provide sufficient accuracy, while keeping the need for adjustment low, 
> this means with the above example a shift value of much more than 8 doesn't 
> improve accuracy significantly.

This seems like a problem with the current method also.. Given that
people just "select" a shift, they could pick one that is too large or
too small easily. 

The method in this patch is not very sophisticated, so we could make it
take more into account.

Daniel