From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759895Ab3D3BAb (ORCPT ); Mon, 29 Apr 2013 21:00:31 -0400 Received: from mail-pa0-f51.google.com ([209.85.220.51]:64030 "EHLO mail-pa0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758616Ab3D3BAa (ORCPT ); Mon, 29 Apr 2013 21:00:30 -0400 Message-ID: <517F17AB.5070100@linaro.org> Date: Mon, 29 Apr 2013 18:00:27 -0700 From: John Stultz User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130329 Thunderbird/17.0.5 MIME-Version: 1.0 To: Thomas Gleixner CC: LKML , Ingo Molnar , Magnus Damm Subject: Re: [patch 05/15] clocksource: Allow clocksource select to skip current clocksource References: <20130425142452.908423538@linutronix.de> <20130425143435.834965397@linutronix.de> In-Reply-To: <20130425143435.834965397@linutronix.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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