Re: + clocksource-increase-initcall-priority.patch added to -mm tree

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> Since it's likely that this interface would get used during bootup I moved all
> the clocksource registration into the postcore initcall.  This also eliminated
> some clocksource shuffling during bootup.

We had the init call in postcore already. John moved it to module init
to eliminate trouble with unsynced / unstable TSCs, IIRC.

John, can you please comment on this.

	tglx
 

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 10:06 +0200, Thomas Gleixner wrote:
> On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > Since it's likely that this interface would get used during bootup I moved all
> > the clocksource registration into the postcore initcall.  This also eliminated
> > some clocksource shuffling during bootup.
> 
> We had the init call in postcore already. John moved it to module init
> to eliminate trouble with unsynced / unstable TSCs, IIRC.
> 
> John, can you please comment on this.

It also breaks pmtimer.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 12:19 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 10:06 +0200, Thomas Gleixner wrote:
> > On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > > Since it's likely that this interface would get used during bootup I moved all
> > > the clocksource registration into the postcore initcall.  This also eliminated
> > > some clocksource shuffling during bootup.
> > 
> > We had the init call in postcore already. John moved it to module init
> > to eliminate trouble with unsynced / unstable TSCs, IIRC.
> > 
> > John, can you please comment on this.
> 
> It also breaks pmtimer.

OGAWA reported this already. It breaks the case when there is a verified
read needed, instead of the fast read. I'll fix it.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 07:50 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 12:19 +0200, Thomas Gleixner wrote:
> > On Sun, 2006-10-08 at 10:06 +0200, Thomas Gleixner wrote:
> > > On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > > > Since it's likely that this interface would get used during bootup I moved all
> > > > the clocksource registration into the postcore initcall.  This also eliminated
> > > > some clocksource shuffling during bootup.
> > > 
> > > We had the init call in postcore already. John moved it to module init
> > > to eliminate trouble with unsynced / unstable TSCs, IIRC.
> > > 
> > > John, can you please comment on this.
> > 
> > It also breaks pmtimer.
> 
> OGAWA reported this already. It breaks the case when there is a verified
> read needed, instead of the fast read. I'll fix it.

I'd like to know, why we need to move that and you did not explain _why_
it is likely that it is used during bootup.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 16:53 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 07:50 -0700, Daniel Walker wrote:
> > On Sun, 2006-10-08 at 12:19 +0200, Thomas Gleixner wrote:
> > > On Sun, 2006-10-08 at 10:06 +0200, Thomas Gleixner wrote:
> > > > On Fri, 2006-10-06 at 18:53 -0700, akpm@osdl.org wrote:
> > > > > Since it's likely that this interface would get used during bootup I moved all
> > > > > the clocksource registration into the postcore initcall.  This also eliminated
> > > > > some clocksource shuffling during bootup.
> > > > 
> > > > We had the init call in postcore already. John moved it to module init
> > > > to eliminate trouble with unsynced / unstable TSCs, IIRC.
> > > > 
> > > > John, can you please comment on this.
> > > 
> > > It also breaks pmtimer.
> > 
> > OGAWA reported this already. It breaks the case when there is a verified
> > read needed, instead of the fast read. I'll fix it.
> 
> I'd like to know, why we need to move that and you did not explain _why_
> it is likely that it is used during bootup.

If the clocksources are registered at the same time as the clocksource
users then you end up with users frequently switching clocks during boot
up. The original clocksource code solved this by not allowing a real
clocksource lookup until after the system fully booted.

However, if you put all the clocksources into postcore initcall, with
that being known in advance, and all the users are in lower priority
initcalls then you don't need extra code to prevent churn during bootup.

The reason that I think this will get used during boot up is because
some of the target users will be instrumentation, and (my prediction
anyway) is that some will need to use the interface early. Still even
postcore may not be early enough.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 08:45 -0700, Daniel Walker wrote:
> > I'd like to know, why we need to move that and you did not explain _why_
> > it is likely that it is used during bootup.
> 
> If the clocksources are registered at the same time as the clocksource
> users then you end up with users frequently switching clocks during boot
> up. The original clocksource code solved this by not allowing a real
> clocksource lookup until after the system fully booted.

No, that was not the reason. It does not hurt anything, when the
clocksource changes during bootup. You simply replace one read out
device by another one.

The reason was that we wanted to select clocksources as late as
possible, as we wanted to have late init clocksources ready and TSC
problems outruled.

> However, if you put all the clocksources into postcore initcall, with
> that being known in advance, and all the users are in lower priority
> initcalls then you don't need extra code to prevent churn during bootup.
> 
> The reason that I think this will get used during boot up is because
> some of the target users will be instrumentation, and (my prediction
> anyway) is that some will need to use the interface early. Still even
> postcore may not be early enough.

Early bootup Instrumentation is really not a good argument to make that
fragile time related stuff even more complex. There is no problem to
register reliable clocksources in early bootup, but do not make this
mandatory. Not every system is an ARM SoC, where you can and must rely
on the one source which is available usually right when the CPU comes
up.

Early bootup instrumentation can very well go with none or coarse
grained time information, if there is a need to have early boot
intstrumentation at all.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 17:58 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 08:45 -0700, Daniel Walker wrote:
> > > I'd like to know, why we need to move that and you did not explain _why_
> > > it is likely that it is used during bootup.
> > 
> > If the clocksources are registered at the same time as the clocksource
> > users then you end up with users frequently switching clocks during boot
> > up. The original clocksource code solved this by not allowing a real
> > clocksource lookup until after the system fully booted.
> 
> No, that was not the reason. It does not hurt anything, when the
> clocksource changes during bootup. You simply replace one read out
> device by another one.
> 
> The reason was that we wanted to select clocksources as late as
> possible, as we wanted to have late init clocksources ready and TSC
> problems outruled.

I think we're going to need to wait for John to comment. His last
comment on this patch was in agreement as long as it solved the churn
issue (as of Aug. 4 on LKML).

> > However, if you put all the clocksources into postcore initcall, with
> > that being known in advance, and all the users are in lower priority
> > initcalls then you don't need extra code to prevent churn during bootup.
> > 
> > The reason that I think this will get used during boot up is because
> > some of the target users will be instrumentation, and (my prediction
> > anyway) is that some will need to use the interface early. Still even
> > postcore may not be early enough.
> 
> Early bootup Instrumentation is really not a good argument to make that
> fragile time related stuff even more complex. There is no problem to
> register reliable clocksources in early bootup, but do not make this
> mandatory. Not every system is an ARM SoC, where you can and must rely
> on the one source which is available usually right when the CPU comes
> up.

It's not mandatory, it's just preferred.. As I said above, to avoid
churn. I don't like the churn at boot up, and I tried to make sure there
was none added in the patch set. 

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 09:18 -0700, Daniel Walker wrote:
> > Early bootup Instrumentation is really not a good argument to make that
> > fragile time related stuff even more complex. There is no problem to
> > register reliable clocksources in early bootup, but do not make this
> > mandatory. Not every system is an ARM SoC, where you can and must rely
> > on the one source which is available usually right when the CPU comes
> > up.
> 
> It's not mandatory, it's just preferred.. As I said above, to avoid
> churn. I don't like the churn at boot up, and I tried to make sure there
> was none added in the patch set. 

What churn at bootup ? The clocksources _can_ be switched and it does
not matter, when this is done. We had the trouble with the early
registration a couple of month ago, and there is no reason to
reintroduce it. On systems which have exactly one clocksource, you can
register them early in bootup, but please do not touch the x86 setup for
no good reason.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 18:52 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 09:18 -0700, Daniel Walker wrote:
> > > Early bootup Instrumentation is really not a good argument to make that
> > > fragile time related stuff even more complex. There is no problem to
> > > register reliable clocksources in early bootup, but do not make this
> > > mandatory. Not every system is an ARM SoC, where you can and must rely
> > > on the one source which is available usually right when the CPU comes
> > > up.
> > 
> > It's not mandatory, it's just preferred.. As I said above, to avoid
> > churn. I don't like the churn at boot up, and I tried to make sure there
> > was none added in the patch set. 
> 
> What churn at bootup ? The clocksources _can_ be switched and it does
> not matter, when this is done. We had the trouble with the early
> registration a couple of month ago, and there is no reason to
> reintroduce it. On systems which have exactly one clocksource, you can
> register them early in bootup, but please do not touch the x86 setup for
> no good reason.

There was a special case inside kernel/time/clocksource.c to prevent
clock switching during boot up. If you remove that (which I have) then
you will end up with clock switching happening a few times during bootup
(whenever a new highest rated clock is registered), that's the churn I'm
referring to.

The churn is not optimal. I've used postcore to prevent it, and make the
API usable earlier. So there is a reason for the change. 

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 10:17 -0700, Daniel Walker wrote:
> There was a special case inside kernel/time/clocksource.c to prevent
> clock switching during boot up. If you remove that (which I have) then
> you will end up with clock switching happening a few times during bootup
> (whenever a new highest rated clock is registered), that's the churn I'm
> referring to.
> 
> The churn is not optimal. I've used postcore to prevent it, and make the
> API usable earlier. So there is a reason for the change. 

