Re: [PATCH 10/23] clocksource: remove update_callback

[Daniel Walker]

On Wed, 2007-01-31 at 11:46 +0100, Ingo Molnar wrote:
> * Daniel Walker <dwalker@xxxxxxxxxx> wrote:
> 
> > Uses the block notifier to replace the functionality of 
> > update_callback(). update_callback() was a special case specifically 
> > for the tsc, but including it in the clocksource structure duplicated 
> > it needlessly for other clocks.
> 
> Firstly, it think it should be mentioned that Thomas' queue already does 
> this, in clocksource-remove-the-update-callback.patch (hence he should 
> have been Cc:-ed). Your queue 'drops' Thomas' patch then redoes it here 
> without mentioning that this is another version of what is in Thomas's 
> queue. So we get this situation:
> 
>    clocksource-remove-the-update-callback.patch
>    drop-clocksource-remove-the-update-callback.patch
>    clocksource_remove_update_callback.patch
> 
> that all flip-flops the same thing.

To be clear this change has exists for a very long time, long before
Thomas implemented it .. 

> Secondly, your patch seems to do other changes as well:
> 
> > @@ -179,6 +172,7 @@ int recalibrate_cpu_khz(void)
> >  	if (cpu_has_tsc) {
> >  		cpu_khz = calculate_cpu_khz();
> >  		tsc_khz = cpu_khz;
> > +		mark_tsc_unstable();
> >  		cpu_data[0].loops_per_jiffy =
> >  			cpufreq_scale(cpu_data[0].loops_per_jiffy,
> >  					cpu_khz_old, cpu_khz);
> 
> this adds a new event to a place that didnt have it before. (If this is 
> fixing up an initialization artifact then that needs a comment at 
> least.)

Alright .. Later on I talk about clock frequency recalculation which
pertains to this change .. 

> plus:
> 
> >  struct clocksource *clock = &clocksource_jiffies;
> > +atomic_t clock_recalc_interval = ATOMIC_INIT(0);
> 
> is not mentioned in the changelog. It's also needlessly global. 
> Furthermore, it seems to be a rather unclean method of passing 
> information from clocksource_callback() into change_clocksource():

It's a bisect artifact .. It's eventually dropped in a latter patch .

> > @@ -176,8 +177,9 @@ static int change_clocksource(void)
> >  		printk(KERN_INFO "Time: %s clocksource has been installed.\n",
> >  		       clock->name);
> >  		return 1;
> > -	} else if (clock->update_callback) {
> > -		return clock->update_callback();
> > +	} else if (unlikely(atomic_read(&clock_recalc_interval))) {
> > +		atomic_set(&clock_recalc_interval, 0);
> > +		return 1;
> 
> that's quite bad: you lost an information passing facility by going to a 
> notifier, and you try to work it around via a global atomic variable. 
> Which also looks quite racy as well.
> 
> The clean solution is i think what Thomas did: he calls straight into 
> clocksource_change_rating(). And look at Thomas' patch:

The information passing was purposefully dropped .. It was used (in
part) to recalculate the frequency of a clock , but after discussing it
with John he suggested we drop the frequency recalculation entirely. So
the information passing should no longer be needed.

Daniel

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/