to fix xtime lock for in the RT kernel patch

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.

George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

Source: MontaVista Software, Inc. George Anzinger <[email protected]>
Type: Defect Fix
Keywords:

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
===================================================================
@@ -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
===================================================================
@@ -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
===================================================================
@@ -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);
 

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

@@ -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);
 

@@ -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

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.

George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

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

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.

George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

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.

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

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.

George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

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

Tomorrow, uh, later today. Need some sleep now...

George Anzinger   [email protected]
High-res-timers:  http://sourceforge.net/projects/high-res-timers/

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/

sure, fine with me. You might want to sync up with Thomas Gleixner, who's working on some of the HRT issues too.

Ingo