* [PATCH] x86: fix hpet timer reinit for x86_64 (v2) @ 2009-02-06 8:50 Pavel Emelyanov 2009-02-06 14:09 ` [PATCH] x86: clean up hpet timer reinit Ingo Molnar 0 siblings, 1 reply; 5+ messages in thread From: Pavel Emelyanov @ 2009-02-06 8:50 UTC (permalink / raw) To: Ingo Molnar; +Cc: Linux Kernel Mailing List, Kirill Korotaev Sorry for late response - took some time to re-check this... There's a small problem with hpet_rtc_reinit function - it checks for the hpet_readl(HPET_COUNTER) - hpet_t1_cmp > 0 to continue increasing both the HPET_T1_CMP (register) and the hpet_t1_cmp (variable). But since the HPET_COUNTER is always 32-bit, if the hpet_t1_cmp is 64-bit this condition will always be FALSE once the latter hits the 32-bit boundary, and we can have a situation, when we don't increase the HPET_T1_CMP register high enough. The result - timer stops ticking, since HPET_T1_CMP becomes less, than the COUNTER and never increased again. Symptoms were observed in user space. An application doing read() on /dev/rtc blocked sometimes for a long periods of time (as turned out - until the counter gets wrap around). The fix is to properly check for one counter being ahead of another the way it's done for jiffies. Plush, change the hpet_t1_cmp to u32 to save 4 bytes from .bss ;) Reported-by: Kirill Korotaev <dev@openvz.org> Signed-off-by: Pavel Emelyanov <xemul@openvz.org> --- diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c index 64d5ad0..388254f 100644 --- a/arch/x86/kernel/hpet.c +++ b/arch/x86/kernel/hpet.c @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags; static int hpet_prev_update_sec; static struct rtc_time hpet_alarm_time; static unsigned long hpet_pie_count; -static unsigned long hpet_t1_cmp; +static u32 hpet_t1_cmp; static unsigned long hpet_default_delta; static unsigned long hpet_pie_delta; static unsigned long hpet_pie_limit; @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit; static rtc_irq_handler irq_handler; /* + * Check that the hpet counter c1 is ahead of the c2 + */ +static inline int hpet_cnt_ahead(u32 c1, u32 c2) +{ + return (s32)(c2 - c1) < 0; +} + +/* * Registers a IRQ handler. */ int hpet_register_irq_handler(rtc_irq_handler handler) @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void) hpet_t1_cmp += delta; hpet_writel(hpet_t1_cmp, HPET_T1_CMP); lost_ints++; - } while ((long)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0); + } while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER))); if (lost_ints) { if (hpet_rtc_flags & RTC_PIE) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH] x86: clean up hpet timer reinit 2009-02-06 8:50 [PATCH] x86: fix hpet timer reinit for x86_64 (v2) Pavel Emelyanov @ 2009-02-06 14:09 ` Ingo Molnar 2009-02-06 15:42 ` Daniel Forrest 0 siblings, 1 reply; 5+ messages in thread From: Ingo Molnar @ 2009-02-06 14:09 UTC (permalink / raw) To: Pavel Emelyanov Cc: Linux Kernel Mailing List, Kirill Korotaev, Linus Torvalds, Thomas Gleixner, H. Peter Anvin * Pavel Emelyanov <xemul@openvz.org> wrote: > Sorry for late response - took some time to re-check this... Applied to tip:timers/urgent, thanks Pavel! Note, since i've already queued up the minimal fix i've created a delta cleanup patch from your v2 patch - see it below. (It is the exact same end result in terms of code, just a nicer splitup.) Ingo -------------------> >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001 From: Pavel Emelyanov <xemul@openvz.org> Date: Wed, 4 Feb 2009 13:40:31 +0300 Subject: [PATCH] x86: clean up hpet timer reinit Implement Linus's suggestion: introduce the hpet_cnt_ahead() helper function to compare hpet time values - like other wrapping counter comparisons are abstracted away elsewhere. (jiffies, ktime_t, etc.) Reported-by: Kirill Korotaev <dev@openvz.org> Signed-off-by: Pavel Emelyanov <xemul@openvz.org> Signed-off-by: Ingo Molnar <mingo@elte.hu> --- arch/x86/kernel/hpet.c | 12 ++++++++++-- 1 files changed, 10 insertions(+), 2 deletions(-) diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c index c761f91..388254f 100644 --- a/arch/x86/kernel/hpet.c +++ b/arch/x86/kernel/hpet.c @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags; static int hpet_prev_update_sec; static struct rtc_time hpet_alarm_time; static unsigned long hpet_pie_count; -static unsigned long hpet_t1_cmp; +static u32 hpet_t1_cmp; static unsigned long hpet_default_delta; static unsigned long hpet_pie_delta; static unsigned long hpet_pie_limit; @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit; static rtc_irq_handler irq_handler; /* + * Check that the hpet counter c1 is ahead of the c2 + */ +static inline int hpet_cnt_ahead(u32 c1, u32 c2) +{ + return (s32)(c2 - c1) < 0; +} + +/* * Registers a IRQ handler. */ int hpet_register_irq_handler(rtc_irq_handler handler) @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void) hpet_t1_cmp += delta; hpet_writel(hpet_t1_cmp, HPET_T1_CMP); lost_ints++; - } while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0); + } while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER))); if (lost_ints) { if (hpet_rtc_flags & RTC_PIE) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: clean up hpet timer reinit 2009-02-06 14:09 ` [PATCH] x86: clean up hpet timer reinit Ingo Molnar @ 2009-02-06 15:42 ` Daniel Forrest 2009-02-06 17:00 ` Pavel Emelyanov 0 siblings, 1 reply; 5+ messages in thread From: Daniel Forrest @ 2009-02-06 15:42 UTC (permalink / raw) To: Ingo Molnar Cc: Pavel Emelyanov, Linux Kernel Mailing List, Kirill Korotaev, Linus Torvalds, Thomas Gleixner, H. Peter Anvin On Fri, Feb 06, 2009 at 03:09:43PM +0100, Ingo Molnar wrote: > > * Pavel Emelyanov <xemul@openvz.org> wrote: > > > Sorry for late response - took some time to re-check this... > > Applied to tip:timers/urgent, thanks Pavel! > > Note, since i've already queued up the minimal fix i've created a delta > cleanup patch from your v2 patch - see it below. (It is the exact same end > result in terms of code, just a nicer splitup.) > > Ingo > > -------------------> > >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001 > From: Pavel Emelyanov <xemul@openvz.org> > Date: Wed, 4 Feb 2009 13:40:31 +0300 > Subject: [PATCH] x86: clean up hpet timer reinit > > Implement Linus's suggestion: introduce the hpet_cnt_ahead() > helper function to compare hpet time values - like other > wrapping counter comparisons are abstracted away elsewhere. > (jiffies, ktime_t, etc.) > > Reported-by: Kirill Korotaev <dev@openvz.org> > Signed-off-by: Pavel Emelyanov <xemul@openvz.org> > Signed-off-by: Ingo Molnar <mingo@elte.hu> > --- > arch/x86/kernel/hpet.c | 12 ++++++++++-- > 1 files changed, 10 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c > index c761f91..388254f 100644 > --- a/arch/x86/kernel/hpet.c > +++ b/arch/x86/kernel/hpet.c > @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags; > static int hpet_prev_update_sec; > static struct rtc_time hpet_alarm_time; > static unsigned long hpet_pie_count; > -static unsigned long hpet_t1_cmp; > +static u32 hpet_t1_cmp; > static unsigned long hpet_default_delta; > static unsigned long hpet_pie_delta; > static unsigned long hpet_pie_limit; > @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit; > static rtc_irq_handler irq_handler; > > /* > + * Check that the hpet counter c1 is ahead of the c2 > + */ > +static inline int hpet_cnt_ahead(u32 c1, u32 c2) > +{ > + return (s32)(c2 - c1) < 0; > +} > + > +/* > * Registers a IRQ handler. > */ > int hpet_register_irq_handler(rtc_irq_handler handler) > @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void) > hpet_t1_cmp += delta; > hpet_writel(hpet_t1_cmp, HPET_T1_CMP); > lost_ints++; > - } while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0); > + } while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER))); These are not equivalent for the case where the values are equal. Let "A = hpet_t1_cmp" and "B = hpet_readl(HPET_COUNTER)" Then "!(A > B)" means "(B - A) >= 0" not "(B - A) > 0" Shouldn't it be: + } while (hpet_cnt_ahead(hpet_readl(HPET_COUNTER), hpet_t1_cmp)); Or am I missing something? > > if (lost_ints) { > if (hpet_rtc_flags & RTC_PIE) > -- -- Daniel K. Forrest Space Science and dan.forrest@ssec.wisc.edu Engineering Center (608) 890 - 0558 University of Wisconsin, Madison ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: clean up hpet timer reinit 2009-02-06 15:42 ` Daniel Forrest @ 2009-02-06 17:00 ` Pavel Emelyanov 2009-02-06 17:11 ` Daniel Forrest 0 siblings, 1 reply; 5+ messages in thread From: Pavel Emelyanov @ 2009-02-06 17:00 UTC (permalink / raw) To: Daniel Forrest Cc: Ingo Molnar, Linux Kernel Mailing List, Kirill Korotaev, Linus Torvalds, Thomas Gleixner, H. Peter Anvin Daniel Forrest wrote: > On Fri, Feb 06, 2009 at 03:09:43PM +0100, Ingo Molnar wrote: >> * Pavel Emelyanov <xemul@openvz.org> wrote: >> >>> Sorry for late response - took some time to re-check this... >> Applied to tip:timers/urgent, thanks Pavel! >> >> Note, since i've already queued up the minimal fix i've created a delta >> cleanup patch from your v2 patch - see it below. (It is the exact same end >> result in terms of code, just a nicer splitup.) >> >> Ingo >> >> -------------------> >> >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001 >> From: Pavel Emelyanov <xemul@openvz.org> >> Date: Wed, 4 Feb 2009 13:40:31 +0300 >> Subject: [PATCH] x86: clean up hpet timer reinit >> >> Implement Linus's suggestion: introduce the hpet_cnt_ahead() >> helper function to compare hpet time values - like other >> wrapping counter comparisons are abstracted away elsewhere. >> (jiffies, ktime_t, etc.) >> >> Reported-by: Kirill Korotaev <dev@openvz.org> >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org> >> Signed-off-by: Ingo Molnar <mingo@elte.hu> >> --- >> arch/x86/kernel/hpet.c | 12 ++++++++++-- >> 1 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c >> index c761f91..388254f 100644 >> --- a/arch/x86/kernel/hpet.c >> +++ b/arch/x86/kernel/hpet.c >> @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags; >> static int hpet_prev_update_sec; >> static struct rtc_time hpet_alarm_time; >> static unsigned long hpet_pie_count; >> -static unsigned long hpet_t1_cmp; >> +static u32 hpet_t1_cmp; >> static unsigned long hpet_default_delta; >> static unsigned long hpet_pie_delta; >> static unsigned long hpet_pie_limit; >> @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit; >> static rtc_irq_handler irq_handler; >> >> /* >> + * Check that the hpet counter c1 is ahead of the c2 >> + */ >> +static inline int hpet_cnt_ahead(u32 c1, u32 c2) >> +{ >> + return (s32)(c2 - c1) < 0; >> +} >> + >> +/* >> * Registers a IRQ handler. >> */ >> int hpet_register_irq_handler(rtc_irq_handler handler) >> @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void) >> hpet_t1_cmp += delta; >> hpet_writel(hpet_t1_cmp, HPET_T1_CMP); >> lost_ints++; >> - } while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0); >> + } while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER))); > > These are not equivalent for the case where the values are equal. > > Let "A = hpet_t1_cmp" and "B = hpet_readl(HPET_COUNTER)" > > Then "!(A > B)" means "(B - A) >= 0" not "(B - A) > 0" > > Shouldn't it be: > > + } while (hpet_cnt_ahead(hpet_readl(HPET_COUNTER), hpet_t1_cmp)); > > Or am I missing something? When comparator it equal to counter (the corner case we're talking about) the hpet_cnt_ahead will return false and the loop will go on shifting the cmp, which is what we need here. >> >> if (lost_ints) { >> if (hpet_rtc_flags & RTC_PIE) >> -- > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] x86: clean up hpet timer reinit 2009-02-06 17:00 ` Pavel Emelyanov @ 2009-02-06 17:11 ` Daniel Forrest 0 siblings, 0 replies; 5+ messages in thread From: Daniel Forrest @ 2009-02-06 17:11 UTC (permalink / raw) To: Pavel Emelyanov Cc: Ingo Molnar, Linux Kernel Mailing List, Kirill Korotaev, Linus Torvalds, Thomas Gleixner, H. Peter Anvin On Fri, Feb 06, 2009 at 08:00:00PM +0300, Pavel Emelyanov wrote: > Daniel Forrest wrote: > > On Fri, Feb 06, 2009 at 03:09:43PM +0100, Ingo Molnar wrote: > >> * Pavel Emelyanov <xemul@openvz.org> wrote: > >> > >>> Sorry for late response - took some time to re-check this... > >> Applied to tip:timers/urgent, thanks Pavel! > >> > >> Note, since i've already queued up the minimal fix i've created a delta > >> cleanup patch from your v2 patch - see it below. (It is the exact same end > >> result in terms of code, just a nicer splitup.) > >> > >> Ingo > >> > >> -------------------> > >> >From ff08f76d738d0ec0f334b187f61e160caa321d54 Mon Sep 17 00:00:00 2001 > >> From: Pavel Emelyanov <xemul@openvz.org> > >> Date: Wed, 4 Feb 2009 13:40:31 +0300 > >> Subject: [PATCH] x86: clean up hpet timer reinit > >> > >> Implement Linus's suggestion: introduce the hpet_cnt_ahead() > >> helper function to compare hpet time values - like other > >> wrapping counter comparisons are abstracted away elsewhere. > >> (jiffies, ktime_t, etc.) > >> > >> Reported-by: Kirill Korotaev <dev@openvz.org> > >> Signed-off-by: Pavel Emelyanov <xemul@openvz.org> > >> Signed-off-by: Ingo Molnar <mingo@elte.hu> > >> --- > >> arch/x86/kernel/hpet.c | 12 ++++++++++-- > >> 1 files changed, 10 insertions(+), 2 deletions(-) > >> > >> diff --git a/arch/x86/kernel/hpet.c b/arch/x86/kernel/hpet.c > >> index c761f91..388254f 100644 > >> --- a/arch/x86/kernel/hpet.c > >> +++ b/arch/x86/kernel/hpet.c > >> @@ -897,7 +897,7 @@ static unsigned long hpet_rtc_flags; > >> static int hpet_prev_update_sec; > >> static struct rtc_time hpet_alarm_time; > >> static unsigned long hpet_pie_count; > >> -static unsigned long hpet_t1_cmp; > >> +static u32 hpet_t1_cmp; > >> static unsigned long hpet_default_delta; > >> static unsigned long hpet_pie_delta; > >> static unsigned long hpet_pie_limit; > >> @@ -905,6 +905,14 @@ static unsigned long hpet_pie_limit; > >> static rtc_irq_handler irq_handler; > >> > >> /* > >> + * Check that the hpet counter c1 is ahead of the c2 > >> + */ > >> +static inline int hpet_cnt_ahead(u32 c1, u32 c2) > >> +{ > >> + return (s32)(c2 - c1) < 0; > >> +} > >> + > >> +/* > >> * Registers a IRQ handler. > >> */ > >> int hpet_register_irq_handler(rtc_irq_handler handler) > >> @@ -1075,7 +1083,7 @@ static void hpet_rtc_timer_reinit(void) > >> hpet_t1_cmp += delta; > >> hpet_writel(hpet_t1_cmp, HPET_T1_CMP); > >> lost_ints++; > >> - } while ((s32)(hpet_readl(HPET_COUNTER) - hpet_t1_cmp) > 0); > >> + } while (!hpet_cnt_ahead(hpet_t1_cmp, hpet_readl(HPET_COUNTER))); > > > > These are not equivalent for the case where the values are equal. > > > > Let "A = hpet_t1_cmp" and "B = hpet_readl(HPET_COUNTER)" > > > > Then "!(A > B)" means "(B - A) >= 0" not "(B - A) > 0" > > > > Shouldn't it be: > > > > + } while (hpet_cnt_ahead(hpet_readl(HPET_COUNTER), hpet_t1_cmp)); > > > > Or am I missing something? > > When comparator it equal to counter (the corner case we're talking about) the > hpet_cnt_ahead will return false and the loop will go on shifting the cmp, > which is what we need here. > Okay, as long as that is what is intended. My point was just that the code being replaced did not loop when the counts were equal so it looked like an unintended change. > >> > >> if (lost_ints) { > >> if (hpet_rtc_flags & RTC_PIE) > >> -- -- Daniel K. Forrest Space Science and dan.forrest@ssec.wisc.edu Engineering Center (608) 890 - 0558 University of Wisconsin, Madison ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2009-02-06 17:11 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2009-02-06 8:50 [PATCH] x86: fix hpet timer reinit for x86_64 (v2) Pavel Emelyanov 2009-02-06 14:09 ` [PATCH] x86: clean up hpet timer reinit Ingo Molnar 2009-02-06 15:42 ` Daniel Forrest 2009-02-06 17:00 ` Pavel Emelyanov 2009-02-06 17:11 ` Daniel Forrest
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).