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

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

[Thomas Gleixner]

On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> Adds a generic sched_clock, along with a boot time override for the scheduler
> clocksource (sched_clocksource).  Hopefully the config option would eventually
> be removed.

Again, this belongs into the clocksource code and not into sched.c

Your patch series scatters clock source related code snippets all over
the place. This becomes simply a maintenance nightmare.

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 11:55 +0200, Thomas Gleixner wrote:
> On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > Adds a generic sched_clock, along with a boot time override for the scheduler
> > clocksource (sched_clocksource).  Hopefully the config option would eventually
> > be removed.
> 
> Again, this belongs into the clocksource code and not into sched.c
> 
> Your patch series scatters clock source related code snippets all over
> the place. This becomes simply a maintenance nightmare.

It's an API, it's suppose to be used in different places.  

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 07:45 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 11:55 +0200, Thomas Gleixner wrote:
> > On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > > Adds a generic sched_clock, along with a boot time override for the scheduler
> > > clocksource (sched_clocksource).  Hopefully the config option would eventually
> > > be removed.
> > 
> > Again, this belongs into the clocksource code and not into sched.c
> > 
> > Your patch series scatters clock source related code snippets all over
> > the place. This becomes simply a maintenance nightmare.
> 
> It's an API, it's suppose to be used in different places.  

Err. clocksource is an API and it is supposed to be at a place, where
clocksource code is.

sched_clock is a user of the clocksource API and there is absolutely no
reason to put that into sched.c

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 16:51 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 07:45 -0700, Daniel Walker wrote:
> > On Sun, 2006-10-08 at 11:55 +0200, Thomas Gleixner wrote:
> > > On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > > > Adds a generic sched_clock, along with a boot time override for the scheduler
> > > > clocksource (sched_clocksource).  Hopefully the config option would eventually
> > > > be removed.
> > > 
> > > Again, this belongs into the clocksource code and not into sched.c
> > > 
> > > Your patch series scatters clock source related code snippets all over
> > > the place. This becomes simply a maintenance nightmare.
> > 
> > It's an API, it's suppose to be used in different places.  
> 
> Err. clocksource is an API and it is supposed to be at a place, where
> clocksource code is.

Agreed on this.

> sched_clock is a user of the clocksource API and there is absolutely no
> reason to put that into sched.c

We could do something like kernel/time/sched_clock.c ? I really don't
like the idea of stuff sched_clock() , and timekeeping back into
clocksource.
 
Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 08:32 -0700, Daniel Walker wrote:
> > sched_clock is a user of the clocksource API and there is absolutely no
> > reason to put that into sched.c
> 
> We could do something like kernel/time/sched_clock.c ? I really don't
> like the idea of stuff sched_clock() , and timekeeping back into
> clocksource.

Do we really want 3 files which each has 50 lines of code ? Although
it's better than pushing that stuff into sched.c and timer.c. timer.c is
enough mess and we really do not want to add more.

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 17:46 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 08:32 -0700, Daniel Walker wrote:
> > > sched_clock is a user of the clocksource API and there is absolutely no
> > > reason to put that into sched.c
> > 
> > We could do something like kernel/time/sched_clock.c ? I really don't
> > like the idea of stuff sched_clock() , and timekeeping back into
> > clocksource.
> 
> Do we really want 3 files which each has 50 lines of code ? Although
> it's better than pushing that stuff into sched.c and timer.c. timer.c is
> enough mess and we really do not want to add more.

I think John's plan, and my plan, is to move the generic time bits, plus
the timekeeping specific clocksource stuff, into a different file.
kernel/timer.c shouldn't have that stuff anyway. That's about 250 lines,
the part of that which is due to the clocksource is about 100 lines.

Are you thinking that this interface is only likely to have one or two
users?

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 09:06 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 17:46 +0200, Thomas Gleixner wrote:
> > On Sun, 2006-10-08 at 08:32 -0700, Daniel Walker wrote:
> > > > sched_clock is a user of the clocksource API and there is absolutely no
> > > > reason to put that into sched.c
> > > 
> > > We could do something like kernel/time/sched_clock.c ? I really don't
> > > like the idea of stuff sched_clock() , and timekeeping back into
> > > clocksource.
> > 
> > Do we really want 3 files which each has 50 lines of code ? Although
> > it's better than pushing that stuff into sched.c and timer.c. timer.c is
> > enough mess and we really do not want to add more.
> 
> I think John's plan, and my plan, is to move the generic time bits, plus
> the timekeeping specific clocksource stuff, into a different file.
> kernel/timer.c shouldn't have that stuff anyway. That's about 250 lines,
> the part of that which is due to the clocksource is about 100 lines.
> 
> Are you thinking that this interface is only likely to have one or two
> users?

Probably. Anyway, can we just keep the stuff in clocksource.c right
now ?

The sched stuff is really not worth a seperate file and the timekeeping
bits might move into kernel/time/timeofday.c once everone is happy to
move the timekeeping code out of timer.c

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 18:19 +0200, Thomas Gleixner wrote:

> Probably. Anyway, can we just keep the stuff in clocksource.c right
> now ?

I suppose I can move the sched_clock stuff.

