Source: MontaVista Software, Inc. George Anzinger <[email protected]>
Type: Defect Fix
Keywords:
Signed-off-by: George Anzinger <[email protected]>
Description:
This patch changes the timer interrupt code for the RT patch to
respect the xtime_lock which should protect jiffies and to collect
offset information on jiffies interrupts. This offset info must
be collected as soon as possible during the jiffies interrupt and
also needs to be protected by the xtime_lock.
The xtime_lock is thus a "raw" lock.
arch/i386/kernel/time.c | 8 +++++---
include/linux/time.h | 2 +-
kernel/timer.c | 2 +-
3 files changed, 7 insertions(+), 5 deletions(-)
Index: topdir/kernel/timer.c
===================================================================
--- topdir.orig/kernel/timer.c
+++ topdir/kernel/timer.c
@@ -946,7 +946,7 @@ unsigned long wall_jiffies = INITIAL_JIF
* playing with xtime and avenrun.
*/
#ifndef ARCH_HAVE_XTIME_LOCK
-DECLARE_SEQLOCK(xtime_lock);
+DECLARE_RAW_SEQLOCK(xtime_lock);
EXPORT_SYMBOL(xtime_lock);
#endif
Index: topdir/include/linux/time.h
===================================================================
--- topdir.orig/include/linux/time.h
+++ topdir/include/linux/time.h
@@ -80,7 +80,7 @@ mktime (unsigned int year, unsigned int
extern struct timespec xtime;
extern struct timespec wall_to_monotonic;
-extern seqlock_t xtime_lock;
+extern raw_seqlock_t xtime_lock;
static inline unsigned long get_seconds(void)
{
Index: topdir/arch/i386/kernel/time.c
===================================================================
--- topdir.orig/arch/i386/kernel/time.c
+++ topdir/arch/i386/kernel/time.c
@@ -20,7 +20,7 @@
* monotonic gettimeofday() with fast_get_timeoffset(),
* drift-proof precision TSC calibration on boot
* (C. Scott Ananian <[email protected]>, Andrew D.
- * Balsa <[email protected]>, Philip Gladstone <[email protected]>;
+ * Balsa <[email protected]>, Philip Gladstone <[email protected]>;
* ported from 2.0.35 Jumbo-9 by Michael Krause <[email protected]>).
* 1998-12-16 Andrea Arcangeli
* Fixed Jumbo-9 code in 2.1.131: do_gettimeofday was missing 1 jiffy
@@ -224,7 +224,10 @@ EXPORT_SYMBOL(profile_pc);
*/
void direct_timer_interrupt(struct pt_regs *regs)
{
+ write_seqlock(&xtime_lock);
+ cur_timer->mark_offset();
do_timer_interrupt_hook(regs);
+ write_sequnlock(&xtime_lock);
}
#endif
@@ -254,6 +257,7 @@ static inline void do_timer_interrupt(in
#endif
#ifndef CONFIG_PREEMPT_HARDIRQS
+ cur_timer->mark_offset();
do_timer_interrupt_hook(regs);
#endif
@@ -312,8 +316,6 @@ irqreturn_t timer_interrupt(int irq, voi
* locally disabled. -arca
*/
write_seqlock(&xtime_lock);
-
- cur_timer->mark_offset();
do_timer_interrupt(irq, NULL, regs);
* George Anzinger <[email protected]> wrote:
> It seems to me that we need to either do the attached or to rewrite
> the timer front end code to just gather the offset info and defer to
> the timer irq thread to update jiffies and the offset stuff. In
> either case we really can not split the two and we do need the
> xtime_lock protection.
how about the patch below? One of the important benefits of the threaded
timer IRQ is the ability to make xtime_lock a mutex.
Ingo
--- linux/arch/i386/kernel/time.c.orig2
+++ linux/arch/i386/kernel/time.c
@@ -313,6 +313,7 @@ irqreturn_t timer_interrupt(int irq, voi
write_seqlock(&xtime_lock);
cur_timer->mark_offset();
+ do_timer(regs);
do_timer_interrupt(irq, NULL, regs);
--- linux/include/asm-i386/mach-default/do_timer.h.orig2
+++ linux/include/asm-i386/mach-default/do_timer.h
@@ -16,7 +16,6 @@
static inline void do_timer_interrupt_hook(struct pt_regs *regs)
{
- do_timer(regs);
#ifndef CONFIG_SMP
update_process_times(user_mode(regs));
#endif
Ingo Molnar wrote:
> * George Anzinger <[email protected]> wrote:
>
>
>>It seems to me that we need to either do the attached or to rewrite
>>the timer front end code to just gather the offset info and defer to
>>the timer irq thread to update jiffies and the offset stuff. In
>>either case we really can not split the two and we do need the
>>xtime_lock protection.
>
>
> how about the patch below? One of the important benefits of the threaded
> timer IRQ is the ability to make xtime_lock a mutex.
The problem is that that removes the
cur_timer->mark_offset();
do_timer(regs);
in time. If this were accompanied by code in the actual interrupt path that
grabbed the offset info and the above was deferred to the irq thread I think it
would be much better.
Something like:
cur_timer->get_offset_info();
in the interrupt path and then, in the thread:
cur_timer->apply_offset_info();
In the non-RT system we would just do:
cur_timer->get_offset_info();
cur_timer->apply_offset_info();
I think then we should let the apply code do the do_timer(regs) call as well as
it is already messing with jiffies to correct for lost ticks. An interesting
point here is what to actually pass for "regs". I suspect we would like the
get_offset code to grab the relevant part of regs as well. But then, we only
really use regs in the accounting path which is else where on SMP machines.
This then bubbles up into a change in the common code (do_timer()) but we
already have that.
I could code this up if you like. Might want to get John Stultz's two cents on
all this.
A related change I made in the HRT SMP code is to not use the local apic timers
to kick off the accounting and run_timers code, but to define a new IPI. In my
current HRT code I send an IPI to all but self right after the jiffies update
(calling the current cpus accounting code right after the IPI request). I
suspect the right thing to do here is to make that an IPI to all cpus, including
self. The reason for this change is to get the run_timers code to run as close
to the jiffies update as possible. This is required to get reasonable high res
timers.
>
> Ingo
>
> --- linux/arch/i386/kernel/time.c.orig2
> +++ linux/arch/i386/kernel/time.c
> @@ -313,6 +313,7 @@ irqreturn_t timer_interrupt(int irq, voi
> write_seqlock(&xtime_lock);
>
> cur_timer->mark_offset();
> + do_timer(regs);
>
> do_timer_interrupt(irq, NULL, regs);
>
> --- linux/include/asm-i386/mach-default/do_timer.h.orig2
> +++ linux/include/asm-i386/mach-default/do_timer.h
> @@ -16,7 +16,6 @@
>
> static inline void do_timer_interrupt_hook(struct pt_regs *regs)
> {
> - do_timer(regs);
> #ifndef CONFIG_SMP
> update_process_times(user_mode(regs));
> #endif
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
* George Anzinger <[email protected]> wrote:
> > how about the patch below? One of the important benefits of the
> > threaded timer IRQ is the ability to make xtime_lock a mutex.
>
> The problem is that that removes the
> cur_timer->mark_offset();
> do_timer(regs);
> in time. [...]
i'm not sure i understand what you mean. My change does:
| @@ -294,6 +313,7 @@ irqreturn_t timer_interrupt(int irq, voi
| write_seqlock(&xtime_lock);
|
| cur_timer->mark_offset();
| + do_timer(regs);
|
| do_timer_interrupt(irq, NULL, regs);
so ->mark_offset and do_timer() go together, and happen under
xtime_lock. What problem is there if we do this?
Ingo
Ingo Molnar wrote:
> * George Anzinger <[email protected]> wrote:
>
>
>>>how about the patch below? One of the important benefits of the
>>>threaded timer IRQ is the ability to make xtime_lock a mutex.
>>
>>The problem is that that removes the
>> cur_timer->mark_offset();
>> do_timer(regs);
>>in time. [...]
>
>
> i'm not sure i understand what you mean. My change does:
>
> | @@ -294,6 +313,7 @@ irqreturn_t timer_interrupt(int irq, voi
> | write_seqlock(&xtime_lock);
> |
> | cur_timer->mark_offset();
> | + do_timer(regs);
> |
> | do_timer_interrupt(irq, NULL, regs);
>
> so ->mark_offset and do_timer() go together, and happen under
> xtime_lock. What problem is there if we do this?
We are trying to get an accurate picture of when, exactly in time, jiffies
changes. We then want to have that marked (mark_offset) with a TCS (or other
clock) so we can tell how many nanoseconds past that time any given point of
time is. This is used by gettimeofday. So if we wait till the thread gets
control, we have a lot of variability in when, exactly, the event took place.
We already have interrupt latency in the mix, but, by moving it to a thread, we
also add scheduling delays due to other RT threads (the actual intent of making
it a thread, right).
We can handle (do today) some variability in this area, but, at least for RT
systems, we would like to get this down to a small a window as possible. The
changes I am suggesting are aimed at getting a good a handle on the current time
as possible. They say nothing about how accurate we are in expiring a timer,
for example.
>
> Ingo
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
* George Anzinger <[email protected]> wrote:
> > so ->mark_offset and do_timer() go together, and happen under
> > xtime_lock. What problem is there if we do this?
>
> We are trying to get an accurate picture of when, exactly in time,
> jiffies changes. [...]
but that's the point of allowing the threading of the timer interrupt.
If you _have_ an interrupt source (and task) that _is_ more important
than the timer interrupt then so be it. Yes, the accuracy of timekeeping
may suffer.
so everything is relative, and the user decides which functionality
should have the better latency. do_offset() can take up to 10 usecs so
it's a latency source i'd like to keep out of the direct IRQ path, as
much as possible.
> We can handle (do today) some variability in this area, but, at least
> for RT systems, we would like to get this down to a small a window as
> possible.
by default the timer interrupt has the highest priority, and you can
still change it to prio 99 to avoid any potential impact from RT tasks
or other interrupt threads.
Ingo
Ingo Molnar wrote:
> * George Anzinger <[email protected]> wrote:
>
>
>>>so ->mark_offset and do_timer() go together, and happen under
>>>xtime_lock. What problem is there if we do this?
>>
>>We are trying to get an accurate picture of when, exactly in time,
>>jiffies changes. [...]
>
>
> but that's the point of allowing the threading of the timer interrupt.
> If you _have_ an interrupt source (and task) that _is_ more important
> than the timer interrupt then so be it. Yes, the accuracy of timekeeping
> may suffer.
>
> so everything is relative, and the user decides which functionality
> should have the better latency. do_offset() can take up to 10 usecs so
> it's a latency source i'd like to keep out of the direct IRQ path, as
> much as possible.
What I am suggesting is spliting the mark code so that it would only grap the
offset (current TSC in most systems) during interrupt processing. Applying this
would be done later in the thread. Since it is not applying the offset, the
xtime_lock would not need to be taken.
>
>
>>We can handle (do today) some variability in this area, but, at least
>>for RT systems, we would like to get this down to a small a window as
>>possible.
>
>
> by default the timer interrupt has the highest priority, and you can
> still change it to prio 99 to avoid any potential impact from RT tasks
> or other interrupt threads.
>
> Ingo
>
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
* George Anzinger <[email protected]> wrote:
> What I am suggesting is spliting the mark code so that it would only
> grap the offset (current TSC in most systems) during interrupt
> processing. Applying this would be done later in the thread. Since
> it is not applying the offset, the xtime_lock would not need to be
> taken.
ok, you are right, and this would be fine with me. Wanna take a shot at
it? I've uploaded the -03 patch which is my most current tree. (with the
do_timer() moving done already.) I've reviewed the TSC offset codepath
again and i'm not sure where i got the 10 usecs from ... it's a pretty
cheap codepath that can be done in the direct interrupt just fine.
Ingo
Ingo Molnar wrote:
> * George Anzinger <[email protected]> wrote:
>
>
>>What I am suggesting is spliting the mark code so that it would only
>>grap the offset (current TSC in most systems) during interrupt
>>processing. Applying this would be done later in the thread. Since
>>it is not applying the offset, the xtime_lock would not need to be
>>taken.
>
>
> ok, you are right, and this would be fine with me. Wanna take a shot at
> it? I've uploaded the -03 patch which is my most current tree. (with the
> do_timer() moving done already.) I've reviewed the TSC offset codepath
> again and i'm not sure where i got the 10 usecs from ... it's a pretty
> cheap codepath that can be done in the direct interrupt just fine.
>
Tomorrow, uh, later today. Need some sleep now...
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
George Anzinger wrote:
> Ingo Molnar wrote:
>
>> * George Anzinger <[email protected]> wrote:
>>
>>
>>> What I am suggesting is spliting the mark code so that it would only
>>> grap the offset (current TSC in most systems) during interrupt
>>> processing. Applying this would be done later in the thread. Since
>>> it is not applying the offset, the xtime_lock would not need to be
>>> taken.
>>
>>
>>
>> ok, you are right, and this would be fine with me. Wanna take a shot at
>> it? I've uploaded the -03 patch which is my most current tree. (with the
>> do_timer() moving done already.) I've reviewed the TSC offset codepath
>> again and i'm not sure where i got the 10 usecs from ... it's a pretty
>> cheap codepath that can be done in the direct interrupt just fine.
>>
> Tomorrow, uh, later today. Need some sleep now...
Ingo, I have been looking at the code being proposed by John Stultz. It looks
like it handles all the issues I am talking about here. I think it would be
best to leave the RT patch as it is WRT this issue and work on getting John's
patch ready for prime time as any work I would do here will just get tossed when
his patch hits the steet.
Meanwhile, I will (already have) get HRT working on RT and make that available
in the next few days.
--
George Anzinger [email protected]
High-res-timers: http://sourceforge.net/projects/high-res-timers/
* George Anzinger <[email protected]> wrote:
> Ingo, I have been looking at the code being proposed by John Stultz.
> It looks like it handles all the issues I am talking about here. I
> think it would be best to leave the RT patch as it is WRT this issue
> and work on getting John's patch ready for prime time as any work I
> would do here will just get tossed when his patch hits the steet.
>
> Meanwhile, I will (already have) get HRT working on RT and make that
> available in the next few days.
sure, fine with me. You might want to sync up with Thomas Gleixner,
who's working on some of the HRT issues too.
Ingo