Re: + clocksource-add-generic-sched_clock.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 16:01 -0700, Daniel Walker wrote:
> > You move tons of code into timer.c, which does not belong there.
> > clocksource is a different thing than timekeeping. timekeeping makes use
> > of clocksources, and your extra layer of timekeeping_clocksource API
> > does not change that at all. What you call abstraction is just an
> > artificial extra layer, as it is intrinsically tied to the clocksource
> > core.
> 
> I think that code does belong there. Yes clocksources and timekeeping
> are different. Which is the point of the patch set.

And I have to accept that you think, that the code belongs there ?

> It's not less maintainable now, or if it is your going to have to be a
> lot more specific.

In order to satisfy your idea of abstraction, you add glue code to make
it work:

clocksource.c:
> +int clocksource_sysfs_register(struct sysdev_attribute * attr)
> +{
> +       return sysdev_create_file(&device_clocksource, attr);
> +}
> +

timer.c:
> +
> +#ifdef CONFIG_GENERIC_TIME
> +       clocksource_sysfs_register(&attr_timeofday_clocksource);

Abstractions are good in general, but this is artificial for no benefit.


You claim, that you optmize update_wll_time()

>         /* check to see if there is a new clocksource to use */
> -       if (change_clocksource()) {
> +       if (unlikely(atomic_read(&clock_check))) {
> +
> +               /*
> +                * Switch to the new override clock, or the highest
> +                * rated clock.
> +                */
> +               if (*clock_override_name)
> +                       change_clocksource(clock_override_name);
> +               else
> +                       change_clocksource(NULL);
> +
>                 clock->error = 0;
>                 clock->xtime_nsec = 0;
>                 hrtimer_clock_notify();
>                 clocksource_calculate_interval(clock, tick_nsec);
> +
> +               /*
> +                * Remove the change signal
> +                */
> +               atomic_dec(&clock_check);
> +
>         }

Well, I guess I have some perceptional disturbance. What exactly is the
optimization ? Pushing clock_override_name into that code path ? As I
said before, the performance difference of change_clocksource() and
atomic_read() is not big enough to justify the mess you create all over
the place.

The whole notifier business is not necessary, if you keep the
clocksource related code in one place. 

You break sched_clock on x86 for no good reason.

	tglx


-
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/