> The sched stuff is really not worth a seperate file and the timekeeping
> bits might move into kernel/time/timeofday.c once everone is happy to
> move the timekeeping code out of timer.c

Ok. 

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 09:27 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 18:19 +0200, Thomas Gleixner wrote:
> 
> > Probably. Anyway, can we just keep the stuff in clocksource.c right
> > now ?
> 
> I suppose I can move the sched_clock stuff.

And keep the code you wanted to move into timer.c too. 

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 18:54 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 09:27 -0700, Daniel Walker wrote:
> > On Sun, 2006-10-08 at 18:19 +0200, Thomas Gleixner wrote:
> > 
> > > Probably. Anyway, can we just keep the stuff in clocksource.c right
> > > now ?
> > 
> > I suppose I can move the sched_clock stuff.
> 
> And keep the code you wanted to move into timer.c too. 

I'm not moving the kernel/timer.c clocksource user back into
kernel/time/clocksource.c . That code completely belongs with the
generic time of day changes. The code is directly coupled, and in fact
it improves the timekeeping clock switching code to have it that way.

I'd be happy to create kernel/time/timeofday.c though .

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 10:26 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 18:54 +0200, Thomas Gleixner wrote:
> > On Sun, 2006-10-08 at 09:27 -0700, Daniel Walker wrote:
> > > On Sun, 2006-10-08 at 18:19 +0200, Thomas Gleixner wrote:
> > > 
> > > > Probably. Anyway, can we just keep the stuff in clocksource.c right
> > > > now ?
> > > 
> > > I suppose I can move the sched_clock stuff.
> > 
> > And keep the code you wanted to move into timer.c too. 
> 
> I'm not moving the kernel/timer.c clocksource user back into
> kernel/time/clocksource.c . That code completely belongs with the
> generic time of day changes. The code is directly coupled, and in fact
> it improves the timekeeping clock switching code to have it that way.

I don't see any reason, why it must be added to timer.c. You can achieve
the same result with calling the code outside, except that the compiler
might miss some inline optimization. The switch clock code is not a
hotpath and so it does not matter whether it is called here or there.

> I'd be happy to create kernel/time/timeofday.c though .

Then please get an agreement with John and Roman, so we can move it now.

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 20:45 +0200, Thomas Gleixner wrote:

> > I'm not moving the kernel/timer.c clocksource user back into
> > kernel/time/clocksource.c . That code completely belongs with the
> > generic time of day changes. The code is directly coupled, and in fact
> > it improves the timekeeping clock switching code to have it that way.
> 
> I don't see any reason, why it must be added to timer.c. You can achieve
> the same result with calling the code outside, except that the compiler
> might miss some inline optimization. The switch clock code is not a
> hotpath and so it does not matter whether it is called here or there.

It wouldn't be as clean to integrate the two. The hotpath is improved
(which is what I was referring too above.)

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 20:45 +0200, Thomas Gleixner wrote:
> 
> > > I'm not moving the kernel/timer.c clocksource user back into
> > > kernel/time/clocksource.c . That code completely belongs with the
> > > generic time of day changes. The code is directly coupled, and in fact
> > > it improves the timekeeping clock switching code to have it that way.
> > 
> > I don't see any reason, why it must be added to timer.c. You can achieve
> > the same result with calling the code outside, except that the compiler
> > might miss some inline optimization. The switch clock code is not a
> > hotpath and so it does not matter whether it is called here or there.
> 
> It wouldn't be as clean to integrate the two. The hotpath is improved
> (which is what I was referring too above.)

Sorry, where is which hotpath improved ?

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 23:21 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> > On Sun, 2006-10-08 at 20:45 +0200, Thomas Gleixner wrote:
> > 
> > > > I'm not moving the kernel/timer.c clocksource user back into
> > > > kernel/time/clocksource.c . That code completely belongs with the
> > > > generic time of day changes. The code is directly coupled, and in fact
> > > > it improves the timekeeping clock switching code to have it that way.
> > > 
> > > I don't see any reason, why it must be added to timer.c. You can achieve
> > > the same result with calling the code outside, except that the compiler
> > > might miss some inline optimization. The switch clock code is not a
> > > hotpath and so it does not matter whether it is called here or there.
> > 
> > It wouldn't be as clean to integrate the two. The hotpath is improved
> > (which is what I was referring too above.)
> 
> Sorry, where is which hotpath improved ?

The hotpath in update_wall_time() kernel/timer.c which involves clock
switching.

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 14:33 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 23:21 +0200, Thomas Gleixner wrote:
> > On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> > > On Sun, 2006-10-08 at 20:45 +0200, Thomas Gleixner wrote:
> > > 
> > > > > I'm not moving the kernel/timer.c clocksource user back into
> > > > > kernel/time/clocksource.c . That code completely belongs with the
> > > > > generic time of day changes. The code is directly coupled, and in fact
> > > > > it improves the timekeeping clock switching code to have it that way.
> > > > 
> > > > I don't see any reason, why it must be added to timer.c. You can achieve
> > > > the same result with calling the code outside, except that the compiler
> > > > might miss some inline optimization. The switch clock code is not a
> > > > hotpath and so it does not matter whether it is called here or there.
> > > 
> > > It wouldn't be as clean to integrate the two. The hotpath is improved
> > > (which is what I was referring too above.)
> > 
> > Sorry, where is which hotpath improved ?
> 
> The hotpath in update_wall_time() kernel/timer.c which involves clock
> switching.

