Messages in this thread | | | Date | Wed, 31 Jan 2007 11:46:10 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH 10/23] clocksource: remove update_callback |
| |
* Daniel Walker <dwalker@mvista.com> 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.
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.)
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():
> @@ -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:
3 files changed, 17 insertions(+), 37 deletions(-)
versus yours:
5 files changed, 45 insertions(+), 42 deletions(-)
Thomas' looks definitely simpler to me.
ingo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |