On Fri, Jan 11 2008, Ingo Molnar wrote:
>
> (David, could you try the patch further below - does it fix bkltrace
> timestamps too?)
>
> * Jens Axboe wrote:
>
> > On Fri, Jan 11 2008, Ingo Molnar wrote:
> > >
> > > * Jens Axboe wrote:
> > >
> > > > > they are from the scheduler git tree (except the first debug patch),
> > > > > but queued up for v2.6.25 at the moment.
> > > >
> > > > So this means that blktrace will be broken with CONFIG_NO_HZ for
> > > > 2.6.24? That's clearly a regression.
> > >
> > > 64-bit CONFIG_NO_HZ is a new feature in v2.6.24. If it happens on
> > > 32-bit too and it didnt happen in v2.6.23 32-bit then it's a
> > > regression.
> >
> > If blktrace worked in 2.6.23 and it doesn't in 2.6.24 because of some
> > option that isn't immediately apparent, then it's a regression.
> > Period.
>
> not completely correct. CONFIG_NO_HZ is a default-disabled option that
> became newly available on 64-bit x86. So if NO_HZ does not completely
> work on 64-bit, and if 32-bit works fine - which we dont know yet (my
> guess would be that it's similarly broken on the same box) then it's not
> a regression.

Ingo, it doesn't matter if the option is disabled by default or not!
The fact is that functionality foo works in 2.6.23 and doesn't in 2.6.24
because of something unrelated. And that IS a regression, no matter what
kind of word play you are doing here :-)

> But even if it's not "technically" a regression, it's something we want
> to fix in .24 if we can, so i'm all with you Jens

That's good

> ktime_get() should have been used instead, which is a proper GTOD
> clocksource. The patch below implements this.

Will give it a whirl, it looks promising indeed and gets rid of the ugly
cpu sync stuff. What is the cost of ktime_get() compared to
sched_clock()?

--
Jens Axboe

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

On Fri, Jan 11 2008, Jens Axboe wrote:
> > ktime_get() should have been used instead, which is a proper GTOD
> > clocksource. The patch below implements this.
>
> Will give it a whirl, it looks promising indeed and gets rid of the ugly
> cpu sync stuff. What is the cost of ktime_get() compared to
> sched_clock()?

Works for me - will apply it right now, we can always evaluate any
potential performance impact later. It's more important to have a
functional blktrace in 2.6.24.

Thanks Ingo!

--
Jens Axboe

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

* Jens Axboe wrote:

> > > If blktrace worked in 2.6.23 and it doesn't in 2.6.24 because of
> > > some option that isn't immediately apparent, then it's a
> > > regression. Period.
> >
> > not completely correct. CONFIG_NO_HZ is a default-disabled option
> > that became newly available on 64-bit x86. So if NO_HZ does not
> > completely work on 64-bit, and if 32-bit works fine - which we dont
> > know yet (my guess would be that it's similarly broken on the same
> > box) then it's not a regression.
>
> Ingo, it doesn't matter if the option is disabled by default or not!
> The fact is that functionality foo works in 2.6.23 and doesn't in
> 2.6.24 because of something unrelated. And that IS a regression, no
> matter what kind of word play you are doing here :-)

argh, Jens. Really. I believe you stopped using your brain because
CONFIG_BKL_IO_TRACE=y is affected by this bug and apparently you've got
a weak spot for it

Think about it another way then, in the context of another, real kernel
feature we introduced in v2.6.24, namely CONFIG_CPU_IDLE=y:

- Fact: feature CONFIG_CPU_IDLE=y did not exist in 64-bit x86 in v2.6.23.
It has known bugs but they are not flagged 'regressions' (because the
feature did not exist before and the option is default-disabled) hence
the feature can stay. Some fixes to it are too dangerous or too
unproven and are delayed to v2.6.25.

and now apply the same rule to CONFIG_NO_HZ:

- Fact: feature CONFIG_NO_HZ=y did not exist in 64-bit x86 in v2.6.23.
It has known bugs but they are not flagged 'regressions' (because the
feature did not exist before and the option is default-disabled) hence
the feature can stay. Some fixes to it are too dangerous or too
unproven and are delayed to v2.6.25.

