Re: [patch 05/15] clocksource: Allow clocksource select to skip current clocksource

[Thomas Gleixner <tglx@linutronix.de>]

On Mon, 29 Apr 2013, John Stultz wrote:
> 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.

Yeah, I can do that.