From: Martin Schwidefsky <[email protected]>
Remove clocksource_read, clocksource_enable and clocksource_disable
inline functions. No functional change.
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: john stultz <[email protected]>
Cc: Daniel Walker <[email protected]>
Signed-off-by: Martin Schwidefsky <[email protected]>
---
include/linux/clocksource.h | 46 --------------------------------------------
kernel/time/timekeeping.c | 32 +++++++++++++++++-------------
2 files changed, 18 insertions(+), 60 deletions(-)
Index: linux-2.6/kernel/time/timekeeping.c
===================================================================
--- linux-2.6.orig/kernel/time/timekeeping.c
+++ linux-2.6/kernel/time/timekeeping.c
@@ -79,7 +79,7 @@ static void clocksource_forward_now(void
cycle_t cycle_now, cycle_delta;
s64 nsec;
- cycle_now = clocksource_read(clock);
+ cycle_now = clock->read(clock);
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
clock->cycle_last = cycle_now;
@@ -114,7 +114,7 @@ void getnstimeofday(struct timespec *ts)
*ts = xtime;
/* read clocksource: */
- cycle_now = clocksource_read(clock);
+ cycle_now = clock->read(clock);
/* calculate the delta since the last update_wall_time: */
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -146,7 +146,7 @@ ktime_t ktime_get(void)
nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
/* read clocksource: */
- cycle_now = clocksource_read(clock);
+ cycle_now = clock->read(clock);
/* calculate the delta since the last update_wall_time: */
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -186,7 +186,7 @@ void ktime_get_ts(struct timespec *ts)
tomono = wall_to_monotonic;
/* read clocksource: */
- cycle_now = clocksource_read(clock);
+ cycle_now = clock->read(clock);
/* calculate the delta since the last update_wall_time: */
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -274,16 +274,18 @@ static void change_clocksource(void)
clocksource_forward_now();
- if (clocksource_enable(new))
+ if (new->enable && ! new->enable(new))
return;
+ /* save mult_orig on enable */
+ new->mult_orig = new->mult;
new->raw_time = clock->raw_time;
old = clock;
clock = new;
- clocksource_disable(old);
+ if (old->disable)
+ old->disable(old);
- clock->cycle_last = 0;
- clock->cycle_last = clocksource_read(clock);
+ clock->cycle_last = clock->read(clock);
clock->error = 0;
clock->xtime_nsec = 0;
clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
@@ -373,7 +375,7 @@ void getrawmonotonic(struct timespec *ts
seq = read_seqbegin(&xtime_lock);
/* read clocksource: */
- cycle_now = clocksource_read(clock);
+ cycle_now = clock->read(clock);
/* calculate the delta since the last update_wall_time: */
cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
@@ -435,9 +437,12 @@ void __init timekeeping_init(void)
ntp_init();
clock = clocksource_get_next();
- clocksource_enable(clock);
+ if (clock->enable)
+ clock->enable(clock);
+ /* save mult_orig on enable */
+ clock->mult_orig = clock->mult;
clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
- clock->cycle_last = clocksource_read(clock);
+ clock->cycle_last = clock->read(clock);
xtime.tv_sec = sec;
xtime.tv_nsec = 0;
@@ -477,8 +482,7 @@ static int timekeeping_resume(struct sys
}
update_xtime_cache(0);
/* re-base the last cycle value */
- clock->cycle_last = 0;
- clock->cycle_last = clocksource_read(clock);
+ clock->cycle_last = clock->read(clock);
clock->error = 0;
timekeeping_suspended = 0;
write_sequnlock_irqrestore(&xtime_lock, flags);
@@ -630,7 +634,7 @@ void update_wall_time(void)
return;
#ifdef CONFIG_GENERIC_TIME
- offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
+ offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
#else
offset = clock->cycle_interval;
#endif
Index: linux-2.6/include/linux/clocksource.h
===================================================================
--- linux-2.6.orig/include/linux/clocksource.h
+++ linux-2.6/include/linux/clocksource.h
@@ -268,52 +268,6 @@ static inline u32 clocksource_hz2mult(u3
}
/**
- * clocksource_read: - Access the clocksource's current cycle value
- * @cs: pointer to clocksource being read
- *
- * Uses the clocksource to return the current cycle_t value
- */
-static inline cycle_t clocksource_read(struct clocksource *cs)
-{
- return cs->read(cs);
-}
-
-/**
- * clocksource_enable: - enable clocksource
- * @cs: pointer to clocksource
- *
- * Enables the specified clocksource. The clocksource callback
- * function should start up the hardware and setup mult and field
- * members of struct clocksource to reflect hardware capabilities.
- */
-static inline int clocksource_enable(struct clocksource *cs)
-{
- int ret = 0;
-
- if (cs->enable)
- ret = cs->enable(cs);
-
- /* save mult_orig on enable */
- cs->mult_orig = cs->mult;
-
- return ret;
-}
-
-/**
- * clocksource_disable: - disable clocksource
- * @cs: pointer to clocksource
- *
- * Disables the specified clocksource. The clocksource callback
- * function should power down the now unused hardware block to
- * save power.
- */
-static inline void clocksource_disable(struct clocksource *cs)
-{
- if (cs->disable)
- cs->disable(cs);
-}
-
-/**
* cyc2ns - converts clocksource cycles to nanoseconds
* @cs: Pointer to clocksource
* @cycles: Cycles
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (clocksource-inline.diff)
> From: Martin Schwidefsky <[email protected]>
>
> Remove clocksource_read, clocksource_enable and clocksource_disable
> inline functions. No functional change.
>
Your still not really explaining this one, is this suppose to be
cleaner? Or is this related to some other part of your clean up?
Daniel
On Wed, 29 Jul 2009 08:15:19 -0600
[email protected] wrote:
> > Remove clocksource_read, clocksource_enable and clocksource_disable
> > inline functions. No functional change.
> >
>
> Your still not really explaining this one, is this suppose to be
> cleaner? Or is this related to some other part of your clean up?
The only one of the three inline functions that is a bit more
complicated is clocksource_enable() because of the mult_orig logic. But
that goes away with a later patch. The function aren't accessors either,
they are used exclusively by the timekeeping code. In short, they are
useless, don't you think?
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 2009-07-29 at 16:44 +0200, Martin Schwidefsky wrote:
> On Wed, 29 Jul 2009 08:15:19 -0600
> [email protected] wrote:
>
> > > Remove clocksource_read, clocksource_enable and clocksource_disable
> > > inline functions. No functional change.
> > >
> >
> > Your still not really explaining this one, is this suppose to be
> > cleaner? Or is this related to some other part of your clean up?
>
> The only one of the three inline functions that is a bit more
> complicated is clocksource_enable() because of the mult_orig logic. But
> that goes away with a later patch. The function aren't accessors either,
> they are used exclusively by the timekeeping code. In short, they are
> useless, don't you think?
Above is what should go in your patch description ..
The reason that I'm not totally into this one is cause these inlines
help to document to the code..
If you have ,
struct clocksource cs;
then several lines later you have
cs->read();
vs,
clocksource_read(cs);
The later is completely clear, and the former isn't.. Instead of "cs"
you could pick any obscure name, and read() isn't exactly unique.. So
really any function in the clocksource structure has the potential for a
helper, and the inlines don't really cost anything ..
Daniel
On Wed, 29 Jul 2009 08:57:13 -0600
[email protected] wrote:
> On Wed, 2009-07-29 at 16:44 +0200, Martin Schwidefsky wrote:
> > On Wed, 29 Jul 2009 08:15:19 -0600
> > [email protected] wrote:
> >
> > > > Remove clocksource_read, clocksource_enable and clocksource_disable
> > > > inline functions. No functional change.
> > > >
> > >
> > > Your still not really explaining this one, is this suppose to be
> > > cleaner? Or is this related to some other part of your clean up?
> >
> > The only one of the three inline functions that is a bit more
> > complicated is clocksource_enable() because of the mult_orig logic. But
> > that goes away with a later patch. The function aren't accessors either,
> > they are used exclusively by the timekeeping code. In short, they are
> > useless, don't you think?
>
> Above is what should go in your patch description ..
Ok, sounds reasonable.
> The reason that I'm not totally into this one is cause these inlines
> help to document to the code..
>
> If you have ,
>
> struct clocksource cs;
>
> then several lines later you have
>
> cs->read();
>
> vs,
>
> clocksource_read(cs);
>
> The later is completely clear, and the former isn't.. Instead of "cs"
> you could pick any obscure name, and read() isn't exactly unique.. So
> really any function in the clocksource structure has the potential for a
> helper, and the inlines don't really cost anything ..
Hmm, you have an object of type struct clocksource and you do
cs->read(cs). If that is not clear enough then I don't know what is. We
do that all over the place in the linux kernel. And I personally find
these useless wrappers rather annoying. I don't like to have to jump to
another place to find out that it just calls the read function of the
object.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 2009-07-29 at 17:32 +0200, Martin Schwidefsky wrote:
> Hmm, you have an object of type struct clocksource and you do
> cs->read(cs). If that is not clear enough then I don't know what is.
It's not as clear as it could be .. In the case above you have to look
in at least two places to know what's going on.. First to see the
cs->read() , and second to see if "cs" is actually a clocksource or
something else.. "cs" could be declared anyplace with any name.
If you see clocksource_read(cs) , you might need to once check what
clocksource_read() is actually doing, but only once.. After that when
you see that function you know that variable is a clocksource, and it's
"read()" is getting called. So you only need to review one line in the
simplest case.
Daniel
On Wed, Jul 29, 2009 at 4:32 PM, Martin
Schwidefsky<[email protected]> wrote:
> Hmm, you have an object of type struct clocksource and you do
> cs->read(cs). If that is not clear enough then I don't know what is. We
> do that all over the place in the linux kernel. And I personally find
> these useless wrappers rather annoying. I don't like to have to jump to
> another place to find out that it just calls the read function of the
> object.
An argument for the helper is that it eases grepability. Sure you can
search for "->read" but that's going to turn up all kinds of
non-clocksource code as well. grep clocksource_read will get you
exactly what you want.
On Wed, 29 Jul 2009 16:36:46 +0100
Will Newton <[email protected]> wrote:
> On Wed, Jul 29, 2009 at 4:32 PM, Martin
> Schwidefsky<[email protected]> wrote:
>
> > Hmm, you have an object of type struct clocksource and you do
> > cs->read(cs). If that is not clear enough then I don't know what is. We
> > do that all over the place in the linux kernel. And I personally find
> > these useless wrappers rather annoying. I don't like to have to jump to
> > another place to find out that it just calls the read function of the
> > object.
>
> An argument for the helper is that it eases grepability. Sure you can
> search for "->read" but that's going to turn up all kinds of
> non-clocksource code as well. grep clocksource_read will get you
> exactly what you want.
That would make sense if the clocksource_read calls are littered all
over the kernel source. But they are not, the only user is
timekeeping.c
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 29 Jul 2009 08:52:50 -0700
Daniel Walker <[email protected]> wrote:
> On Wed, 2009-07-29 at 17:32 +0200, Martin Schwidefsky wrote:
> > Hmm, you have an object of type struct clocksource and you do
> > cs->read(cs). If that is not clear enough then I don't know what is.
>
> It's not as clear as it could be .. In the case above you have to look
> in at least two places to know what's going on.. First to see the
> cs->read() , and second to see if "cs" is actually a clocksource or
> something else.. "cs" could be declared anyplace with any name.
Well you have something like that in the code:
struct clocksource *clock;
clock = timekeeper.clock;
cycle_now = clock->read(clock);
If I read the function top to bottom I immediately see that clock is a
clocksource and that the code does a read on it. That is not the case
if I need to lookup the clocksource_read wrapper.
> If you see clocksource_read(cs) , you might need to once check what
> clocksource_read() is actually doing, but only once.. After that when
> you see that function you know that variable is a clocksource, and it's
> "read()" is getting called. So you only need to review one line in the
> simplest case.
After you learned (once) that timekeeper.clock is a clock source you
have no trouble to understand the 6 occurrences of clock->read(clock)
there are in the code.
Anyway this seems to be a matter of personal preference, in the end I
don't care too much about the inline functions.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 29 Jul 2009 18:27:54 +0200
Martin Schwidefsky <[email protected]> wrote:
> On Wed, 29 Jul 2009 16:36:46 +0100
> Will Newton <[email protected]> wrote:
>
> > On Wed, Jul 29, 2009 at 4:32 PM, Martin
> > Schwidefsky<[email protected]> wrote:
> >
> > > Hmm, you have an object of type struct clocksource and you do
> > > cs->read(cs). If that is not clear enough then I don't know what is. We
> > > do that all over the place in the linux kernel. And I personally find
> > > these useless wrappers rather annoying. I don't like to have to jump to
> > > another place to find out that it just calls the read function of the
> > > object.
> >
> > An argument for the helper is that it eases grepability. Sure you can
> > search for "->read" but that's going to turn up all kinds of
> > non-clocksource code as well. grep clocksource_read will get you
> > exactly what you want.
>
> That would make sense if the clocksource_read calls are littered all
> over the kernel source. But they are not, the only user is
> timekeeping.c
And What I find odd as well is e.g. this:
offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask
A mixture of a wrapper to get something out of the clock and a direct
access to other fields of the clocksource. Either it is wrapped or it
isn't, no? A proper wrapper would do all of the above and not just the
call over the read function pointer.
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Wed, 29 Jul 2009 18:27:54 +0200, Martin Schwidefsky said:
> On Wed, 29 Jul 2009 16:36:46 +0100 Will Newton <[email protected]> wrote:
> > An argument for the helper is that it eases grepability. Sure you can
> > search for "->read" but that's going to turn up all kinds of
> > non-clocksource code as well. grep clocksource_read will get you
> > exactly what you want.
>
> That would make sense if the clocksource_read calls are littered all
> over the kernel source. But they are not, the only user is
> timekeeping.c
You know that a priori because you're familiar with that code. But there's
another use case: An idiot monkey like myself manages to break the kernel
*again* in some part of the kernel they're totally unfamiliar with, and
they need to discover for themselves that timekeeping.c is the only user.
(Of course, in another 5-6 years I'll probably have broken something in
every part of the kernel and whinged at Andrew about it, and that argument
won't apply anymore.. ;)
On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> plain text document attachment (clocksource-inline.diff)
> From: Martin Schwidefsky <[email protected]>
>
> Remove clocksource_read, clocksource_enable and clocksource_disable
> inline functions. No functional change.
>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: john stultz <[email protected]>
> Cc: Daniel Walker <[email protected]>
> Signed-off-by: Martin Schwidefsky <[email protected]>
Again, Magnus's fix for mult_orig that Thomas has (or *should* have)
queued at this point for 2.6.31 will collide with this, but the fix is
trivial.
Acked-by: John Stultz <[email protected]>
> ---
> include/linux/clocksource.h | 46 --------------------------------------------
> kernel/time/timekeeping.c | 32 +++++++++++++++++-------------
> 2 files changed, 18 insertions(+), 60 deletions(-)
>
> Index: linux-2.6/kernel/time/timekeeping.c
> ===================================================================
> --- linux-2.6.orig/kernel/time/timekeeping.c
> +++ linux-2.6/kernel/time/timekeeping.c
> @@ -79,7 +79,7 @@ static void clocksource_forward_now(void
> cycle_t cycle_now, cycle_delta;
> s64 nsec;
>
> - cycle_now = clocksource_read(clock);
> + cycle_now = clock->read(clock);
> cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> clock->cycle_last = cycle_now;
>
> @@ -114,7 +114,7 @@ void getnstimeofday(struct timespec *ts)
> *ts = xtime;
>
> /* read clocksource: */
> - cycle_now = clocksource_read(clock);
> + cycle_now = clock->read(clock);
>
> /* calculate the delta since the last update_wall_time: */
> cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -146,7 +146,7 @@ ktime_t ktime_get(void)
> nsecs = xtime.tv_nsec + wall_to_monotonic.tv_nsec;
>
> /* read clocksource: */
> - cycle_now = clocksource_read(clock);
> + cycle_now = clock->read(clock);
>
> /* calculate the delta since the last update_wall_time: */
> cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -186,7 +186,7 @@ void ktime_get_ts(struct timespec *ts)
> tomono = wall_to_monotonic;
>
> /* read clocksource: */
> - cycle_now = clocksource_read(clock);
> + cycle_now = clock->read(clock);
>
> /* calculate the delta since the last update_wall_time: */
> cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -274,16 +274,18 @@ static void change_clocksource(void)
>
> clocksource_forward_now();
>
> - if (clocksource_enable(new))
> + if (new->enable && ! new->enable(new))
> return;
> + /* save mult_orig on enable */
> + new->mult_orig = new->mult;
>
> new->raw_time = clock->raw_time;
> old = clock;
> clock = new;
> - clocksource_disable(old);
> + if (old->disable)
> + old->disable(old);
>
> - clock->cycle_last = 0;
> - clock->cycle_last = clocksource_read(clock);
> + clock->cycle_last = clock->read(clock);
> clock->error = 0;
> clock->xtime_nsec = 0;
> clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
> @@ -373,7 +375,7 @@ void getrawmonotonic(struct timespec *ts
> seq = read_seqbegin(&xtime_lock);
>
> /* read clocksource: */
> - cycle_now = clocksource_read(clock);
> + cycle_now = clock->read(clock);
>
> /* calculate the delta since the last update_wall_time: */
> cycle_delta = (cycle_now - clock->cycle_last) & clock->mask;
> @@ -435,9 +437,12 @@ void __init timekeeping_init(void)
> ntp_init();
>
> clock = clocksource_get_next();
> - clocksource_enable(clock);
> + if (clock->enable)
> + clock->enable(clock);
> + /* save mult_orig on enable */
> + clock->mult_orig = clock->mult;
> clocksource_calculate_interval(clock, NTP_INTERVAL_LENGTH);
> - clock->cycle_last = clocksource_read(clock);
> + clock->cycle_last = clock->read(clock);
>
> xtime.tv_sec = sec;
> xtime.tv_nsec = 0;
> @@ -477,8 +482,7 @@ static int timekeeping_resume(struct sys
> }
> update_xtime_cache(0);
> /* re-base the last cycle value */
> - clock->cycle_last = 0;
> - clock->cycle_last = clocksource_read(clock);
> + clock->cycle_last = clock->read(clock);
> clock->error = 0;
> timekeeping_suspended = 0;
> write_sequnlock_irqrestore(&xtime_lock, flags);
> @@ -630,7 +634,7 @@ void update_wall_time(void)
> return;
>
> #ifdef CONFIG_GENERIC_TIME
> - offset = (clocksource_read(clock) - clock->cycle_last) & clock->mask;
> + offset = (clock->read(clock) - clock->cycle_last) & clock->mask;
> #else
> offset = clock->cycle_interval;
> #endif
> Index: linux-2.6/include/linux/clocksource.h
> ===================================================================
> --- linux-2.6.orig/include/linux/clocksource.h
> +++ linux-2.6/include/linux/clocksource.h
> @@ -268,52 +268,6 @@ static inline u32 clocksource_hz2mult(u3
> }
>
> /**
> - * clocksource_read: - Access the clocksource's current cycle value
> - * @cs: pointer to clocksource being read
> - *
> - * Uses the clocksource to return the current cycle_t value
> - */
> -static inline cycle_t clocksource_read(struct clocksource *cs)
> -{
> - return cs->read(cs);
> -}
> -
> -/**
> - * clocksource_enable: - enable clocksource
> - * @cs: pointer to clocksource
> - *
> - * Enables the specified clocksource. The clocksource callback
> - * function should start up the hardware and setup mult and field
> - * members of struct clocksource to reflect hardware capabilities.
> - */
> -static inline int clocksource_enable(struct clocksource *cs)
> -{
> - int ret = 0;
> -
> - if (cs->enable)
> - ret = cs->enable(cs);
> -
> - /* save mult_orig on enable */
> - cs->mult_orig = cs->mult;
> -
> - return ret;
> -}
> -
> -/**
> - * clocksource_disable: - disable clocksource
> - * @cs: pointer to clocksource
> - *
> - * Disables the specified clocksource. The clocksource callback
> - * function should power down the now unused hardware block to
> - * save power.
> - */
> -static inline void clocksource_disable(struct clocksource *cs)
> -{
> - if (cs->disable)
> - cs->disable(cs);
> -}
> -
> -/**
> * cyc2ns - converts clocksource cycles to nanoseconds
> * @cs: Pointer to clocksource
> * @cycles: Cycles
>
On Wed, Jul 29, 2009 at 07:15:30AM -0700, Daniel Walker wrote:
> On Wed, 2009-07-29 at 15:41 +0200, Martin Schwidefsky wrote:
> > plain text document attachment (clocksource-inline.diff)
> > From: Martin Schwidefsky <[email protected]>
> >
> > Remove clocksource_read, clocksource_enable and clocksource_disable
> > inline functions. No functional change.
> >
>
> Your still not really explaining this one, is this suppose to be
> cleaner? Or is this related to some other part of your clean up?
It removes an entirely useless layer of obsfucation.
> You know that a priori because you're familiar with that code. But there's
> another use case: An idiot monkey like myself manages to break the kernel
> *again* in some part of the kernel they're totally unfamiliar with, and
> they need to discover for themselves that timekeeping.c is the only user.
>
> (Of course, in another 5-6 years I'll probably have broken something in
> every part of the kernel and whinged at Andrew about it, and that argument
> won't apply anymore.. ;)
Andthe poor grepping monkey can be sure that everyone used the useless
wrapper exactly how? If you want to help people with grepping rename
the method from read to clocksource_read..
On Thu, 30 Jul 2009 17:48:36 EDT, Christoph Hellwig said:
> > You know that a priori because you're familiar with that code. But there's
> > another use case: An idiot monkey like myself manages to break the kernel
> > *again* in some part of the kernel they're totally unfamiliar with, and
> > they need to discover for themselves that timekeeping.c is the only user.
> >
> > (Of course, in another 5-6 years I'll probably have broken something in
> > every part of the kernel and whinged at Andrew about it, and that argument
> > won't apply anymore.. ;)
>
> Andthe poor grepping monkey can be sure that everyone used the useless
> wrapper exactly how? If you want to help people with grepping rename
> the method from read to clocksource_read..
Even better. ;)