current_tick_length used to do a little more, but now it just returns
tick_length, which we can also access directly at the few places, where
it's needed.
Signed-off-by: Roman Zippel <[email protected]>
---
include/linux/timex.h | 4 ++--
kernel/time/ntp.c | 16 ++--------------
kernel/time/timekeeping.c | 5 ++---
3 files changed, 6 insertions(+), 19 deletions(-)
Index: linux-2.6-mm/include/linux/timex.h
===================================================================
--- linux-2.6-mm.orig/include/linux/timex.h
+++ linux-2.6-mm/include/linux/timex.h
@@ -234,8 +234,8 @@ static inline int ntp_synced(void)
#endif
#define NTP_INTERVAL_LENGTH (NSEC_PER_SEC/NTP_INTERVAL_FREQ)
-/* Returns how long ticks are at present, in ns / 2^(SHIFT_SCALE-10). */
-extern u64 current_tick_length(void);
+/* Returns how long ticks are at present, in ns / 2^NTP_SCALE_SHIFT. */
+extern u64 tick_length;
extern void second_overflow(void);
extern void update_ntp_one_tick(void);
Index: linux-2.6-mm/kernel/time/ntp.c
===================================================================
--- linux-2.6-mm.orig/kernel/time/ntp.c
+++ linux-2.6-mm/kernel/time/ntp.c
@@ -23,7 +23,8 @@
*/
unsigned long tick_usec = TICK_USEC; /* USER_HZ period (usec) */
unsigned long tick_nsec; /* ACTHZ period (nsec) */
-static u64 tick_length, tick_length_base;
+u64 tick_length;
+static u64 tick_length_base;
#define MAX_TICKADJ 500 /* microsecs */
#define MAX_TICKADJ_SCALED (((u64)(MAX_TICKADJ * NSEC_PER_USEC) << \
@@ -203,19 +204,6 @@ void second_overflow(void)
}
}
-/*
- * Return how long ticks are at the moment, that is, how much time
- * update_wall_time_one_tick will add to xtime next time we call it
- * (assuming no calls to do_adjtimex in the meantime).
- * The return value is in fixed-point nanoseconds shifted by the
- * specified number of bits to the right of the binary point.
- * This function has no side-effects.
- */
-u64 current_tick_length(void)
-{
- return tick_length;
-}
-
#ifdef CONFIG_GENERIC_CMOS_UPDATE
/* Disable the cmos update - used by virtualization and embedded */
Index: linux-2.6-mm/kernel/time/timekeeping.c
===================================================================
--- linux-2.6-mm.orig/kernel/time/timekeeping.c
+++ linux-2.6-mm/kernel/time/timekeeping.c
@@ -374,8 +374,7 @@ static __always_inline int clocksource_b
* Now calculate the error in (1 << look_ahead) ticks, but first
* remove the single look ahead already included in the error.
*/
- tick_error = current_tick_length() >>
- (NTP_SCALE_SHIFT - clock->shift + 1);
+ tick_error = tick_length >> (NTP_SCALE_SHIFT - clock->shift + 1);
tick_error -= clock->xtime_interval >> 1;
error = ((error - tick_error) >> look_ahead) + tick_error;
@@ -467,7 +466,7 @@ void update_wall_time(void)
}
/* accumulate error between NTP and clock interval */
- clock->error += current_tick_length();
+ clock->error += tick_length;
clock->error -= clock->xtime_interval << (NTP_SCALE_SHIFT - clock->shift);
}
--
On Fri, 2008-03-14 at 19:40 +0100, [email protected] wrote:
> plain text document attachment (tick_length)
> current_tick_length used to do a little more, but now it just returns
> tick_length, which we can also access directly at the few places, where
> it's needed.
>
> Signed-off-by: Roman Zippel <[email protected]>
Hrm. I'm not sure I like using a global variable instead of using an
accessor function. At least with the accessor function folks couldn't
just tweak the value outside ntp as easily.
Is there any additional rational for this change?
thanks
-john
Hi,
On Fri, 14 Mar 2008, john stultz wrote:
> On Fri, 2008-03-14 at 19:40 +0100, [email protected] wrote:
> > plain text document attachment (tick_length)
> > current_tick_length used to do a little more, but now it just returns
> > tick_length, which we can also access directly at the few places, where
> > it's needed.
> >
> > Signed-off-by: Roman Zippel <[email protected]>
>
> Hrm. I'm not sure I like using a global variable instead of using an
> accessor function. At least with the accessor function folks couldn't
> just tweak the value outside ntp as easily.
Why would someone do something silly like this?
> Is there any additional rational for this change?
Useless bloat?
bye, Roman
On Sat, 2008-03-15 at 04:32 +0100, Roman Zippel wrote:
> Hi,
>
> On Fri, 14 Mar 2008, john stultz wrote:
>
> > On Fri, 2008-03-14 at 19:40 +0100, [email protected] wrote:
> > > plain text document attachment (tick_length)
> > > current_tick_length used to do a little more, but now it just returns
> > > tick_length, which we can also access directly at the few places, where
> > > it's needed.
> > >
> > > Signed-off-by: Roman Zippel <[email protected]>
> >
> > Hrm. I'm not sure I like using a global variable instead of using an
> > accessor function. At least with the accessor function folks couldn't
> > just tweak the value outside ntp as easily.
>
> Why would someone do something silly like this?
Its not a huge deal, but we've seen globally scoped timekeeping
variables misused either accidentally or intentionally. Awhile back ppc
was corrupting time_offset by using it for a timezone offset value.
So I think its a valid maintainability issue.
> > Is there any additional rational for this change?
>
> Useless bloat?
I agree its a trade off. But do you have performance numbers to make the
maintainability trade off worth it? (I admit, it is used in the
timer_interrupt, so it may very well be worth it, but we might want to
check first).
thanks
-john
Hi,
On Fri, 14 Mar 2008, john stultz wrote:
> > Why would someone do something silly like this?
>
> Its not a huge deal, but we've seen globally scoped timekeeping
> variables misused either accidentally or intentionally. Awhile back ppc
> was corrupting time_offset by using it for a timezone offset value.
>
> So I think its a valid maintainability issue.
ppc used to do a whole lot more than that, so I don't think that counts as
a valid example.
tick_length is overwritten at every timer and modifying it would have
barely any effect compared to other values which are already exported.
> > > Is there any additional rational for this change?
> >
> > Useless bloat?
>
> I agree its a trade off. But do you have performance numbers to make the
> maintainability trade off worth it? (I admit, it is used in the
> timer_interrupt, so it may very well be worth it, but we might want to
> check first).
It depends on the architecture, but it's effects regularly executed code
and every byte counts.
bye, Roman
On Fri, Mar 14, 2008 at 9:18 PM, Roman Zippel <[email protected]> wrote:
> > > > Is there any additional rational for this change?
> > >
> > > Useless bloat?
> >
> > I agree its a trade off. But do you have performance numbers to make the
> > maintainability trade off worth it? (I admit, it is used in the
> > timer_interrupt, so it may very well be worth it, but we might want to
> > check first).
>
> It depends on the architecture, but it's effects regularly executed code
> and every byte counts.
Then make the original function an inline. With -O2 it should compile
to exactly the same thing.
Hi,
On Sat, 15 Mar 2008, Ray Lee wrote:
> Then make the original function an inline. With -O2 it should compile
> to exactly the same thing.
This would also defeat John's intention of keeping the value static.
bye, Roman
On Sat, 2008-03-15 at 18:14 +0100, Roman Zippel wrote:
> Hi,
>
> On Sat, 15 Mar 2008, Ray Lee wrote:
>
> > Then make the original function an inline. With -O2 it should compile
> > to exactly the same thing.
>
> This would also defeat John's intention of keeping the value static.
Well, don't mistake me for being fanatical about it. Having the values
be static is cleaner, but if its a real performance issue, then clearly
performance wins.
I do like Ray's suggestion, and think using the inline'd function is
preferred to the raw variable, as it better establishes through use if
nothing else, the read-only nature of the value outside of ntp.
thanks
-john
Hi,
On Tuesday 18. March 2008, john stultz wrote:
> On Sat, 2008-03-15 at 18:14 +0100, Roman Zippel wrote:
> > Hi,
> >
> > On Sat, 15 Mar 2008, Ray Lee wrote:
> > > Then make the original function an inline. With -O2 it should compile
> > > to exactly the same thing.
> >
> > This would also defeat John's intention of keeping the value static.
>
> Well, don't mistake me for being fanatical about it. Having the values
> be static is cleaner, but if its a real performance issue, then clearly
> performance wins.
There are two aspects, such functions tend to generate slightly larger and
slower code.
> I do like Ray's suggestion, and think using the inline'd function is
> preferred to the raw variable, as it better establishes through use if
> nothing else, the read-only nature of the value outside of ntp.
In other languages one would use private or protected for this, but we don't
have this. I'm not too fond of an inline function, as it would mark it as
some kind of public API, which it isn't. It's just an internal value used by
the timekeeping code, which happens to be needed by a few source files. If
you want to make a little more private, it would be better to move it to
header under kernel/time/.
bye, Roman