ok?

Yes, it's bad that this happened, and perhaps it _is_ a regression, but
not for the reason you claim. [ It might be a regression if 32-bit
blktrace has the same problem under NO_HZ for example _AND_ bkltrace
worked perfectly on the same box on v2.6.23. ]

Kernel regressions have a _strict_ definition: they mean "anything that
worked before will work in the future too". Not: "anything that worked
before will work in the future too if you enable random new non-default
kernel features".

[ If the latter was the criterium we'd probably never see new features
integrated! New stuff has bugs, because it's not nearly as well-tested
as older stuff. What matters is to 1) not turn it on by default if it
has known bugs 2) to always make positive progress on it, i.e.: to not
regress new features in future kernel releases. This way, in the ideal
case, we'll have a monotonic series towards a perfect, bug-free kernel
]

> > ktime_get() should have been used instead, which is a proper GTOD
> > clocksource. The patch below implements this.
>
> Will give it a whirl, it looks promising indeed and gets rid of the
> ugly cpu sync stuff. [...]

fantastic!

> [...] What is the cost of ktime_get() compared to sched_clock()?

compared to the costs of IO transfers it should be OK. It can be really
fast but in the worst-case it can be as slow as 3-6 usecs (when we use
the acpi_pm clocksource). If there's complaints then probably the
networking folks will yell first

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/

On Fri, 2008-01-11 at 11:29 +0100, Ingo Molnar wrote:
> (David, could you try the patch further below - does it fix bkltrace
> timestamps too?)

Jens already got back to you, but I can confirm it works here as well.
--
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office


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

On Fri, 2008-01-11 at 10:34 +0100, Ingo Molnar wrote:
> * David Dillow wrote:
>
> > Ingo, Thomas added as I think this is related to
> > sched.c:__update_rq_clock()'s checking for forward time warps.
>
> yep, we've got some fixes in this area. Do blktrace timestamps work fine
> in v2.6.23, with NOHZ?

Do you still want this tested, given that your ktime change works? If
so, it will likely be next week before I can devote some cycles to it --
it'll take installing a new distro on the machines.
--
Dave Dillow
National Center for Computational Science
Oak Ridge National Laboratory
(865) 241-6602 office


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

* David Dillow wrote:

> On Fri, 2008-01-11 at 11:29 +0100, Ingo Molnar wrote:
> > (David, could you try the patch further below - does it fix bkltrace
> > timestamps too?)
>
> Jens already got back to you, but I can confirm it works here as well.

great, thanks for testing it. (i'm not sure Jens saw the same issues as
you, so your test was useful too.)

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/

On Fri, Jan 11 2008, Ingo Molnar wrote:
>
> * Jens Axboe wrote:
>
> > > > If blktrace worked in 2.6.23 and it doesn't in 2.6.24 because of
> > > > some option that isn't immediately apparent, then it's a
> > > > regression. Period.
> > >
> > > not completely correct. CONFIG_NO_HZ is a default-disabled option
> > > that became newly available on 64-bit x86. So if NO_HZ does not
> > > completely work on 64-bit, and if 32-bit works fine - which we dont
> > > know yet (my guess would be that it's similarly broken on the same
> > > box) then it's not a regression.
> >
> > Ingo, it doesn't matter if the option is disabled by default or not!
> > The fact is that functionality foo works in 2.6.23 and doesn't in
> > 2.6.24 because of something unrelated. And that IS a regression, no
> > matter what kind of word play you are doing here :-)
>
> argh, Jens. Really. I believe you stopped using your brain because
> CONFIG_BKL_IO_TRACE=y is affected by this bug and apparently you've got
> a weak spot for it

I don't think so.

> Think about it another way then, in the context of another, real kernel
> feature we introduced in v2.6.24, namely CONFIG_CPU_IDLE=y:
>
> - Fact: feature CONFIG_CPU_IDLE=y did not exist in 64-bit x86 in v2.6.23.
> It has known bugs but they are not flagged 'regressions' (because the
> feature did not exist before and the option is default-disabled) hence
> the feature can stay. Some fixes to it are too dangerous or too
> unproven and are delayed to v2.6.25.

