Messages in this thread | | | Date | Mon, 29 Apr 2013 18:00:27 -0700 | From | John Stultz <> | Subject | Re: [patch 05/15] clocksource: Allow clocksource select to skip current clocksource |
| |
On 04/25/2013 01:31 PM, Thomas Gleixner wrote: > Preparatory patch for clocksource unbind support. > > Modify clocksource_select, so it skips the current clocksource on > request and tries to find a fallback clocksource. Convert all existing > users. No functional change. > > Signed-off-by: Thomas Gleixner<tglx@linutronix.de> > --- > kernel/time/clocksource.c | 24 ++++++++++++++---------- > 1 file changed, 14 insertions(+), 10 deletions(-) > > Index: tip/kernel/time/clocksource.c > =================================================================== > --- tip.orig/kernel/time/clocksource.c > +++ tip/kernel/time/clocksource.c > @@ -553,7 +553,7 @@ static u64 clocksource_max_deferment(str > > #ifndef CONFIG_ARCH_USES_GETTIMEOFFSET > > -static struct clocksource *clocksource_find_best(bool oneshot) > +static struct clocksource *clocksource_find_best(bool oneshot, bool skipcur) > { > struct clocksource *cs; > > @@ -566,6 +566,8 @@ static struct clocksource *clocksource_f > * the best rating. > */ > list_for_each_entry(cs, &clocksource_list, list) { > + if (skipcur && cs == curr_clocksource) > + continue; > if (oneshot && !(cs->flags & CLOCK_SOURCE_VALID_FOR_HRES)) > continue; > return cs; > @@ -581,18 +583,20 @@ static struct clocksource *clocksource_f > * Select the clocksource with the best rating, or the clocksource, > * which is selected by userspace override. > */ > -static void clocksource_select(void) > +static void clocksource_select(bool skipcur) > { > bool oneshot = tick_oneshot_mode_active(); > struct clocksource *best, *cs; > > /* Find the best suitable clocksource */ > - best = clocksource_find_best(oneshot); > + best = clocksource_find_best(oneshot, skipcur); > if (!best) > return; > > /* Check for the override clocksource. */ > list_for_each_entry(cs, &clocksource_list, list) { > + if (skipcur && cs == curr_clocksource) > + continue; > if (strcmp(cs->name, override_name) != 0) > continue; > /* > @@ -620,7 +624,7 @@ static void clocksource_select(void) > > #else /* !CONFIG_ARCH_USES_GETTIMEOFFSET */ > > -static inline void clocksource_select(void) { } > +static inline void clocksource_select(bool skipcur) { } > > #endif
Not a major objection, but I sort of don't like the bool flag arguments to function that alters their behavior, since its not really very descriptive. I know I'll be looking at the code below thinking "hmmm.. clocksource_select(false).. now what does that mean?" at some point not too far away.
Instead, would leaving clocksource_select() alone, and introducing a clocksource_select_fallback() for the skipcur behavior, be a more descriptive choice? The code could very much use the same skipcur logic, in a __clocksource_select() or something function, but just helps make the normal usage easier to read.
> > @@ -645,7 +649,7 @@ static int __init clocksource_done_booti > clocksource_watchdog_kthread(NULL); > > mutex_lock(&clocksource_mutex); > - clocksource_select(); > + clocksource_select(false); > mutex_unlock(&clocksource_mutex); > return 0; > } > @@ -739,7 +743,7 @@ int __clocksource_register_scale(struct > mutex_lock(&clocksource_mutex); > clocksource_enqueue(cs); > clocksource_enqueue_watchdog(cs); > - clocksource_select(); > + clocksource_select(false); > mutex_unlock(&clocksource_mutex); > return 0; > } > @@ -766,7 +770,7 @@ int clocksource_register(struct clocksou > mutex_lock(&clocksource_mutex); > clocksource_enqueue(cs); > clocksource_enqueue_watchdog(cs); > - clocksource_select(); > + clocksource_select(false); > mutex_unlock(&clocksource_mutex); > return 0; > }
thanks -john
| |