lkml.org 
[lkml]   [2007]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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/

    \
     
     \ /
      Last update: 2007-01-31 11:51    [W:5.865 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site