>
> and now apply the same rule to CONFIG_NO_HZ:
>
> - Fact: feature CONFIG_NO_HZ=y did not exist in 64-bit x86 in v2.6.23.
> It has known bugs but they are not flagged 'regressions' (because the
> feature did not exist before and the option is default-disabled) hence
> the feature can stay. Some fixes to it are too dangerous or too
> unproven and are delayed to v2.6.25.

Forget about this unrelated feature, it has nothing to do with this
situation. What if some (eg) sata driver broke because someone enabled
CONFIG_NO_HZ. Would you say that's not a regression because it worked
before that option was there? That's crap, no user would buy that
argument. As far as the user is concerned, it IS a regression.

Since we obviously disagree on this, lets agree to not debate it any
further. It doesn't matter anyway, since the issue is resolved.

> > [...] What is the cost of ktime_get() compared to sched_clock()?
>
> compared to the costs of IO transfers it should be OK. It can be really
> fast but in the worst-case it can be as slow as 3-6 usecs (when we use
> the acpi_pm clocksource). If there's complaints then probably the
> networking folks will yell first

How does that compare to sched_clock()? 3-6 usec would render blktrace
unusable for certain types of testing, unfortunately. This isn't just
queried per IO, it's done for every action for that IO. So you'd
typically have up to eg 10 traces for a single IO piece, perhaps even
more.

For 2.6.24 the solution is fine, I'll revisit the impact when I can.

--
Jens Axboe

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

On Fri, Jan 11 2008, Ingo Molnar wrote:
>
> * David Dillow wrote:
>
> > On Fri, 2008-01-11 at 11:29 +0100, Ingo Molnar wrote:
> > > (David, could you try the patch further below - does it fix bkltrace
> > > timestamps too?)
> >
> > Jens already got back to you, but I can confirm it works here as well.
>
> great, thanks for testing it. (i'm not sure Jens saw the same issues as
> you, so your test was useful too.)

Right, I merely checked that ktime_get() worked for me.

--
Jens Axboe

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

Ingo Molnar wrote:

> ok. I have applied all but this one

Hmm, I couldn't find them in mingo/linux-2.6-sched-devel.git.

> i think it's much simpler to do what i have below. Could you try it on
> your box? Or if it is using ACPI idle - in that case the callbacks
> should already be there and there should be no need for further fixups.
>
> Subject: x86: idle wakeup event in the HLT loop

I use ACPI, so this patch has no effect.

FYI, I'm currently trying to track down where rq->clock started to
overflow with nohz=off, and it seems to be before 2.6.23, so my patches
are not at fault ;-) Or maybe I am dreaming and it was always
overflowing. Investigating ...

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

Guillaume Chazarain wrote:

> FYI, I'm currently trying to track down where rq->clock started to
> overflow with nohz=off, and it seems to be before 2.6.23, so my patches
> are not at fault ;-) Or maybe I am dreaming and it was always
> overflowing. Investigating ...

And the winner is:

commit 529c77261bccd9d37f110f58b0753d95beaa9fa2
Author: Ingo Molnar
Date: Fri Aug 10 23:05:11 2007 +0200

sched: improve rq-clock overflow logic

improve the rq-clock overflow logic: limit the absolute rq->clock
delta since the last scheduler tick, instead of limiting the delta
itself.

tested by Arjan van de Ven - whole laptop was misbehaving due to
an incorrectly calibrated cpu_khz confusing sched_clock().

Signed-off-by: Ingo Molnar
Signed-off-by: Arjan van de Ven

