to fix xtime lock for in the RT kernel patch
diff mbox series

Message ID 41F04573.7070508@mvista.com
State New, archived
Headers show
Series
  • to fix xtime lock for in the RT kernel patch
Related show

Commit Message

George Anzinger Jan. 20, 2005, 11:57 p.m. UTC
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.

Comments

tip-bot for Ingo Molnar Jan. 21, 2005, 6:35 a.m. UTC | #1
* George Anzinger <george@mvista.com> 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
-
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/
George Anzinger Jan. 21, 2005, 8:16 a.m. UTC | #2
Ingo Molnar wrote:
> * George Anzinger <george@mvista.com> 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
>
tip-bot for Ingo Molnar Jan. 21, 2005, 8:21 a.m. UTC | #3
* George Anzinger <george@mvista.com> 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
-
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/
George Anzinger Jan. 21, 2005, 8:39 a.m. UTC | #4
Ingo Molnar wrote:
> * George Anzinger <george@mvista.com> 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
>
tip-bot for Ingo Molnar Jan. 21, 2005, 8:45 a.m. UTC | #5
* George Anzinger <george@mvista.com> 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
-
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/
George Anzinger Jan. 21, 2005, 8:54 a.m. UTC | #6
Ingo Molnar wrote:
> * George Anzinger <george@mvista.com> 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
>
tip-bot for Ingo Molnar Jan. 21, 2005, 9 a.m. UTC | #7
* George Anzinger <george@mvista.com> 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
-
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/
George Anzinger Jan. 21, 2005, 9:08 a.m. UTC | #8
Ingo Molnar wrote:
> * George Anzinger <george@mvista.com> 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 Jan. 27, 2005, 8:53 p.m. UTC | #9
George Anzinger wrote:
> Ingo Molnar wrote:
> 
>> * George Anzinger <george@mvista.com> 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.
tip-bot for Ingo Molnar Jan. 28, 2005, 4:35 a.m. UTC | #10
* George Anzinger <george@mvista.com> 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
-
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/

Patch
diff mbox series

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 <cananian@alumni.princeton.edu>, Andrew D.
- *	Balsa <andrebalsa@altern.org>, Philip Gladstone <philip@raptor.com>;
+ * 	Balsa <andrebalsa@altern.org>, Philip Gladstone <philip@raptor.com>;
  *	ported from 2.0.35 Jumbo-9 by Michael Krause <m.krause@tu-harburg.de>).
  * 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);