[patch] block: fix blktrace timestamps

[Ingo Molnar]

(David, could you try the patch further below - does it fix bkltrace 
timestamps too?)

* Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:

> On Fri, Jan 11 2008, Ingo Molnar wrote:
> > 
> > * Jens Axboe <jens.axboe@xxxxxxxxxx> wrote:
> > 
> > > > they are from the scheduler git tree (except the first debug patch), 
> > > > but queued up for v2.6.25 at the moment.
> > > 
> > > So this means that blktrace will be broken with CONFIG_NO_HZ for 
> > > 2.6.24? That's clearly a regression.
> > 
> > 64-bit CONFIG_NO_HZ is a new feature in v2.6.24. If it happens on 
> > 32-bit too and it didnt happen in v2.6.23 32-bit then it's a 
> > regression.
> 
> If blktrace worked in 2.6.23 and it doesn't in 2.6.24 because of some 
> option that isn't immediately apparent, then it's a regression. 
> Period.

not completely correct. CONFIG_NO_HZ is a default-disabled option that 
became newly available on 64-bit x86. So if NO_HZ does not completely 
work on 64-bit, and if 32-bit works fine - which we dont know yet (my 
guess would be that it's similarly broken on the same box) then it's not 
a regression.

But even if it's not "technically" a regression, it's something we want 
to fix in .24 if we can, so i'm all with you Jens :)

and you can see that by looking at the patch. It does not unbreak 
something that 2.6.24 broke. It makes something work better that used to 
be so broken for such a long time. (sched_clock() based blktrace)

anyway, the most dangerous one of these patches has become well-tested 
meanwhile in x86.git. So maybe we can get this fix into v2.6.24. Maybe 
not. We'll see.

> > all this comes from blktrace's original decision of using sched_clock()
> > :-) It's not a global timesource and it's not trivial to turn it into a
> > halfways usable global timesource.
> 
> Hey, it was a high res time source and the only one easily available 
> :) I'm fine with using another timesource, I'll take suggestions or 
> patches any day!

Correction: it was not a high res time source, it was "the scheduler's 
per-cpu, non-exported, non-coherent, warps-and-jumps-like-hell high-res 
timesource that was intentionally called the _sched_ clock" ;-)

ktime_get() should have been used instead, which is a proper GTOD 
clocksource. The patch below implements this.

	Ingo

------------->
Subject: block: fix blktrace timestamps
From: Ingo Molnar <mingo@xxxxxxx>

David Dillow reported broken blktrace timestamps. The reason
is cpu_clock() which is not a global time source.

Fix bkltrace timestamps by using ktime_get() like the networking
code does for packet timestamps. This also removes a whole lot
of complexity from bkltrace.c and shrinks the code by 500 bytes:

   text    data     bss     dec     hex filename
   2888     124      44    3056     bf0 blktrace.o.before
   2390     116      44    2550     9f6 blktrace.o.after

Signed-off-by: Ingo Molnar <mingo@xxxxxxx>
---
 block/blktrace.c |   69 +------------------------------------------------------
 1 file changed, 2 insertions(+), 67 deletions(-)

Index: linux-x86.q/block/blktrace.c
===================================================================
--- linux-x86.q.orig/block/blktrace.c
+++ linux-x86.q/block/blktrace.c
@@ -25,7 +25,6 @@
 #include <linux/time.h>
 #include <asm/uaccess.h>
 
-static DEFINE_PER_CPU(unsigned long long, blk_trace_cpu_offset) = { 0, };
 static unsigned int blktrace_seq __read_mostly = 1;
 
 /*
@@ -41,7 +40,7 @@ static void trace_note(struct blk_trace 
 		const int cpu = smp_processor_id();
 
 		t->magic = BLK_IO_TRACE_MAGIC | BLK_IO_TRACE_VERSION;
-		t->time = cpu_clock(cpu) - per_cpu(blk_trace_cpu_offset, cpu);
+		t->time = ktime_to_ns(ktime_get());
 		t->device = bt->dev;
 		t->action = action;
 		t->pid = pid;
@@ -159,7 +158,7 @@ void __blk_add_trace(struct blk_trace *b
 
 		t->magic = BLK_IO_TRACE_MAGIC | BLK_IO_TRACE_VERSION;
 		t->sequence = ++(*sequence);
-		t->time = cpu_clock(cpu) - per_cpu(blk_trace_cpu_offset, cpu);
+		t->time = ktime_to_ns(ktime_get());
 		t->sector = sector;
 		t->bytes = bytes;
 		t->action = what;
@@ -506,73 +505,9 @@ void blk_trace_shutdown(struct request_q
 	}
 }
 
-/*
- * Average offset over two calls to cpu_clock() with a gettimeofday()
- * in the middle
- */
-static void blk_check_time(unsigned long long *t, int this_cpu)
-{
-	unsigned long long a, b;
-	struct timeval tv;
-
-	a = cpu_clock(this_cpu);
-	do_gettimeofday(&tv);
-	b = cpu_clock(this_cpu);
-
-	*t = tv.tv_sec * 1000000000 + tv.tv_usec * 1000;
-	*t -= (a + b) / 2;
-}
-
-/*
- * calibrate our inter-CPU timings
- */
-static void blk_trace_check_cpu_time(void *data)
-{
-	unsigned long long *t;
-	int this_cpu = get_cpu();
-
-	t = &per_cpu(blk_trace_cpu_offset, this_cpu);
-
-	/*
-	 * Just call it twice, hopefully the second call will be cache hot
-	 * and a little more precise
-	 */
-	blk_check_time(t, this_cpu);
-	blk_check_time(t, this_cpu);
-
-	put_cpu();
-}
-
-static void blk_trace_set_ht_offsets(void)
-{
-#if defined(CONFIG_SCHED_SMT)
-	int cpu, i;
-
-	/*
-	 * now make sure HT siblings have the same time offset
-	 */
-	preempt_disable();
-	for_each_online_cpu(cpu) {
-		unsigned long long *cpu_off, *sibling_off;
-
-		for_each_cpu_mask(i, per_cpu(cpu_sibling_map, cpu)) {
-			if (i == cpu)
-				continue;
-
-			cpu_off = &per_cpu(blk_trace_cpu_offset, cpu);
-			sibling_off = &per_cpu(blk_trace_cpu_offset, i);
-			*sibling_off = *cpu_off;
-		}
-	}
-	preempt_enable();
-#endif
-}
-
 static __init int blk_trace_init(void)
 {
 	mutex_init(&blk_tree_mutex);
-	on_each_cpu(blk_trace_check_cpu_time, NULL, 1, 1);
-	blk_trace_set_ht_offsets();
 
 	return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/