diff --git a/kernel/sched.c b/kernel/sched.c
index b0afd8d..6247e4a 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -263,6 +263,7 @@ struct rq {

unsigned int clock_warps, clock_overflows;
unsigned int clock_unstable_events;
+ u64 tick_timestamp;

atomic_t nr_iowait;

@@ -341,8 +342,11 @@ static void __update_rq_clock(struct rq *rq)
/*
* Catch too large forward jumps too:
*/
- if (unlikely(delta > 2*TICK_NSEC)) {
- clock++;
+ if (unlikely(clock + delta > rq->tick_timestamp + TICK_NSEC)) {
+ if (clock < rq->tick_timestamp + TICK_NSEC)
+ clock = rq->tick_timestamp + TICK_NSEC;
+ else
+ clock++;
rq->clock_overflows++;
} else {
if (unlikely(delta > rq->clock_max_delta))
@@ -3308,9 +3312,16 @@ void scheduler_tick(void)
int cpu = smp_processor_id();
struct rq *rq = cpu_rq(cpu);
struct task_struct *curr = rq->curr;
+ u64 next_tick = rq->tick_timestamp + TICK_NSEC;

spin_lock(&rq->lock);
__update_rq_clock(rq);
+ /*
+ * Let rq->clock advance by at least TICK_NSEC:
+ */
+ if (unlikely(rq->clock < next_tick))
+ rq->clock = next_tick;
+ rq->tick_timestamp = rq->clock;
update_cpu_load(rq);
if (curr != rq->idle) /* FIXME: needed? */
curr->sched_class->task_tick(rq, curr);


Seems like I originally was not the only one seeing 2 jiffies jumps ;-)
I'll adapt my patches.

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

Hi.

Ingo Molnar wrote:
> * nigel@suspend2.net wrote:
>
>>>>> Just out of curiosity, could you try the appended cumulative patch
>>>>> and report .clock_warps, .clock_overflows and .clock_underflows as
>>>>> you did.
>>>> With those patches, CONFIG_NO_HZ works just fine.
>> Could these patches also help with hibernation issues? I'm trying
>> x86_64+NO_HZ, and seeing activity delayed during the atomic copy and
>> afterwards until I manually generate interrupts (by pressing keys).
>
> i dont think that should be related to cpu_clock() use. Does the patch
> below make any difference? (or could you try x86.git to get the whole
> stack of x86 changes that we have at the moment.) Here's the coordinates
> for x86.git:

Sorry for the delay in replying. Something seems to help, but I haven't
managed to identify what yet. I don't think it was the patch appended
because I'm on UP. If you care, I'll see if I can find the time to look
more carefully.

Nigel

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

On Mon, Jan 14 2008, Ingo Molnar wrote:
>
> * Ingo Molnar wrote:
>
> > because a perfectly working system is:
> >
> > "a user's .config that worked before should work with the new kernel
> > too"
> >
> > not:
> >
> > "a user's .config that worked before should work now too, with random
> > new kernel features enabled as well."
> >
> > the latter appears to be the rule you are applying, but it's not the
> > regression rule we are using.
>
> Jens, just to bring your definition of regressions to its logical
> conclusion: does this mean that if there is any longstanding bug in the
> block layer that you know about, but i didnt ever utilize that bit of
> the block layer it in my .config, and if i enable it now in the .config
> and i experience that bug, does it suddenly count as a regression? Do
> you realize that your definition for "regressions" turns _almost every_
> current bug in the kernel into a regression?

Ingo, why do you keep harping this issue? I thought I suggested we agree
to disagree on this and let it rest.

And I would say that, yes, that is a regression, if that config option
is a core option that people are likely to enable. The CONFIG_NO_HZ is a
new option, people will select it. Your example pertain more to the 'use
mmio for IO operations' type options for drivers. If you enable that and
your driver suddenly stops working, you have a clear idea of WHY it
stops working and how to fix it. Not so with this blktrace scenario, I
bet that would take people quite a while to figure out how it broke.

Can we drop this subject now, please? The issue is resolved (and
merged), debating definitions of regressions is not very productive :-)

--
Jens Axboe

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

On Mon, Jan 14 2008, Christoph Hellwig wrote:
>
> Folks, this is getting a little silly.

It is

> Even if CONFIG_NO_HZ is new this is a an important regression, and
> yes we should avoid regressions wherever we can, and for such a quite
> important feature we should fix it. On the other hand blktrace is using
> the wrong interface, and it has been told multiple times on lkml that
> sched_clock() should never ever be used outside the scheduler. So the
> burden to fix this regression lies on the shoulders of the blktrace
> maintainer.

That is true. So far alternatives have all been slower though, so not
very tempting to transition to.

> No need for silly name calling here.

I don't know what thread you are reading, but neiter Ingo nor I have
resorted to silly name calling. The thread has long since diverted from
the original topic, though.

--
Jens Axboe

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

* Jens Axboe wrote:

> > - Fact: feature CONFIG_CPU_IDLE=y did not exist in 64-bit x86 in v2.6.23.
> > It has known bugs but they are not flagged 'regressions' (because the
> > feature did not exist before and the option is default-disabled) hence
> > the feature can stay. Some fixes to it are too dangerous or too
> > unproven and are delayed to v2.6.25.
>
> >
> > and now apply the same rule to CONFIG_NO_HZ:
> >
> > - Fact: feature CONFIG_NO_HZ=y did not exist in 64-bit x86 in v2.6.23.
> > It has known bugs but they are not flagged 'regressions' (because the
> > feature did not exist before and the option is default-disabled) hence
> > the feature can stay. Some fixes to it are too dangerous or too
> > unproven and are delayed to v2.6.25.
>
> Forget about this unrelated feature, it has nothing to do with this
> situation. [...]

why "forget" about a perfectly valid example right from v2.6.24 that
demonstrates our definition for regressions and demonstrates the rules
we are applying? Because it does not fit your argument?

> [...] What if some (eg) sata driver broke because someone enabled
> CONFIG_NO_HZ. Would you say that's not a regression because it worked
> before that option was there? [...]

as long as it's the same situation as have here (i.e. that NO_HZ
appeared on an architecture where it wasnt offered before): yes,
exactly, it's _NOT_ a regression in that sata driver [the sata driver
might not even have been touched], and it's _NOT_ a regression in
CONFIG_NO_HZ either because a regression can only be something that
"something breaks that worked before". In that case it's a _bug_ in the
newly introduced NO_HZ code.

