lkml.org 
[lkml]   [2013]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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



\
 
 \ /
  Last update: 2013-04-30 03:21    [W:0.447 / U:1.080 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site