Yes, a bad one. The disabling had a totally different reason and you are
not listening at all.

You just introduce a problem again, because it does not happen on your
machines and you think, that some not yet available instrumentation code
needs high resolution time stamps. 

The reason why this was delayed into late boot is simply that the
unstable, unsynchronized TSC's made way too much trouble and the pmtimer
can not be initialized early.

I'm not going to accept that. Your change might work on 5 machines you
have tested on, but we start over with the same breakage we solved
already. 

This early boot instrumentation code can work with low resolution time
information quite well and none of the boot code does need any high res
time information. Boot code is different from a running system and has
different requirements. 

This is a solution for a nonexisting problem, which just brings back
already solved ones.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 21:03 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 10:17 -0700, Daniel Walker wrote:
> > There was a special case inside kernel/time/clocksource.c to prevent
> > clock switching during boot up. If you remove that (which I have) then
> > you will end up with clock switching happening a few times during bootup
> > (whenever a new highest rated clock is registered), that's the churn I'm
> > referring to.
> > 
> > The churn is not optimal. I've used postcore to prevent it, and make the
> > API usable earlier. So there is a reason for the change. 
> 
> Yes, a bad one. The disabling had a totally different reason and you are
> not listening at all.

I am listening, Thomas .. 

> You just introduce a problem again, because it does not happen on your
> machines and you think, that some not yet available instrumentation code
> needs high resolution time stamps. 

Ok.

> The reason why this was delayed into late boot is simply that the
> unstable, unsynchronized TSC's made way too much trouble and the pmtimer
> can not be initialized early.
> 
> I'm not going to accept that. Your change might work on 5 machines you
> have tested on, but we start over with the same breakage we solved
> already. 

Your going to have to explain the breakage further. I don't recall
seeing any discussion on this.

> This early boot instrumentation code can work with low resolution time
> information quite well and none of the boot code does need any high res
> time information. Boot code is different from a running system and has
> different requirements. 
> 
> This is a solution for a nonexisting problem, which just brings back
> already solved ones.

There is at least one existing problem which it does solve. How about we
discuss the early TSC boot breakage, which you mention above, so I can
properly fix this situation.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 13:39 -0700, Daniel Walker wrote:
> > The reason why this was delayed into late boot is simply that the
> > unstable, unsynchronized TSC's made way too much trouble and the pmtimer
> > can not be initialized early.
> > 
> > I'm not going to accept that. Your change might work on 5 machines you
> > have tested on, but we start over with the same breakage we solved
> > already. 
> 
> Your going to have to explain the breakage further. I don't recall
> seeing any discussion on this.

See above, but I'm happy to copy it.
> > The reason why this was delayed into late boot is simply that the
> > unstable, unsynchronized TSC's made way too much trouble and the pmtimer
> > can not be initialized early.

I don't have the bug reports handy, but they are in the LKML archives.

> > This early boot instrumentation code can work with low resolution time
> > information quite well and none of the boot code does need any high res
> > time information. Boot code is different from a running system and has
> > different requirements. 
> > 
> > This is a solution for a nonexisting problem, which just brings back
> > already solved ones.
> 
> There is at least one existing problem which it does solve. 

Which one exactly? I'm not aware of a problem with the existing code at
all.

> How about we
> discuss the early TSC boot breakage, which you mention above, so I can
> properly fix this situation.

See above. Also it interferes probably with the highres/dyntick code, as
we might switch over way too early. Have not looked into that yet.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 22:52 +0200, Thomas Gleixner wrote:

> See above, but I'm happy to copy it.
> > > The reason why this was delayed into late boot is simply that the
> > > unstable, unsynchronized TSC's made way too much trouble and the pmtimer
> > > can not be initialized early.
> 
> I don't have the bug reports handy, but they are in the LKML archives.

Like I said I need more detail. I'm assuming John will know what your
talking about, or do you have a rough time frame?

> > > This early boot instrumentation code can work with low resolution time
> > > information quite well and none of the boot code does need any high res
> > > time information. Boot code is different from a running system and has
> > > different requirements. 
> > > 
> > > This is a solution for a nonexisting problem, which just brings back
> > > already solved ones.
> > 
> > There is at least one existing problem which it does solve. 
> 
> Which one exactly? I'm not aware of a problem with the existing code at
> all.

Clock shuffling.

> > How about we
> > discuss the early TSC boot breakage, which you mention above, so I can
> > properly fix this situation.
> 
> See above. Also it interferes probably with the highres/dyntick code, as
> we might switch over way too early. Have not looked into that yet.

