On Wed, 2007-01-31 at 11:46 +0100, Ingo Molnar wrote: > * Daniel Walker <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/