[PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[Petr Vandrovec]

Hello Linus,
  can you apply patch below?

Since beginning of July my Opteron box was randomly crashing and being rebooted
by hardware watchdog.  Today it finally did it in front of me, and this patch
will hopefully fix it.

Problem is that at the end of June (28th, commit 
47f176fdaf8924bc83fddcf9658f2fd3ef60d573, [PATCH] Using msleep() instead of HZ) 
rtc_get_rtc_time was converted to use msleep() instead of busy waiting.  But
rtc_get_rtc_time is used by hpet_rtc_interrupt, and scheduling is not allowed
during interrupt.  So I'm reverting this part of original change, replacing
msleep() back with busy loop.

Original old code was busy waiting for 20ms, while on my hardware in the worst
case update-in-progress bit was asserted for at most 363 passes through loop
(on 2GHz dual Opteron), much less than even one jiffie, not even talking
about 20ms.  So I changed code to just wait only as long as necessary.  Otherwise
when RTC was set to generate 8192Hz timer, it stopped doing anything for
20ms (160 pulses were skipped!) from time to time, and this is rather suboptimal
as far as I can tell.

						Thanks,
							Petr Vandrovec


Signed-off-by: Petr Vandrovec <vandrove@vc.cvut.cz>

diff -urdN linux/drivers/char/rtc.c linux/drivers/char/rtc.c
--- linux/drivers/char/rtc.c	2005-08-05 12:43:54.000000000 +0000
+++ linux/drivers/char/rtc.c	2005-08-05 13:26:48.000000000 +0000
@@ -1209,6 +1209,7 @@
 
 void rtc_get_rtc_time(struct rtc_time *rtc_tm)
 {
+	unsigned long uip_watchdog = jiffies;
 	unsigned char ctrl;
 #ifdef CONFIG_MACH_DECSTATION
 	unsigned int real_year;
@@ -1224,8 +1225,10 @@
 	 * Once the read clears, read the RTC time (again via ioctl). Easy.
 	 */
 
-	if (rtc_is_updating() != 0)
-		msleep(20);
+	while (rtc_is_updating() != 0 && jiffies - uip_watchdog < 2*HZ/100) {
+		barrier();
+		cpu_relax();
+	}
 
 	/*
 	 * Only the values that we read from the RTC are set. We leave

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[linux-os (Dick Johnson)]

On Fri, 5 Aug 2005, Petr Vandrovec wrote:

> Hello Linus,
>  can you apply patch below?
>
> Since beginning of July my Opteron box was randomly crashing and being rebooted
> by hardware watchdog.  Today it finally did it in front of me, and this patch
> will hopefully fix it.
>
> Problem is that at the end of June (28th, commit
> 47f176fdaf8924bc83fddcf9658f2fd3ef60d573, [PATCH] Using msleep() instead of HZ)
> rtc_get_rtc_time was converted to use msleep() instead of busy waiting.  But
> rtc_get_rtc_time is used by hpet_rtc_interrupt, and scheduling is not allowed
> during interrupt.  So I'm reverting this part of original change, replacing
> msleep() back with busy loop.
>
> Original old code was busy waiting for 20ms, while on my hardware in the worst
> case update-in-progress bit was asserted for at most 363 passes through loop
> (on 2GHz dual Opteron), much less than even one jiffie, not even talking
> about 20ms.  So I changed code to just wait only as long as necessary.  Otherwise
> when RTC was set to generate 8192Hz timer, it stopped doing anything for
> 20ms (160 pulses were skipped!) from time to time, and this is rather suboptimal
> as far as I can tell.
>
> 						Thanks,
> 							Petr Vandrovec
>
>

It this is called from an interrupt (I hope not), jiffies will
not change unless you have two or more CPUs. Please don't do this.
Just spin while rtc is updating some maximum number of loops.


> Signed-off-by: Petr Vandrovec <vandrove@vc.cvut.cz>
>
> diff -urdN linux/drivers/char/rtc.c linux/drivers/char/rtc.c
> --- linux/drivers/char/rtc.c	2005-08-05 12:43:54.000000000 +0000
> +++ linux/drivers/char/rtc.c	2005-08-05 13:26:48.000000000 +0000
> @@ -1209,6 +1209,7 @@
>
> void rtc_get_rtc_time(struct rtc_time *rtc_tm)
> {
> +	unsigned long uip_watchdog = jiffies;
> 	unsigned char ctrl;
> #ifdef CONFIG_MACH_DECSTATION
> 	unsigned int real_year;
> @@ -1224,8 +1225,10 @@
> 	 * Once the read clears, read the RTC time (again via ioctl). Easy.
> 	 */
>
> -	if (rtc_is_updating() != 0)
> -		msleep(20);
> +	while (rtc_is_updating() != 0 && jiffies - uip_watchdog < 2*HZ/100) {
> +		barrier();
> +		cpu_relax();
> +	}
>
> 	/*
> 	 * Only the values that we read from the RTC are set. We leave
> -
> 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/
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.12 on an i686 machine (5537.79 BogoMips).
Warning : 98.36% of all statistics are fiction.
.
I apologize for the following. I tried to kill it with the above dot :

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[Petr Vandrovec]

linux-os (Dick Johnson) wrote:
> On Fri, 5 Aug 2005, Petr Vandrovec wrote:
> 
> 
>>Hello Linus,
>> can you apply patch below?
>>
>>Since beginning of July my Opteron box was randomly crashing and being rebooted
>>by hardware watchdog.  Today it finally did it in front of me, and this patch
>>will hopefully fix it.
>>
>>Problem is that at the end of June (28th, commit
>>47f176fdaf8924bc83fddcf9658f2fd3ef60d573, [PATCH] Using msleep() instead of HZ)
>>rtc_get_rtc_time was converted to use msleep() instead of busy waiting.  But
>>rtc_get_rtc_time is used by hpet_rtc_interrupt, and scheduling is not allowed
>>during interrupt.  So I'm reverting this part of original change, replacing
>>msleep() back with busy loop.
>>
>>Original old code was busy waiting for 20ms, while on my hardware in the worst
>>case update-in-progress bit was asserted for at most 363 passes through loop
>>(on 2GHz dual Opteron), much less than even one jiffie, not even talking
>>about 20ms.  So I changed code to just wait only as long as necessary.  Otherwise
>>when RTC was set to generate 8192Hz timer, it stopped doing anything for
>>20ms (160 pulses were skipped!) from time to time, and this is rather suboptimal
>>as far as I can tell.
>>
>>						Thanks,
>>							Petr Vandrovec
>>
>>
> 
> 
> It this is called from an interrupt (I hope not), jiffies will
> not change unless you have two or more CPUs. Please don't do this.
> Just spin while rtc is updating some maximum number of loops.

It is called from rtc interrupt (hpet_rtc_interrupt calls it, if
you really want I'll reproduce oopses about scheduling in the
interrupt again and post it here for you convience).  But
being inside rtc interrupt should not prevent jiffies from being
incremented as system timer has higher priority than rtc.  We had
code which looped around until jiffies expired for years and as far
as I know nobody complained that boxes deadlock from time to time.
I just added test for 'rtc_is_updating()' to the test.
							Petr

> 
> 
> 
>>Signed-off-by: Petr Vandrovec <vandrove@vc.cvut.cz>
>>
>>diff -urdN linux/drivers/char/rtc.c linux/drivers/char/rtc.c
>>--- linux/drivers/char/rtc.c	2005-08-05 12:43:54.000000000 +0000
>>+++ linux/drivers/char/rtc.c	2005-08-05 13:26:48.000000000 +0000
>>@@ -1209,6 +1209,7 @@
>>
>>void rtc_get_rtc_time(struct rtc_time *rtc_tm)
>>{
>>+	unsigned long uip_watchdog = jiffies;
>>	unsigned char ctrl;
>>#ifdef CONFIG_MACH_DECSTATION
>>	unsigned int real_year;
>>@@ -1224,8 +1225,10 @@
>>	 * Once the read clears, read the RTC time (again via ioctl). Easy.
>>	 */
>>
>>-	if (rtc_is_updating() != 0)
>>-		msleep(20);
>>+	while (rtc_is_updating() != 0 && jiffies - uip_watchdog < 2*HZ/100) {
>>+		barrier();
>>+		cpu_relax();
>>+	}
>>
>>	/*
>>	 * Only the values that we read from the RTC are set. We leave
>>-
>>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/
>>
> 
> 
> Cheers,
> Dick Johnson
> Penguin : Linux version 2.6.12 on an i686 machine (5537.79 BogoMips).
> Warning : 98.36% of all statistics are fiction.
> .
> I apologize for the following. I tried to kill it with the above dot :
> 
> ****************************************************************
> The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.
> 
> Thank you.
> 

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[linux-os (Dick Johnson)]

On Fri, 5 Aug 2005, Petr Vandrovec wrote:

> linux-os (Dick Johnson) wrote:
>> On Fri, 5 Aug 2005, Petr Vandrovec wrote:
>>
>>
>>> Hello Linus,
>>> can you apply patch below?
>>>
>>> Since beginning of July my Opteron box was randomly crashing and being rebooted
>>> by hardware watchdog.  Today it finally did it in front of me, and this patch
>>> will hopefully fix it.
>>>
>>> Problem is that at the end of June (28th, commit
>>> 47f176fdaf8924bc83fddcf9658f2fd3ef60d573, [PATCH] Using msleep() instead of HZ)
>>> rtc_get_rtc_time was converted to use msleep() instead of busy waiting.  But
>>> rtc_get_rtc_time is used by hpet_rtc_interrupt, and scheduling is not allowed
>>> during interrupt.  So I'm reverting this part of original change, replacing
>>> msleep() back with busy loop.
>>>
>>> Original old code was busy waiting for 20ms, while on my hardware in the worst
>>> case update-in-progress bit was asserted for at most 363 passes through loop
>>> (on 2GHz dual Opteron), much less than even one jiffie, not even talking
>>> about 20ms.  So I changed code to just wait only as long as necessary.  Otherwise
>>> when RTC was set to generate 8192Hz timer, it stopped doing anything for
>>> 20ms (160 pulses were skipped!) from time to time, and this is rather suboptimal
>>> as far as I can tell.
>>>
>>> 						Thanks,
>>> 							Petr Vandrovec
>>>
>>>
>>
>>
>> It this is called from an interrupt (I hope not), jiffies will
>> not change unless you have two or more CPUs. Please don't do this.
>> Just spin while rtc is updating some maximum number of loops.
>
> It is called from rtc interrupt (hpet_rtc_interrupt calls it, if
> you really want I'll reproduce oopses about scheduling in the
> interrupt again and post it here for you convience).  But
> being inside rtc interrupt should not prevent jiffies from being
> incremented as system timer has higher priority than rtc.  We had
> code which looped around until jiffies expired for years and as far
> as I know nobody complained that boxes deadlock from time to time.
> I just added test for 'rtc_is_updating()' to the test.
> 							Petr
>

The fact that the timer interrupt has a higher priority is not
relevent. An interrupt service routine will not be interrupted
unless the interrupt service routine specifically enables
interrupts, something which should generally not be done
because a higher-priority interrupt will take the CPU away,
delaying the completion of the current interrupt.

If somebody needs to spin in an interrupt, basically not
a good thing to do either, they have no choice but to spin.
They can't "time-out", only exhaust a spin-count.

>>
>>
>>
>>> Signed-off-by: Petr Vandrovec <vandrove@vc.cvut.cz>
>>>
>>> diff -urdN linux/drivers/char/rtc.c linux/drivers/char/rtc.c
>>> --- linux/drivers/char/rtc.c	2005-08-05 12:43:54.000000000 +0000
>>> +++ linux/drivers/char/rtc.c	2005-08-05 13:26:48.000000000 +0000
>>> @@ -1209,6 +1209,7 @@
>>>
>>> void rtc_get_rtc_time(struct rtc_time *rtc_tm)
>>> {
>>> +	unsigned long uip_watchdog = jiffies;
>>> 	unsigned char ctrl;
>>> #ifdef CONFIG_MACH_DECSTATION
>>> 	unsigned int real_year;
>>> @@ -1224,8 +1225,10 @@
>>> 	 * Once the read clears, read the RTC time (again via ioctl). Easy.
>>> 	 */
>>>
>>> -	if (rtc_is_updating() != 0)
>>> -		msleep(20);
>>> +	while (rtc_is_updating() != 0 && jiffies - uip_watchdog < 2*HZ/100) {
>>> +		barrier();
>>> +		cpu_relax();
>>> +	}
>>>
>>> 	/*
>>> 	 * Only the values that we read from the RTC are set. We leave
>>> -
>>> 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/
>>>
>>
>>
>> Cheers,
>> Dick Johnson
>> Penguin : Linux version 2.6.12 on an i686 machine (5537.79 BogoMips).
>> Warning : 98.36% of all statistics are fiction.
>> .
>> I apologize for the following. I tried to kill it with the above dot :
>>
>> ****************************************************************
>> The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.
>>
>> Thank you.
>>
>
>
>

Cheers,
Dick Johnson
Penguin : Linux version 2.6.12 on an i686 machine (5537.79 BogoMips).
Warning : 98.36% of all statistics are fiction.
.
I apologize for the following. I tried to kill it with the above dot :

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[Andrew Morton]

Petr Vandrovec <vandrove@vc.cvut.cz> wrote:
>
> Since beginning of July my Opteron box was randomly crashing and being rebooted
>  by hardware watchdog.  Today it finally did it in front of me, and this patch
>  will hopefully fix it.
> 
>  Problem is that at the end of June (28th, commit 
>  47f176fdaf8924bc83fddcf9658f2fd3ef60d573, [PATCH] Using msleep() instead of HZ) 
>  rtc_get_rtc_time was converted to use msleep() instead of busy waiting.  But
>  rtc_get_rtc_time is used by hpet_rtc_interrupt, and scheduling is not allowed
>  during interrupt.  So I'm reverting this part of original change, replacing
>  msleep() back with busy loop.
> 
>  Original old code was busy waiting for 20ms, while on my hardware in the worst
>  case update-in-progress bit was asserted for at most 363 passes through loop
>  (on 2GHz dual Opteron), much less than even one jiffie, not even talking
>  about 20ms.  So I changed code to just wait only as long as necessary.  Otherwise
>  when RTC was set to generate 8192Hz timer, it stopped doing anything for
>  20ms (160 pulses were skipped!) from time to time, and this is rather suboptimal
>  as far as I can tell.

That's all pretty sad stuff.  I guess for now we can go back to the busy
loop.  Longer-term it would be nice if we could tune up the HPET driver in
some manner so we can avoid this busy-wait-in-interrupt.

I'm not sure who the HPET maintainer/expert is nowadays.  Robert Picco did
the original work but I haven't seen Robert around for a long time?

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[Venkatesh Pallipadi]

On Fri, Aug 05, 2005 at 11:53:29AM -0700, Andrew Morton wrote:
> 
> That's all pretty sad stuff.  I guess for now we can go back to the busy
> loop.  Longer-term it would be nice if we could tune up the HPET driver in
> some manner so we can avoid this busy-wait-in-interrupt.
> 
> I'm not sure who the HPET maintainer/expert is nowadays.  Robert Picco did
> the original work but I haven't seen Robert around for a long time?

Actually there are two parts in HPET. 
1) Using HPET for kernel timer and RTC emulation
2) HPET driver to export timers to user(/dev/hpet) and kernel drivers

We did the part (1) for i386 and I think Andi/Vojtech did (1) for x86_64. And 
Robert Picco did (2).

So, using rtc_get_rtc_time() in an interrupt handler will be my code. In this
part we try to emulate RTC interrupt using HPET and we have to read the current
RTC time in the interrupt handler. I can't think of any way of not doing
rtc_get_rtc_time here.

I think we should have two versions of rtc_get_rtc_time. One which does msleep,
that can be called from process context (in drivers/char/rtc.c) and one that
can be called from interrupt context (i386 and x86_64 hpet time routines). Or 
same routine behaving differently depending on where it is called from.

And for the hpet rtc emulation routines it should be OK even if the time is 
slightly off and not exact. So, probably we should be able to force read
rtc even when update is in progress. That way we can avoid the busy loop.
Unless RTC returns grossly wrong time values while UIP flag is set. I need to
look at RTC specs to verify that.

Thanks,
Venki

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[linux-os (Dick Johnson)]

On Fri, 5 Aug 2005, Venkatesh Pallipadi wrote:

> On Fri, Aug 05, 2005 at 11:53:29AM -0700, Andrew Morton wrote:
>>
>> That's all pretty sad stuff.  I guess for now we can go back to the busy
>> loop.  Longer-term it would be nice if we could tune up the HPET driver in
>> some manner so we can avoid this busy-wait-in-interrupt.
>>
>> I'm not sure who the HPET maintainer/expert is nowadays.  Robert Picco did
>> the original work but I haven't seen Robert around for a long time?
>
> Actually there are two parts in HPET.
> 1) Using HPET for kernel timer and RTC emulation
> 2) HPET driver to export timers to user(/dev/hpet) and kernel drivers
>
> We did the part (1) for i386 and I think Andi/Vojtech did (1) for x86_64. And
> Robert Picco did (2).
>
> So, using rtc_get_rtc_time() in an interrupt handler will be my code. In this
> part we try to emulate RTC interrupt using HPET and we have to read the current
> RTC time in the interrupt handler. I can't think of any way of not doing
> rtc_get_rtc_time here.
>
> I think we should have two versions of rtc_get_rtc_time. One which does msleep,
> that can be called from process context (in drivers/char/rtc.c) and one that
> can be called from interrupt context (i386 and x86_64 hpet time routines). Or
> same routine behaving differently depending on where it is called from.
>
> And for the hpet rtc emulation routines it should be OK even if the time is
> slightly off and not exact. So, probably we should be able to force read
> rtc even when update is in progress. That way we can avoid the busy loop.
> Unless RTC returns grossly wrong time values while UIP flag is set. I need to
> look at RTC specs to verify that.
>
> Thanks,
> Venki

The usual way is to read all time registers, save those values.
Read all registers again. Do this until the two consecutive reads
return the same values. You never have to busy-wait at all.
When I do this, I put the values read in two arrays, I memcmp()
them and, if not the same use memcpy() to copy new to old and
try again.


Cheers,
Dick Johnson
Penguin : Linux version 2.6.12 on an i686 machine (5537.79 BogoMips).
Warning : 98.36% of all statistics are fiction.
.
I apologize for the following. I tried to kill it with the above dot :

****************************************************************
The information transmitted in this message is confidential and may be privileged.  Any review, retransmission, dissemination, or other use of this information by persons or entities other than the intended recipient is prohibited.  If you are not the intended recipient, please notify Analogic Corporation immediately - by replying to this message or by sending an email to DeliveryErrors@analogic.com - and destroy all copies of this information, including any attachments, without reading or disclosing them.

Thank you.

Re: [PATCH 2.6.13-rc5-gitNOW] msleep() cannot be used from interrupt

[Andrew Morton]

Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> wrote:
>
> On Fri, Aug 05, 2005 at 11:53:29AM -0700, Andrew Morton wrote:
> > 
> > That's all pretty sad stuff.  I guess for now we can go back to the busy
> > loop.  Longer-term it would be nice if we could tune up the HPET driver in
> > some manner so we can avoid this busy-wait-in-interrupt.
> > 
> > I'm not sure who the HPET maintainer/expert is nowadays.  Robert Picco did
> > the original work but I haven't seen Robert around for a long time?
> 
> Actually there are two parts in HPET. 
> 1) Using HPET for kernel timer and RTC emulation
> 2) HPET driver to export timers to user(/dev/hpet) and kernel drivers
> 
> We did the part (1) for i386 and I think Andi/Vojtech did (1) for x86_64. And 
> Robert Picco did (2).

OK, thanks.

> So, using rtc_get_rtc_time() in an interrupt handler will be my code. In this
> part we try to emulate RTC interrupt using HPET and we have to read the current
> RTC time in the interrupt handler. I can't think of any way of not doing
> rtc_get_rtc_time here.
>
> I think we should have two versions of rtc_get_rtc_time. One which does msleep,
> that can be called from process context (in drivers/char/rtc.c) and one that
> can be called from interrupt context (i386 and x86_64 hpet time routines). Or 
> same routine behaving differently depending on where it is called from.

Yes, we could do that.  But when one is chasing "worst-case latency", we
need to address the worst-case machine, as well as the worst-case
codepath...

> And for the hpet rtc emulation routines it should be OK even if the time is 
> slightly off and not exact. So, probably we should be able to force read
> rtc even when update is in progress. That way we can avoid the busy loop.
> Unless RTC returns grossly wrong time values while UIP flag is set. I need to
> look at RTC specs to verify that.

Yup, no hurry.  If we can improve that codepath sometime it'd be nice, thanks.