Prev: Protect prefetch macro arguments.
Next: dmar: section mismatch cleanup
From: Andrew Morton on 26 Mar 2010 17:30
On Tue, 23 Mar 2010 23:36:11 -0400
Yury Polyanskiy <ypolyans(a)princeton.edu> wrote:

> The drivers/char/hangcheck-timer.c is doubly broken. First, the
> following line overflows unsigned long:
> # define TIMER_FREQ (HZ*loops_per_jiffy)
>
> Second, and more importantly, loops_per_jiffy has little to do with the conversion from the
> the time scale of get_cycles() (aka rdtsc) to the time scale of jiffies.

It's a bit odd to have a driver be this broken on x86_32 for five years
without anyone noticing. What are the user-visible effects of these
shortcomings?

Also, please do send us a Signed-off-by: for this patch, as explained
in Documentation/SubmittingPatches, thanks.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Joel Becker on 26 Mar 2010 17:50
On Tue, Mar 23, 2010 at 11:36:11PM -0400, Yury Polyanskiy wrote:
> Second, and more importantly, loops_per_jiffy has little to do with the conversion from the
> the time scale of get_cycles() (aka rdtsc) to the time scale of jiffies.

It used to! Fundamentally, of course, we didn't have a
monotonic clock everywhere that satisfied hangcheck-timer's needs. So
we had to use different approaches on different architectures.

> @@ -130,7 +129,9 @@ extern unsigned long long monotonic_clock(void);
> #else
> static inline unsigned long long monotonic_clock(void)
> {
> - return get_cycles();
> + struct timespec ts;
> + getrawmonotonic(&ts);
> + return timespec_to_ns(&ts);
> }
> #endif /* HAVE_MONOTONIC */

I have two questions:

1) Does getrawmonotonic() satisfy hangcheck-timer? What I mean is, will
it always return the wallclock nanoseconds even in the face of CPU speed
changes, suspend, udelay, or any other suspension of kernel operation?
Yes, I know this is a tougher standard than rdtsc(), but this is what
hangcheck-timer wants. rdtsc() at least satisfied udelay and PCI hangs.

2) If it does satisfy, why not use it for all hangcheck usage instead of
any ifdefs?

Joel

--

"The cynics are right nine times out of ten."
- H. L. Mencken

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker(a)oracle.com
Phone: (650) 506-8127
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Yury Polyanskiy on 26 Mar 2010 18:00
On Fri, 26 Mar 2010 14:24:23 -0700
Andrew Morton <akpm(a)linux-foundation.org> wrote:

> On Tue, 23 Mar 2010 23:36:11 -0400
> Yury Polyanskiy <ypolyans(a)princeton.edu> wrote:
>
> > The drivers/char/hangcheck-timer.c is doubly broken. First, the
> > following line overflows unsigned long:
> > # define TIMER_FREQ (HZ*loops_per_jiffy)
> >
> > Second, and more importantly, loops_per_jiffy has little to do with the conversion from the
> > the time scale of get_cycles() (aka rdtsc) to the time scale of jiffies.
>
> It's a bit odd to have a driver be this broken on x86_32 for five years
> without anyone noticing. What are the user-visible effects of these
> shortcomings?

When the overflown value of TIMER_FREQ is abnormally low, it spams the
syslog with KERN_CRIT messages "Hangcheck: hangcheck value past margin!"
But whether it happens or not depends on HZ and lpj in a complex way.
People have hit it occasionally as far as google search can tell.

>
> Also, please do send us a Signed-off-by: for this patch, as explained
> in Documentation/SubmittingPatches, thanks.
>

Sorry.

Signed-off-by: Yury Polyanskiy <polyanskiy(a)gmail.com>

Thank you Andrew!

Yury
From: Yury Polyanskiy on 26 Mar 2010 18:10
On Fri, 26 Mar 2010 14:46:49 -0700
Joel Becker <Joel.Becker(a)oracle.com> wrote:

> On Tue, Mar 23, 2010 at 11:36:11PM -0400, Yury Polyanskiy wrote:

> 1) Does getrawmonotonic() satisfy hangcheck-timer? What I mean is, will
> it always return the wallclock nanoseconds even in the face of CPU speed
> changes, suspend, udelay, or any other suspension of kernel operation?
> Yes, I know this is a tougher standard than rdtsc(), but this is what
> hangcheck-timer wants. rdtsc() at least satisfied udelay and PCI hangs.

Yes, as far as I can tell. Note that rdtsc is hosed on suspend-resume.

>
> 2) If it does satisfy, why not use it for all hangcheck usage instead of
> any ifdefs?

On my part, I didn't want to touch the S390 code since I can't test it.

Yury
From: Joel Becker on 26 Mar 2010 21:00
On Fri, Mar 26, 2010 at 06:00:25PM -0400, Yury Polyanskiy wrote:
> On Fri, 26 Mar 2010 14:46:49 -0700
> Joel Becker <Joel.Becker(a)oracle.com> wrote:
>
> > On Tue, Mar 23, 2010 at 11:36:11PM -0400, Yury Polyanskiy wrote:
>
> > 1) Does getrawmonotonic() satisfy hangcheck-timer? What I mean is, will
> > it always return the wallclock nanoseconds even in the face of CPU speed
> > changes, suspend, udelay, or any other suspension of kernel operation?
> > Yes, I know this is a tougher standard than rdtsc(), but this is what
> > hangcheck-timer wants. rdtsc() at least satisfied udelay and PCI hangs.
>
> Yes, as far as I can tell. Note that rdtsc is hosed on suspend-resume.

Yeah, I know. rdtsc hangcheck-timer really required no suspend
or cpufreq. Since it is only really used by servers, this wasn't a
terrible restriction. Then virtualization came along...

> > 2) If it does satisfy, why not use it for all hangcheck usage instead of
> > any ifdefs?
>
> On my part, I didn't want to touch the S390 code since I can't test it.

Makes sense. Andrew, would you mind pushing this through your
tree?

Acked-by: Joel Becker <joel.becker(a)oracle.com>

Joel

--

Dort wo man B�cher verbrennt, verbrennt man am Ende auch Mensch.
(Wherever they burn books, they will also end up burning people.)
- Heinrich Heine

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker(a)oracle.com
Phone: (650) 506-8127
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
 |  Next  |  Last
Pages: 1 2 3 4
Prev: Protect prefetch macro arguments.
Next: dmar: section mismatch cleanup