And why the heck does this require to move _clocksource_ related code
including sysfs hackery into timer.c ? Your improvement works with
extern code as well.

	tglx

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

[Daniel Walker]

On Sun, 2006-10-08 at 23:41 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 14:33 -0700, Daniel Walker wrote:
> > On Sun, 2006-10-08 at 23:21 +0200, Thomas Gleixner wrote:
> > > On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> > > > On Sun, 2006-10-08 at 20:45 +0200, Thomas Gleixner wrote:
> > > > 
> > > > > > I'm not moving the kernel/timer.c clocksource user back into
> > > > > > kernel/time/clocksource.c . That code completely belongs with the
> > > > > > generic time of day changes. The code is directly coupled, and in fact
> > > > > > it improves the timekeeping clock switching code to have it that way.
> > > > > 
> > > > > I don't see any reason, why it must be added to timer.c. You can achieve
> > > > > the same result with calling the code outside, except that the compiler
> > > > > might miss some inline optimization. The switch clock code is not a
> > > > > hotpath and so it does not matter whether it is called here or there.
> > > > 
> > > > It wouldn't be as clean to integrate the two. The hotpath is improved
> > > > (which is what I was referring too above.)
> > > 
> > > Sorry, where is which hotpath improved ?
> > 
> > The hotpath in update_wall_time() kernel/timer.c which involves clock
> > switching.
> 
> And why the heck does this require to move _clocksource_ related code
> including sysfs hackery into timer.c ? Your improvement works with
> extern code as well.

There are no clocksource internals added by me. There is a exposed
clocksource API which is all that is used.

The design of the original clocksource interface was specific for
timekeeping. What I did was modified it to be used by more than just
timekeeping.

If I add tons of externs there and cram all that into clocksource.c ,
that would just be a mess. Then we're tending back to the original
clocksource design when it's designed just for time keeping.

Daniel

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

[Thomas Gleixner]

On Sun, 2006-10-08 at 15:08 -0700, Daniel Walker wrote:
> > And why the heck does this require to move _clocksource_ related code
> > including sysfs hackery into timer.c ? Your improvement works with
> > extern code as well.
> 
> There are no clocksource internals added by me. There is a exposed
> clocksource API which is all that is used.

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.

> The design of the original clocksource interface was specific for
> timekeeping. What I did was modified it to be used by more than just
> timekeeping.
> 
> If I add tons of externs there and cram all that into clocksource.c ,
> that would just be a mess. Then we're tending back to the original
> clocksource design when it's designed just for time keeping.

Tons of externs for the optimization of the clock source switch? Sorry,
I'm not following. 

The clock source switch happens once or twice during bootup and the
replacement of a call with an atomic check does not in any way
legitimate the move of code into timer.c. The number of cylces saved is
not impressing. Moving the clock source switch out of that path at all
would be progress and save real cylces. 
.
The maintainability of code has to weighed carefully against some
obscure cylce savings.

	tglx

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

[Daniel Walker]

On Mon, 2006-10-09 at 00:35 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 15:08 -0700, Daniel Walker wrote:
> > > And why the heck does this require to move _clocksource_ related code
> > > including sysfs hackery into timer.c ? Your improvement works with
> > > extern code as well.
> > 
> > There are no clocksource internals added by me. There is a exposed
> > clocksource API which is all that is used.
> 
> 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.

htimers is intrinsically connected to time keeping, but they aren't
housed in the same files. Just because one subsystem uses another
doesn't mean they shouldn't be organized/optimized/maintained on their
own.

> > The design of the original clocksource interface was specific for
> > timekeeping. What I did was modified it to be used by more than just
> > timekeeping.
> > 
> > If I add tons of externs there and cram all that into clocksource.c ,
> > that would just be a mess. Then we're tending back to the original
> > clocksource design when it's designed just for time keeping.
> 
> Tons of externs for the optimization of the clock source switch? Sorry,
> I'm not following. 
> 
> The clock source switch happens once or twice during bootup and the
> replacement of a call with an atomic check does not in any way
> legitimate the move of code into timer.c. The number of cylces saved is
> not impressing. Moving the clock source switch out of that path at all
> would be progress and save real cylces. 
> .

Moving the code is a separation issue. I _could_ move it back, but you
haven't convinced me that it wins anything. In fact you've convinced me
that it would be a step back instead of forward.

> The maintainability of code has to weighed carefully against some
> obscure cylce savings.

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

Daniel

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

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

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > Adds a generic sched_clock, along with a boot time override for the 
> > scheduler clocksource (sched_clocksource).  Hopefully the config 
> > option would eventually be removed.
> 
> Again, this belongs into the clocksource code and not into sched.c
> 
> Your patch series scatters clock source related code snippets all over 
> the place. This becomes simply a maintenance nightmare.

seconded.

	Ingo