I would be surprised if it caused problems. The clock switching is done
at the same time as always. I tried not to alter generic time
functionality in any way.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> > 
> > Which one exactly? I'm not aware of a problem with the existing code at
> > all.
> 
> Clock shuffling.

What's the problem with that ? It replaces clocks. Where _is_ the
problem ?

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 23:20 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> > > 
> > > Which one exactly? I'm not aware of a problem with the existing code at
> > > all.
> > 
> > Clock shuffling.
> 
> What's the problem with that ? It replaces clocks. Where _is_ the
> problem ?

The problem is that it's not optimal to have clocks switching furiously.
This is something John notes as an issue in the unchanged
kernel/time/clocksource.c file.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 14:31 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 23:20 +0200, Thomas Gleixner wrote:
> > On Sun, 2006-10-08 at 14:15 -0700, Daniel Walker wrote:
> > > > 
> > > > Which one exactly? I'm not aware of a problem with the existing code at
> > > > all.
> > > 
> > > Clock shuffling.
> > 
> > What's the problem with that ? It replaces clocks. Where _is_ the
> > problem ?
> 
> The problem is that it's not optimal to have clocks switching furiously.
> This is something John notes as an issue in the unchanged
> kernel/time/clocksource.c file.

I don't see that behaviour on my machines and nobody complains about
that. I don't care about stale comments. Point me to a bug report
instead of your perception of what's optimal and not.

Working is not necessary optimal, but your vision of optimal is not
necessarily working either.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Sun, 2006-10-08 at 23:38 +0200, Thomas Gleixner wrote:

> 
> I don't see that behaviour on my machines and nobody complains about
> that. I don't care about stale comments. Point me to a bug report
> instead of your perception of what's optimal and not.

Let both do this. Lets discuss empirical behavior. Otherwise we aren't
making any progress. 

> Working is not necessary optimal, but your vision of optimal is not
> necessarily working either.

I'm looking for the TSC issue you mentioned.. I tested it as best I
could, so I need more information to say it's not working.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 15:13 -0700, Daniel Walker wrote:
> On Sun, 2006-10-08 at 23:38 +0200, Thomas Gleixner wrote:
> 
> > 
> > I don't see that behaviour on my machines and nobody complains about
> > that. I don't care about stale comments. Point me to a bug report
> > instead of your perception of what's optimal and not.
> 
> Let both do this. Lets discuss empirical behavior. Otherwise we aren't
> making any progress. 

Go, grep the LKML archives and let those who had problems test your
modifications. Come back when they confirm that it does not change
anything.

You want to change behaviour of the current code, so it's your job to
verify that it does not break anything.

I have been there and done that with the ARM interrupt code
http://www.linutronix.de/index.php?page=testing

I know what I'm talking about.

	tglx

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Daniel Walker]

On Mon, 2006-10-09 at 00:53 +0200, Thomas Gleixner wrote:
> On Sun, 2006-10-08 at 15:13 -0700, Daniel Walker wrote:
> > On Sun, 2006-10-08 at 23:38 +0200, Thomas Gleixner wrote:
> > 
> > > 
> > > I don't see that behaviour on my machines and nobody complains about
> > > that. I don't care about stale comments. Point me to a bug report
> > > instead of your perception of what's optimal and not.
> > 
> > Let both do this. Lets discuss empirical behavior. Otherwise we aren't
> > making any progress. 
> 
> Go, grep the LKML archives and let those who had problems test your
> modifications. Come back when they confirm that it does not change
> anything.

This is why I want this to go into -mm .. To flush out the corner cases
that it's impossible for me to find on my own.

> You want to change behaviour of the current code, so it's your job to
> verify that it does not break anything.

I have, within my ability to do so.

> I have been there and done that with the ARM interrupt code
> http://www.linutronix.de/index.php?page=testing
> 
> I know what I'm talking about.

I think you know what your talking about, and I respect your opinion.. I
think you have some points, that I will follow up on. However most of
what your saying comes off to me like, "Reverse _all_ your changes!" ,
"Makes this patch set go away for 2 months", and I don't understand
where that is coming from.

Daniel

Re: + clocksource-increase-initcall-priority.patch added to -mm tree

[Thomas Gleixner]

On Sun, 2006-10-08 at 16:35 -0700, Daniel Walker wrote:
> I think you know what your talking about, and I respect your opinion.. I
> think you have some points, that I will follow up on. However most of
> what your saying comes off to me like, "Reverse _all_ your changes!" ,
> "Makes this patch set go away for 2 months", and I don't understand
> where that is coming from.

I did nowhere say, that you should revert _ALL_ your changes ! If you
need 2 month to fixup the issues I pointed at , I can't help it.

	tglx