> [...] That's crap, no user would buy that argument. As far as the user
> is concerned, it IS a regression.

the user enabled a newly introduced kernel feature and that new feature
might be not perfect. It might break a whole lot of unrelated code. We
consider them bugs (of course), and work on fixing them, but still we
dont consider such bugs a regression and dont revert a new feature if
it's not fixed. It's a special-case. We apply that special case to all
sorts of new kernel features, such as the new iwl3945 driver in 2.6.24,
which has a number of unfixed bugs. We applied that to new kernel
features as well, such as containers/control-groups in 2.6.23, etc. This
is nothing new, we did this numerous times ever since the new-style
development model was introduced around 2.6.12. _You_ might not know
about it but that does not change the situation.

because a perfectly working system is:

"a user's .config that worked before should work with the new kernel
too"

not:

"a user's .config that worked before should work now too, with random
new kernel features enabled as well."

the latter appears to be the rule you are applying, but it's not the
regression rule we are using.

> Since we obviously disagree on this, lets agree to not debate it any
> further. It doesn't matter anyway, since the issue is resolved.

this is not a matter of opinion or "disagreement", this is a matter of
fact. You were the one telling me: "go fix that regression", so this
issue might come up anytime in the future. Better clear it once and for
all, we all have got better things to do than to rehash the same issue
again and again.

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/

* Ingo Molnar wrote:

> because a perfectly working system is:
>
> "a user's .config that worked before should work with the new kernel
> too"
>
> not:
>
> "a user's .config that worked before should work now too, with random
> new kernel features enabled as well."
>
> the latter appears to be the rule you are applying, but it's not the
> regression rule we are using.

Jens, just to bring your definition of regressions to its logical
conclusion: does this mean that if there is any longstanding bug in the
block layer that you know about, but i didnt ever utilize that bit of
the block layer it in my .config, and if i enable it now in the .config
and i experience that bug, does it suddenly count as a regression? Do
you realize that your definition for "regressions" turns _almost every_
current bug in the kernel into a regression?

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/

Folks, this is getting a little silly.

Even if CONFIG_NO_HZ is new this is a an important regression, and
yes we should avoid regressions wherever we can, and for such a quite
important feature we should fix it. On the other hand blktrace is using
the wrong interface, and it has been told multiple times on lkml that
sched_clock() should never ever be used outside the scheduler. So the
burden to fix this regression lies on the shoulders of the blktrace
maintainer.

No need for silly name calling here.

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