Skip to contentSkip to conversation

sched_clock(): cleanups

PATCH In review Ingo Molnar 11 participants 59 messages May 25, 2007 – May 28, 2007

lore
Share & tools
lore

Ingo Molnar

author
#0 lore raw

PATCH 1/1 sched_clock(): cleanups

Subject: [patch] sched_clock(): cleanups
From: Ingo Molnar <[email protected]>

clean up sched-clock.c - mostly comment style fixes.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/sched-clock.c |   79 ++++++++++++++++++++++++++---------------
 1 file changed, 51 insertions(+), 28 deletions(-)
arch/i386/kernel/sched-clock.c +51 -28
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/sched-clock.c
===================================================================
@@ -60,18 +60,20 @@ DEFINE_PER_CPU(struct sc_data, sc_data) 
  */
 unsigned long long native_sched_clock(void)
 {
-	unsigned long long r;
 	struct sc_data *sc = &get_cpu_var(sc_data);
+	unsigned long long r;
+	unsigned long flags;
 
 	if (sc->unstable) {
-		unsigned long flags;
 		r = (jiffies_64 - sc->sync_base) * (1000000000 / HZ);
 		r += sc->ns_base;
 		local_irq_save(flags);
-		/* last_val is used to avoid non monotonity on a
-		   stable->unstable transition. Make sure the time
-		   never goes to before the last value returned by
-		   the TSC clock */
+		/*
+		 * last_val is used to avoid non monotonity on a
+		 * stable->unstable transition. Make sure the time
+		 * never goes to before the last value returned by
+		 * the TSC clock
+		 */
 		if (r <= sc->last_val)
 			r = sc->last_val + 1;
 		sc->last_val = r;

@@ -87,8 +89,10 @@ unsigned long long native_sched_clock(vo
 	return r;
 }
 
-/* We need to define a real function for sched_clock, to override the
-   weak default version */
+/*
+ * We need to define a real function for sched_clock, to override the
+ * weak default version
+ */
 #ifdef CONFIG_PARAVIRT
 unsigned long long sched_clock(void)
 {

@@ -101,9 +105,11 @@ unsigned long long sched_clock(void)
 
 static int no_sc_for_printk;
 
-/* printk clock: when it is known the sc results are very non monotonic
-   fall back to jiffies for printk. Other sched_clock users are supposed
-   to handle this. */
+/*
+ * printk clock: when it is known the sc results are very non monotonic
+ * fall back to jiffies for printk. Other sched_clock users are supposed
+ * to handle this.
+ */
 unsigned long long printk_clock(void)
 {
 	if (unlikely(no_sc_for_printk))

@@ -119,15 +125,19 @@ static void resync_sc_freq(struct sc_dat
 		sc->unstable = 1;
 		return;
 	}
-	/* Handle nesting, but when we're zero multiple calls in a row
-	   are ok too and not a bug */
+	/*
+	 * Handle nesting, but when we're zero multiple calls in a row
+	 * are ok too and not a bug
+	 */
 	if (sc->unstable > 0)
 		sc->unstable--;
 	if (sc->unstable)
 		return;
-	/* RED-PEN protect with seqlock? I hope that's not needed
-	   because sched_clock callers should be able to tolerate small
-	   errors. */
+	/*
+	 * RED-PEN protect with seqlock? I hope that's not needed
+	 * because sched_clock callers should be able to tolerate small
+	 * errors.
+	 */
 	sc->ns_base = ktime_to_ns(ktime_get());
 	rdtscll(sc->sync_base);
 	sc->cyc2ns_scale = (1000000 << CYC2NS_SCALE_FACTOR) / newfreq;

@@ -137,6 +147,7 @@ static void call_r_s_f(void *arg)
 {
 	struct cpufreq_freqs *freq = arg;
 	unsigned f = freq->new;
+
 	if (!f)
 		f = cpufreq_get(freq->cpu);
 	if (!f)

@@ -146,7 +157,9 @@ static void call_r_s_f(void *arg)
 
 static void call_r_s_f_here(void *arg)
 {
-	struct cpufreq_freqs f = { .cpu = get_cpu(), .new = 0 };
+	struct cpufreq_freqs f = { .new = 0 };
+
+	f.cpu = get_cpu();
 	call_r_s_f(&f);
 	put_cpu();
 }

@@ -155,9 +168,11 @@ static int sc_freq_event(struct notifier
 			 void *data)
 {
 	struct cpufreq_freqs *freq = data;
-	int cpu = get_cpu();
-	struct sc_data *sc = &per_cpu(sc_data, cpu);
+	struct sc_data *sc;
+	int cpu;
 
+	cpu = get_cpu();
+	sc = &per_cpu(sc_data, cpu);
 	if (cpu_has(&cpu_data[cpu], X86_FEATURE_CONSTANT_TSC))
 		goto out;
 	if (freq->old == freq->new)

@@ -167,16 +182,20 @@ static int sc_freq_event(struct notifier
 	case CPUFREQ_SUSPENDCHANGE:
 		/* Mark TSC unstable during suspend/resume */
 	case CPUFREQ_PRECHANGE:
-		/* Mark TSC as unstable until cpu frequency change is done
-		   because we don't know when exactly it will change.
-		   unstable in used as a counter to guard against races
-		   between the cpu frequency notifiers and normal resyncs */
+		/*
+		 * Mark TSC as unstable until cpu frequency change is done
+		 * because we don't know when exactly it will change.
+		 * unstable in used as a counter to guard against races
+		 * between the cpu frequency notifiers and normal resyncs
+		 */
 		sc->unstable++;
 		/* FALL THROUGH */
 	case CPUFREQ_RESUMECHANGE:
 	case CPUFREQ_POSTCHANGE:
-		/* Frequency change or resume is done -- update everything and
-		   mark TSC as stable again. */
+		/*
+		 * Frequency change or resume is done -- update everything
+		 * and mark TSC as stable again.
+		 */
 		if (cpu == freq->cpu)
 			resync_sc_freq(sc, freq->new);
 		else

@@ -186,6 +205,7 @@ static int sc_freq_event(struct notifier
 	}
 out:
 	put_cpu();
+
 	return NOTIFY_DONE;
 }
 

@@ -197,6 +217,7 @@ static int __cpuinit
 sc_cpu_event(struct notifier_block *self, unsigned long event, void *hcpu)
 {
 	long cpu = (long)hcpu;
+
 	if (event == CPU_ONLINE) {
 		struct cpufreq_freqs f = { .cpu = cpu, .new = 0 };
 		smp_call_function_single(cpu, call_r_s_f, &f, 0, 1);

@@ -209,13 +230,15 @@ static __init int init_sched_clock(void)
 	if (unsynchronized_tsc())
 		no_sc_for_printk = 1;
 
-	/* On a race between the various events the initialization might be
-	   done multiple times, but code is tolerant to this */
+	/*
+	 * On a race between the various events the initialization might be
+	 * done multiple times, but code is tolerant to this
+	 */
 	cpufreq_register_notifier(&sc_freq_notifier,
 				CPUFREQ_TRANSITION_NOTIFIER);
 	hotcpu_notifier(sc_cpu_event, 0);
 	on_each_cpu(call_r_s_f_here, NULL, 0, 0);
+
 	return 0;
 }
 core_initcall(init_sched_clock);
-
#1 lore raw

Hi Ingo,

arch/i386/kernel/sched-clock.c static void call_r_s_f(void *arg) · line 153

Quoted 9 lines · Ingo Molnar

[...]

arch/i386/kernel/sched-clock.c +1 -0
@@ -137,6 +147,7 @@ static void call_r_s_f(void *arg)
 {
        struct cpufreq_freqs *freq = arg;
        unsigned f = freq->new;
+
        if (!f)
                f = cpufreq_get(freq->cpu);
        if (!f)

if (!f)

	f = cpufreq_get(freq->cpu);
if (!f)
	f = tsc_khz;

?

Something's not quite right here :-)

Satyam

author
#2 lore raw
Quoted 14 lines · Satyam Sharma

Hi Ingo,

arch/i386/kernel/sched-clock.c static void call_r_s_f(void *arg) · line 153

Quoted 9 lines · Ingo Molnar

[...]

arch/i386/kernel/sched-clock.c +1 -0
@@ -137,6 +147,7 @@ static void call_r_s_f(void *arg)
 {
       struct cpufreq_freqs *freq = arg;
       unsigned f = freq->new;
+
       if (!f)
               f = cpufreq_get(freq->cpu);
       if (!f)

if (!f)

Quoted 7 lines
	f = cpufreq_get(freq->cpu);
if (!f)
	f = tsc_khz;

?

Something's not quite right here :-)

yeah, that looks like a real bug. When i looked at it during the cleanup i guess i got distracted by the descriptive function name of call_r_s_f() ... :-/

Ingo

author
#3 lore raw
Quoted 9 lines
Quoted 9 lines

if (!f)

	f = cpufreq_get(freq->cpu);
if (!f)
	f = tsc_khz;

?

Something's not quite right here :-)

ah, that's fine. It does this: 'try to give f a value', and then: 'if still no value then give it tsc_khz as a last resort)

call_r_s_f() still needs an urgent rerenaming though =B-)

Ingo

#4 lore raw
Quoted 6 lines · Ingo Molnar
Quoted 6 lines
Quoted 6 lines

if (!f)

    f = cpufreq_get(freq->cpu);
if (!f)
    f = tsc_khz;
Quoted 6 lines
Quoted 3 lines
Quoted 3 lines

?

Something's not quite right here :-)

ah, that's fine. It does this: 'try to give f a value', and then: 'if still no value then give it tsc_khz as a last resort)

Ugh, yes, didn't know cpufreq_get can return 0.

Quoted 1 line

call_r_s_f() still needs an urgent rerenaming though =B-)

So does "call_r_s_f_here()" :-)

Satyam

#5 lore raw
Quoted 5 lines · Satyam Sharma
Quoted 1 line · Ingo Molnar

call_r_s_f() still needs an urgent rerenaming though =B-)

So does "call_r_s_f_here()" :-)

That name makes me think of INTERCAL's 'DO COME FROM' statement. And any code that makes one think of INTERCAL is say,.. special.. :-)

#6 lore raw
Quoted 9 lines · Peter Zijlstra
Quoted 5 lines · Satyam Sharma
Quoted 1 line · Ingo Molnar

call_r_s_f() still needs an urgent rerenaming though =B-)

So does "call_r_s_f_here()" :-)

That name makes me think of INTERCAL's 'DO COME FROM' statement. And any code that makes one think of INTERCAL is say,.. special.. :-)

Propose a better way to code this then? It's not my fault that dealing with callbacks in C is so messy. _here just massages one callback prototype (smp_call_function's) into another (cpufreq's) because both callbacks do the same in this case.

The r_s_f BTW stands for resync_sc_freq which is a function earlier in the file and should be familiar to a serious reader.

-Andi

author
#7 lore raw
Quoted 4 lines · Andi Kleen

Propose a better way to code this then? It's not my fault that dealing with callbacks in C is so messy. _here just massages one callback prototype (smp_call_function's) into another (cpufreq's) because both callbacks do the same in this case.

see the last iteration of the cleanups i did. Naming the function after what it does, and prefixing the preempt-unsafe one __ does the trick.

Quoted 2 lines

The r_s_f BTW stands for resync_sc_freq which is a function earlier in the file and should be familiar to a serious reader.

I consider myself a serious reader and it wasnt obvious to me. Names must always be descriptive, we cannot hold all the details in our heads all the time.

Ingo

#8 lore raw
In reply to Andi Kleen
Quoted 8 lines · Andi Kleen
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Satyam Sharma
Quoted 1 line · Ingo Molnar

call_r_s_f() still needs an urgent rerenaming though =B-)

So does "call_r_s_f_here()" :-)

Quoted 7 lines
Quoted 2 lines

That name makes me think of INTERCAL's 'DO COME FROM' statement. And any code that makes one think of INTERCAL is say,.. special.. :-)

Propose a better way to code this then? It's not my fault that dealing with callbacks in C is so messy. _here just massages one callback prototype (smp_call_function's) into another (cpufreq's) because both callbacks do the same in this case.

I see you point; however a function called:
call_<some_other_function>_here() just doesn't make sense. It says as much as: we should call some_other_function() but for some reason we dont.

Quoted 2 lines

The r_s_f BTW stands for resync_sc_freq which is a function earlier in the file and should be familiar to a serious reader.

It was.

#9 lore raw
Quoted 17 lines · Peter Zijlstra
Quoted 8 lines · Andi Kleen
Quoted 7 lines · Peter Zijlstra
Quoted 5 lines · Satyam Sharma
Quoted 1 line · Ingo Molnar

call_r_s_f() still needs an urgent rerenaming though =B-)

So does "call_r_s_f_here()" :-)

Quoted 7 lines
Quoted 2 lines

That name makes me think of INTERCAL's 'DO COME FROM' statement. And any code that makes one think of INTERCAL is say,.. special.. :-)

Propose a better way to code this then? It's not my fault that dealing with callbacks in C is so messy. _here just massages one callback prototype (smp_call_function's) into another (cpufreq's) because both callbacks do the same in this case.

Quoted 4 lines

I see you point; however a function called:
call_<some_other_function>_here() just doesn't make sense. It says as much as: we should call some_other_function() but for some reason we dont.

It's just different semantics between cpufreq and smp_call_functions. cpufreq doesn't execute on that CPU but gives you the cpu number, smp_call_* executes on that CPU but doesn't give you a cpu number. _here means call cpufreq callback on the current CPU.

-Andi

#10 lore raw
In reply to Ingo Molnar
Quoted 6 lines · Ingo Molnar
Quoted 6 lines
Quoted 6 lines

if (!f)

	f = cpufreq_get(freq->cpu);
if (!f)
	f = tsc_khz;
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

?

Something's not quite right here :-)

ah, that's fine. It does this: 'try to give f a value', and then: 'if still no value then give it tsc_khz as a last resort)

call_r_s_f() still needs an urgent rerenaming though =B-)

The whole thing would be much simpler if

- cpu up callbacks had a way to request executing on the target CPU [i looked at this, but it was a little more involved than a simple cleanup]
- smp_call_function_single had sane semantics regarding calling the same CPU similar to on_each_cpu()
- C had anonymous lambda functions with nice syntax (ok one can dream)

But right now so many callbacks are needed that I ran out of good names.

-Andi

#11 lore raw
In reply to Satyam Sharma
Quoted 8 lines

if (!f)

	f = cpufreq_get(freq->cpu);
if (!f)
	f = tsc_khz;

?

Something's not quite right here :-)

What do you think is wrong? cpufreq_get can return 0.

Admittedly the second test could be inside a block of the first, but then it would make the code more ugly and this code is not performance critical.

-Andi

author
#12 lore raw
Quoted 7 lines · Andi Kleen
Quoted 7 lines

if (!f)

	f = cpufreq_get(freq->cpu);
if (!f)
	f = tsc_khz;

?

Quoted 7 lines
Quoted 1 line

Something's not quite right here :-)

What do you think is wrong? cpufreq_get can return 0.

Admittedly the second test could be inside a block of the first, but then it would make the code more ugly and this code is not performance critical.

moving it inside the block makes the code _cleaner_ in fact :-)

Ingo

author
#13 lore raw
Quoted 5 lines · Ingo Molnar
Quoted 3 lines

Admittedly the second test could be inside a block of the first, but then it would make the code more ugly and this code is not performance critical.

moving it inside the block makes the code _cleaner_ in fact :-)

updated version of the cleanup patch below, renamed r_s_c to resync_freq, and changed 'f' to 'freq'.

Ingo

---------------->
Subject: [patch] sched_clock(): cleanups
From: Ingo Molnar <[email protected]>

clean up sched-clock.c - mostly comment style fixes.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/sched-clock.c |  108 +++++++++++++++++++++++++----------------
 1 file changed, 68 insertions(+), 40 deletions(-)
arch/i386/kernel/sched-clock.c +68 -40
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/sched-clock.c
===================================================================
@@ -60,18 +60,20 @@ DEFINE_PER_CPU(struct sc_data, sc_data) 
  */
 unsigned long long native_sched_clock(void)
 {
-	unsigned long long r;
 	struct sc_data *sc = &get_cpu_var(sc_data);
+	unsigned long long r;
+	unsigned long flags;
 
 	if (sc->unstable) {
-		unsigned long flags;
 		r = (jiffies_64 - sc->sync_base) * (1000000000 / HZ);
 		r += sc->ns_base;
 		local_irq_save(flags);
-		/* last_val is used to avoid non monotonity on a
-		   stable->unstable transition. Make sure the time
-		   never goes to before the last value returned by
-		   the TSC clock */
+		/*
+		 * last_val is used to avoid non monotonity on a
+		 * stable->unstable transition. Make sure the time
+		 * never goes to before the last value returned by
+		 * the TSC clock
+		 */
 		if (r <= sc->last_val)
 			r = sc->last_val + 1;
 		sc->last_val = r;

@@ -87,8 +89,10 @@ unsigned long long native_sched_clock(vo
 	return r;
 }
 
-/* We need to define a real function for sched_clock, to override the
-   weak default version */
+/*
+ * We need to define a real function for sched_clock, to override the
+ * weak default version
+ */
 #ifdef CONFIG_PARAVIRT
 unsigned long long sched_clock(void)
 {

@@ -101,9 +105,11 @@ unsigned long long sched_clock(void)
 
 static int no_sc_for_printk;
 
-/* printk clock: when it is known the sc results are very non monotonic
-   fall back to jiffies for printk. Other sched_clock users are supposed
-   to handle this. */
+/*
+ * printk clock: when it is known the sc results are very non monotonic
+ * fall back to jiffies for printk. Other sched_clock users are supposed
+ * to handle this.
+ */
 unsigned long long printk_clock(void)
 {
 	if (unlikely(no_sc_for_printk))

@@ -119,35 +125,46 @@ static void resync_sc_freq(struct sc_dat
 		sc->unstable = 1;
 		return;
 	}
-	/* Handle nesting, but when we're zero multiple calls in a row
-	   are ok too and not a bug */
+	/*
+	 * Handle nesting, but when we're zero multiple calls in a row
+	 * are ok too and not a bug
+	 */
 	if (sc->unstable > 0)
 		sc->unstable--;
 	if (sc->unstable)
 		return;
-	/* RED-PEN protect with seqlock? I hope that's not needed
-	   because sched_clock callers should be able to tolerate small
-	   errors. */
+	/*
+	 * RED-PEN protect with seqlock? I hope that's not needed
+	 * because sched_clock callers should be able to tolerate small
+	 * errors.
+	 */
 	sc->ns_base = ktime_to_ns(ktime_get());
 	rdtscll(sc->sync_base);
 	sc->cyc2ns_scale = (1000000 << CYC2NS_SCALE_FACTOR) / newfreq;
 }
 
-static void call_r_s_f(void *arg)
+static void __resync_freq(void *arg)
 {
 	struct cpufreq_freqs *freq = arg;
-	unsigned f = freq->new;
-	if (!f)
-		f = cpufreq_get(freq->cpu);
-	if (!f)
-		f = tsc_khz;
-	resync_sc_freq(&per_cpu(sc_data, freq->cpu), f);
+	unsigned freq = freq->new;
+
+	if (!freq) {
+		freq = cpufreq_get(freq->cpu);
+		/*
+		 * Still no frequency? Then fall back to tsc_khz:
+		 */
+		if (!freq)
+			freq = tsc_khz;
+	}
+	resync_sc_freq(&per_cpu(sc_data, freq->cpu), freq);
 }
 
-static void call_r_s_f_here(void *arg)
+static void resync_freq(void *arg)
 {
-	struct cpufreq_freqs f = { .cpu = get_cpu(), .new = 0 };
-	call_r_s_f(&f);
+	struct cpufreq_freqs f = { .new = 0 };
+
+	f.cpu = get_cpu();
+	__resync_freq(&f);
 	put_cpu();
 }
 

@@ -155,9 +172,11 @@ static int sc_freq_event(struct notifier
 			 void *data)
 {
 	struct cpufreq_freqs *freq = data;
-	int cpu = get_cpu();
-	struct sc_data *sc = &per_cpu(sc_data, cpu);
+	struct sc_data *sc;
+	int cpu;
 
+	cpu = get_cpu();
+	sc = &per_cpu(sc_data, cpu);
 	if (cpu_has(&cpu_data[cpu], X86_FEATURE_CONSTANT_TSC))
 		goto out;
 	if (freq->old == freq->new)

@@ -167,25 +186,30 @@ static int sc_freq_event(struct notifier
 	case CPUFREQ_SUSPENDCHANGE:
 		/* Mark TSC unstable during suspend/resume */
 	case CPUFREQ_PRECHANGE:
-		/* Mark TSC as unstable until cpu frequency change is done
-		   because we don't know when exactly it will change.
-		   unstable in used as a counter to guard against races
-		   between the cpu frequency notifiers and normal resyncs */
+		/*
+		 * Mark TSC as unstable until cpu frequency change is done
+		 * because we don't know when exactly it will change.
+		 * unstable in used as a counter to guard against races
+		 * between the cpu frequency notifiers and normal resyncs
+		 */
 		sc->unstable++;
 		/* FALL THROUGH */
 	case CPUFREQ_RESUMECHANGE:
 	case CPUFREQ_POSTCHANGE:
-		/* Frequency change or resume is done -- update everything and
-		   mark TSC as stable again. */
+		/*
+		 * Frequency change or resume is done -- update everything
+		 * and mark TSC as stable again.
+		 */
 		if (cpu == freq->cpu)
 			resync_sc_freq(sc, freq->new);
 		else
-			smp_call_function_single(freq->cpu, call_r_s_f,
+			smp_call_function_single(freq->cpu, __resync_freq,
 						 freq, 0, 1);
 		break;
 	}
 out:
 	put_cpu();
+
 	return NOTIFY_DONE;
 }
 

@@ -197,9 +221,11 @@ static int __cpuinit
 sc_cpu_event(struct notifier_block *self, unsigned long event, void *hcpu)
 {
 	long cpu = (long)hcpu;
+
 	if (event == CPU_ONLINE) {
 		struct cpufreq_freqs f = { .cpu = cpu, .new = 0 };
-		smp_call_function_single(cpu, call_r_s_f, &f, 0, 1);
+
+		smp_call_function_single(cpu, __resync_freq, &f, 0, 1);
 	}
 	return NOTIFY_DONE;
 }

@@ -209,13 +235,15 @@ static __init int init_sched_clock(void)
 	if (unsynchronized_tsc())
 		no_sc_for_printk = 1;
 
-	/* On a race between the various events the initialization might be
-	   done multiple times, but code is tolerant to this */
+	/*
+	 * On a race between the various events the initialization might be
+	 * done multiple times, but code is tolerant to this
+	 */
 	cpufreq_register_notifier(&sc_freq_notifier,
 				CPUFREQ_TRANSITION_NOTIFIER);
 	hotcpu_notifier(sc_cpu_event, 0);
-	on_each_cpu(call_r_s_f_here, NULL, 0, 0);
+	on_each_cpu(resync_freq, NULL, 0, 0);
+
 	return 0;
 }
 core_initcall(init_sched_clock);
-
author
#14 lore raw
Quoted 2 lines · Ingo Molnar

updated version of the cleanup patch below, renamed r_s_c to resync_freq, and changed 'f' to 'freq'.

updated one below.

Ingo

------------------->
Subject: [patch] sched_clock(): cleanups
From: Ingo Molnar <[email protected]>

clean up sched-clock.c - mostly comment style fixes.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/sched-clock.c |  110 +++++++++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 41 deletions(-)
arch/i386/kernel/sched-clock.c +69 -41
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/sched-clock.c
===================================================================
@@ -60,18 +60,20 @@ DEFINE_PER_CPU(struct sc_data, sc_data) 
  */
 unsigned long long native_sched_clock(void)
 {
-	unsigned long long r;
 	struct sc_data *sc = &get_cpu_var(sc_data);
+	unsigned long long r;
+	unsigned long flags;
 
 	if (sc->unstable) {
-		unsigned long flags;
 		r = (jiffies_64 - sc->sync_base) * (1000000000 / HZ);
 		r += sc->ns_base;
 		local_irq_save(flags);
-		/* last_val is used to avoid non monotonity on a
-		   stable->unstable transition. Make sure the time
-		   never goes to before the last value returned by
-		   the TSC clock */
+		/*
+		 * last_val is used to avoid non monotonity on a
+		 * stable->unstable transition. Make sure the time
+		 * never goes to before the last value returned by
+		 * the TSC clock
+		 */
 		if (r <= sc->last_val)
 			r = sc->last_val + 1;
 		sc->last_val = r;

@@ -87,8 +89,10 @@ unsigned long long native_sched_clock(vo
 	return r;
 }
 
-/* We need to define a real function for sched_clock, to override the
-   weak default version */
+/*
+ * We need to define a real function for sched_clock, to override the
+ * weak default version
+ */
 #ifdef CONFIG_PARAVIRT
 unsigned long long sched_clock(void)
 {

@@ -101,9 +105,11 @@ unsigned long long sched_clock(void)
 
 static int no_sc_for_printk;
 
-/* printk clock: when it is known the sc results are very non monotonic
-   fall back to jiffies for printk. Other sched_clock users are supposed
-   to handle this. */
+/*
+ * printk clock: when it is known the sc results are very non monotonic
+ * fall back to jiffies for printk. Other sched_clock users are supposed
+ * to handle this.
+ */
 unsigned long long printk_clock(void)
 {
 	if (unlikely(no_sc_for_printk))

@@ -119,35 +125,46 @@ static void resync_sc_freq(struct sc_dat
 		sc->unstable = 1;
 		return;
 	}
-	/* Handle nesting, but when we're zero multiple calls in a row
-	   are ok too and not a bug */
+	/*
+	 * Handle nesting, but when we're zero multiple calls in a row
+	 * are ok too and not a bug
+	 */
 	if (sc->unstable > 0)
 		sc->unstable--;
 	if (sc->unstable)
 		return;
-	/* RED-PEN protect with seqlock? I hope that's not needed
-	   because sched_clock callers should be able to tolerate small
-	   errors. */
+	/*
+	 * RED-PEN protect with seqlock? I hope that's not needed
+	 * because sched_clock callers should be able to tolerate small
+	 * errors.
+	 */
 	sc->ns_base = ktime_to_ns(ktime_get());
 	rdtscll(sc->sync_base);
 	sc->cyc2ns_scale = (1000000 << CYC2NS_SCALE_FACTOR) / newfreq;
 }
 
-static void call_r_s_f(void *arg)
+static void __resync_freq(void *arg)
 {
-	struct cpufreq_freqs *freq = arg;
-	unsigned f = freq->new;
-	if (!f)
-		f = cpufreq_get(freq->cpu);
-	if (!f)
-		f = tsc_khz;
-	resync_sc_freq(&per_cpu(sc_data, freq->cpu), f);
+	struct cpufreq_freqs *freqp = arg;
+	unsigned freq = freqp->new;
+
+	if (!freq) {
+		freq = cpufreq_get(freqp->cpu);
+		/*
+		 * Still no frequency? Then fall back to tsc_khz:
+		 */
+		if (!freq)
+			freq = tsc_khz;
+	}
+	resync_sc_freq(&per_cpu(sc_data, freqp->cpu), freq);
 }
 
-static void call_r_s_f_here(void *arg)
+static void resync_freq(void *arg)
 {
-	struct cpufreq_freqs f = { .cpu = get_cpu(), .new = 0 };
-	call_r_s_f(&f);
+	struct cpufreq_freqs f = { .new = 0 };
+
+	f.cpu = get_cpu();
+	__resync_freq(&f);
 	put_cpu();
 }
 

@@ -155,9 +172,11 @@ static int sc_freq_event(struct notifier
 			 void *data)
 {
 	struct cpufreq_freqs *freq = data;
-	int cpu = get_cpu();
-	struct sc_data *sc = &per_cpu(sc_data, cpu);
+	struct sc_data *sc;
+	int cpu;
 
+	cpu = get_cpu();
+	sc = &per_cpu(sc_data, cpu);
 	if (cpu_has(&cpu_data[cpu], X86_FEATURE_CONSTANT_TSC))
 		goto out;
 	if (freq->old == freq->new)

@@ -167,25 +186,30 @@ static int sc_freq_event(struct notifier
 	case CPUFREQ_SUSPENDCHANGE:
 		/* Mark TSC unstable during suspend/resume */
 	case CPUFREQ_PRECHANGE:
-		/* Mark TSC as unstable until cpu frequency change is done
-		   because we don't know when exactly it will change.
-		   unstable in used as a counter to guard against races
-		   between the cpu frequency notifiers and normal resyncs */
+		/*
+		 * Mark TSC as unstable until cpu frequency change is done
+		 * because we don't know when exactly it will change.
+		 * unstable in used as a counter to guard against races
+		 * between the cpu frequency notifiers and normal resyncs
+		 */
 		sc->unstable++;
 		/* FALL THROUGH */
 	case CPUFREQ_RESUMECHANGE:
 	case CPUFREQ_POSTCHANGE:
-		/* Frequency change or resume is done -- update everything and
-		   mark TSC as stable again. */
+		/*
+		 * Frequency change or resume is done -- update everything
+		 * and mark TSC as stable again.
+		 */
 		if (cpu == freq->cpu)
 			resync_sc_freq(sc, freq->new);
 		else
-			smp_call_function_single(freq->cpu, call_r_s_f,
+			smp_call_function_single(freq->cpu, __resync_freq,
 						 freq, 0, 1);
 		break;
 	}
 out:
 	put_cpu();
+
 	return NOTIFY_DONE;
 }
 

@@ -197,9 +221,11 @@ static int __cpuinit
 sc_cpu_event(struct notifier_block *self, unsigned long event, void *hcpu)
 {
 	long cpu = (long)hcpu;
+
 	if (event == CPU_ONLINE) {
 		struct cpufreq_freqs f = { .cpu = cpu, .new = 0 };
-		smp_call_function_single(cpu, call_r_s_f, &f, 0, 1);
+
+		smp_call_function_single(cpu, __resync_freq, &f, 0, 1);
 	}
 	return NOTIFY_DONE;
 }

@@ -209,13 +235,15 @@ static __init int init_sched_clock(void)
 	if (unsynchronized_tsc())
 		no_sc_for_printk = 1;
 
-	/* On a race between the various events the initialization might be
-	   done multiple times, but code is tolerant to this */
+	/*
+	 * On a race between the various events the initialization might be
+	 * done multiple times, but code is tolerant to this
+	 */
 	cpufreq_register_notifier(&sc_freq_notifier,
 				CPUFREQ_TRANSITION_NOTIFIER);
 	hotcpu_notifier(sc_cpu_event, 0);
-	on_each_cpu(call_r_s_f_here, NULL, 0, 0);
+	on_each_cpu(resync_freq, NULL, 0, 0);
+
 	return 0;
 }
 core_initcall(init_sched_clock);
-
author
#15 lore raw

PATCH 1/1 x86_64: fix sched_clock()

Subject: [patch] x86_64: fix sched_clock()
From: Ingo Molnar <[email protected]>

sched_clock() is totally broken on x86_64, because it is not defined by the architecture at all! It fell the victim to the opaqueness of __attribute__((weak)) and to non-testing.

the i386 version was supposed to be used. This patch fixes that. Booted and tested on x86_64 and i386.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/x86_64/kernel/Makefile |    3 ++-
 include/asm-x86_64/tsc.h    |    5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)
arch/x86_64/kernel/Makefile +2 -1
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/x86_64/kernel/Makefile
===================================================================
@@ -9,7 +9,7 @@ obj-y	:= process.o signal.o entry.o trap
 		x8664_ksyms.o i387.o syscall.o vsyscall.o \
 		setup64.o bootflag.o e820.o reboot.o quirks.o i8237.o \
 		pci-dma.o pci-nommu.o alternative.o hpet.o tsc.o bugs.o \
-		perfctr-watchdog.o
+		perfctr-watchdog.o sched-clock.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_X86_MCE)		+= mce.o therm_throt.o

@@ -49,6 +49,7 @@ obj-y				+= pcspeaker.o
 
 CFLAGS_vsyscall.o		:= $(PROFILING) -g0
 
+sched-clock-y			+= ../../i386/kernel/sched-clock.o
 therm_throt-y                   += ../../i386/kernel/cpu/mcheck/therm_throt.o
 bootflag-y			+= ../../i386/kernel/bootflag.o
 legacy_serial-y			+= ../../i386/kernel/legacy_serial.o
include/asm-x86_64/tsc.h +5 -0
Index: linux-cfs-2.6.22-rc2-mm1.q/include/asm-x86_64/tsc.h
===================================================================
@@ -1 +1,6 @@
 #include <asm-i386/tsc.h>
+
+/*
+ * To be able to share sched-clock.c between i386 and x86_64:
+ */
+#define tsc_disable 0
#16 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 6 lines · Ingo Molnar

Subject: [patch] x86_64: fix sched_clock()
From: Ingo Molnar <[email protected]>

sched_clock() is totally broken on x86_64, because it is not defined by the architecture at all! It fell the victim to the opaqueness of __attribute__((weak)) and to non-testing.

More non retesting.

Quoted 3 lines

the i386 version was supposed to be used. This patch fixes that. Booted and tested on x86_64 and i386.

Hmm indeed. I actually had it correct at some point (i remember fixing 64bit compile errors in sched-clock ;-). I guess the Makefile hunk accidentially dropped out during some later merging and this didn't get noticed due to the weak attribute.

That also explains the issue reported by Peter earlier I guess.

-Andi

author
#17 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 7 lines · Andi Kleen
Quoted 2 lines

the i386 version was supposed to be used. This patch fixes that. Booted and tested on x86_64 and i386.

Hmm indeed. I actually had it correct at some point (i remember fixing 64bit compile errors in sched-clock ;-). I guess the Makefile hunk accidentially dropped out during some later merging and this didn't get noticed due to the weak attribute.

well the tsc.h bit was needed too.

Quoted 1 line

That also explains the issue reported by Peter earlier I guess.

yes, it does, at least on my box.

Ingo

#18 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 6 lines · Ingo Molnar
Quoted 3 lines · Andi Kleen
Quoted 2 lines

the i386 version was supposed to be used. This patch fixes that. Booted and tested on x86_64 and i386.

Quoted 6 lines
Quoted 4 lines

Hmm indeed. I actually had it correct at some point (i remember fixing 64bit compile errors in sched-clock ;-). I guess the Makefile hunk accidentially dropped out during some later merging and this didn't get noticed due to the weak attribute.

well the tsc.h bit was needed too.

It's not in defconfig at least. I just tried it.

-Andi

author
#19 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 8 lines · Andi Kleen
Quoted 6 lines
Quoted 4 lines

Hmm indeed. I actually had it correct at some point (i remember fixing 64bit compile errors in sched-clock ;-). I guess the Makefile hunk accidentially dropped out during some later merging and this didn't get noticed due to the weak attribute.

well the tsc.h bit was needed too.

It's not in defconfig at least. I just tried it.

what do you mean? The tsc.h bit is needed because arch/i386/kernel/sched-clock.c (now built on x86_64 too with the patch i sent) uses the tsc_disable global flag which is non-existent on x86_64. So my tsc.h change adds that global flag, always-defined to 0.

Ingo

#20 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andi Kleen
Quoted 5 lines
Quoted 4 lines

Hmm indeed. I actually had it correct at some point (i remember fixing 64bit compile errors in sched-clock ;-). I guess the Makefile hunk accidentially dropped out during some later merging and this didn't get noticed due to the weak attribute.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

well the tsc.h bit was needed too.

It's not in defconfig at least. I just tried it.

what do you mean? The tsc.h bit is needed because

Means i readded the Makefile hunk and it compiled for me in 64bit without changing anything else.

Quoted 3 lines

arch/i386/kernel/sched-clock.c (now built on x86_64 too with the patch i sent) uses the tsc_disable global flag which is non-existent on x86_64. So my tsc.h change adds that global flag, always-defined to 0.

My version of sched_clock.c doesn't have any reference to tsc_disable.

-andi

author
#21 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 6 lines · Andi Kleen
Quoted 4 lines

arch/i386/kernel/sched-clock.c (now built on x86_64 too with the patch i sent) uses the tsc_disable global flag which is non-existent on x86_64. So my tsc.h change adds that global flag, always-defined to 0.

My version of sched_clock.c doesn't have any reference to tsc_disable.

must be an -mm fix. I used -mm as a basis of my work. Please apply my patch.

Ingo

#22 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andi Kleen
Quoted 4 lines

arch/i386/kernel/sched-clock.c (now built on x86_64 too with the patch i sent) uses the tsc_disable global flag which is non-existent on x86_64. So my tsc.h change adds that global flag, always-defined to 0.

Quoted 4 lines
Quoted 1 line

My version of sched_clock.c doesn't have any reference to tsc_disable.

must be an -mm fix. I used -mm as a basis of my work. Please apply my patch.

I would prefer to find out why the mm patch was added and then hopefully remove it. IMNSHO it should not be needed.

-Andi

author
#23 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 5 lines · Andi Kleen
Quoted 2 lines

must be an -mm fix. I used -mm as a basis of my work. Please apply my patch.

I would prefer to find out why the mm patch was added and then hopefully remove it. IMNSHO it should not be needed.

it comes in via:

fix-x86_64-mm-sched-clock-share.patch

From: Rusty Russell <[email protected]>

If you set tsc_disable (eg "notsc" on cmdline), sched-clock.c gives a divide by zero on boot.

so IMNSHO it is very much needed ;-)

Ingo

#24 lore raw

Re: [patch] x86_64: fix sched_clock()

In reply to Andi Kleen
Quoted 9 lines · On Fri, 25 May 2007 10:45:47 +0200 Andi Kleen <[email protected]> wrote:
Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andi Kleen
Quoted 4 lines

arch/i386/kernel/sched-clock.c (now built on x86_64 too with the patch i sent) uses the tsc_disable global flag which is non-existent on x86_64. So my tsc.h change adds that global flag, always-defined to 0.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

My version of sched_clock.c doesn't have any reference to tsc_disable.

must be an -mm fix. I used -mm as a basis of my work. Please apply my patch.

I would prefer to find out why the mm patch was added and then hopefully remove it. IMNSHO it should not be needed.

This? I sent it to you earlier this week:

From: Rusty Russell <[email protected]>

If you set tsc_disable (eg "notsc" on cmdline), sched-clock.c gives a divide by zero on boot.

Signed-off-by Rusty Russell <[email protected]>
Cc Andi Kleen <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 arch/i386/kernel/sched-clock.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)
arch/i386/kernel/sched-clock.c renamed from arch/i386/kernel/sched-clock.c~fix-x86_64-mm-sched-clock-share +1 -1
diff -puN arch/i386/kernel/sched-clock.c~fix-x86_64-mm-sched-clock-share arch/i386/kernel/sched-clock.c
@@ -115,7 +115,7 @@ unsigned long long printk_clock(void)
 static void resync_sc_freq(struct sc_data *sc, unsigned int newfreq)
 {
 	sc->sync_base = jiffies;
-	if (!cpu_has_tsc) {
+	if (!cpu_has_tsc || tsc_disable) {
 		sc->unstable = 1;
 		return;
 	}

_

#25 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 1 line

This? I sent it to you earlier this week:

Sorry haven't processed those yet.

Ah. The correct fix here is to clear the tsc flag in boot_cpu_data when the option is set. Will do that.

-Andi

Quoted 28 lines

From: Rusty Russell <[email protected]>

If you set tsc_disable (eg "notsc" on cmdline), sched-clock.c gives a divide by zero on boot.

Signed-off-by Rusty Russell <[email protected]>
Cc Andi Kleen <[email protected]>
Signed-off-by Andrew Morton <[email protected]>

---

 arch/i386/kernel/sched-clock.c |    2 +-
 1 files changed, 1 insertion(+), 1 deletion(-)

diff -puN arch/i386/kernel/sched-clock.c~fix-x86_64-mm-sched-clock-share arch/i386/kernel/sched-clock.c

arch/i386/kernel/sched-clock.c renamed from arch/i386/kernel/sched-clock.c~fix-x86_64-mm-sched-clock-share +1 -1
@@ -115,7 +115,7 @@ unsigned long long printk_clock(void)
 static void resync_sc_freq(struct sc_data *sc, unsigned int newfreq)
 {
 	sc->sync_base = jiffies;
-	if (!cpu_has_tsc) {
+	if (!cpu_has_tsc || tsc_disable) {
 		sc->unstable = 1;
 		return;
 	}

_

author
#26 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 6 lines · Andi Kleen
Quoted 1 line

This? I sent it to you earlier this week:

Sorry haven't processed those yet.

Ah. The correct fix here is to clear the tsc flag in boot_cpu_data when the option is set. Will do that.

please indicate that you've picked up my style cleanups, i dont want to redo all this a few days/weeks down the line ...

Ingo

#27 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 5 lines · Ingo Molnar
Quoted 2 lines · Andi Kleen
Quoted 1 line

This? I sent it to you earlier this week:

Quoted 7 lines
Quoted 4 lines

Sorry haven't processed those yet.

Ah. The correct fix here is to clear the tsc flag in boot_cpu_data when the option is set. Will do that.

please indicate that you've picked up my style cleanups, i dont want to redo all this a few days/weeks down the line ...

It's done slightly differently now due to conflicting earlier changes, but the end result should be about what you intended. You're also still credited in the cleanup patch of course.

-Andi

author
#28 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 5 lines · Andi Kleen
Quoted 2 lines

please indicate that you've picked up my style cleanups, i dont want to redo all this a few days/weeks down the line ...

It's done slightly differently now due to conflicting earlier changes, but the end result should be about what you intended. [...]

please send me your current sched-clock.c, i'll redo any remaining cleanups.

But ... i find your approach curious, why didnt you just apply the cleanups i sent? You clearly started working on this as a reaction to my cleanup patches and to the bugfixes i sent ontop of the cleanup patches. Your "I'll do this differently" approach is totally unnecessary from a commit management point of view (this is new code after all and will/should go upstream in a single clean chunk anyway), the only effect this has is that that you are discouraging contributors like me from contributing cleanups to the x86_64 tree.

To further underline this feeling i got, none of your reactions to any of my patches showed even a single positive thought that you are happy about people fixing code you are introducing (in fact you didnt even indicate which patch you took, only at my repeated prodding did you say anything about the cleanup patch i sent!), so to me the impression is that deep in yourself you are (subconsciously) not happy about others contributing to the x86_64 tree. Please tell me that i'm wrong :-(

Ingo

#29 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 11 lines · Ingo Molnar
Quoted 5 lines · Andi Kleen
Quoted 2 lines

please indicate that you've picked up my style cleanups, i dont want to redo all this a few days/weeks down the line ...

It's done slightly differently now due to conflicting earlier changes, but the end result should be about what you intended. [...]

please send me your current sched-clock.c, i'll redo any remaining cleanups.

It needs at least one new preliminary patch (to add on_cpu_single); please get the series from ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches-2.6.22-rc2-git7-070525-1.tar.gz You need at least tsc-unstable upto paravirt-add-a-sched_clock-paravirt for everything

Quoted 8 lines

But ... i find your approach curious, why didnt you just apply the cleanups i sent? You clearly started working on this as a reaction to my cleanup patches and to the bugfixes i sent ontop of the cleanup patches. Your "I'll do this differently" approach is totally unnecessary from a commit management point of view (this is new code after all and will/should go upstream in a single clean chunk anyway), the only effect this has is that that you are discouraging contributors like me from contributing cleanups to the x86_64 tree.

Unfortunately right now it is a already a set of patches; e.g. due to the paravirt ops change. I can merge back the cleanup change; but kept it separately due to your earlier complaint about not using your patch. I think one hunk of your original one is still in there :-)

Quoted 3 lines

so to me the impression is
that deep in yourself you are (subconsciously) not happy about others contributing to the x86_64 tree. Please tell me that i'm wrong :-(

You're reading too much into that. Of course I value contributions to x86-64, including cleanups. In general when I don't like it I complain so saying nothing is approval (or me being not reading email, but that doesn't happen that often)

-Andi

author
#30 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 8 lines · Andi Kleen
Quoted 2 lines

please send me your current sched-clock.c, i'll redo any remaining cleanups.

It needs at least one new preliminary patch (to add on_cpu_single); please get the series from ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches-2.6.22-rc2-git7-070525-1.tar.gz You need at least tsc-unstable upto paravirt-add-a-sched_clock-paravirt for everything

thanks.

you missed one patch: please pick up the NUMAQ change i did too. (i kept the printk to make sure someone notices that and actually tests thing - i dont have a NUMAQ machine to try this on.)

i'm looking at the other things now.

Ingo

author
#31 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 5 lines · Ingo Molnar
Quoted 3 lines · Andi Kleen
Quoted 2 lines

please send me your current sched-clock.c, i'll redo any remaining cleanups.

Quoted 7 lines
Quoted 5 lines

It needs at least one new preliminary patch (to add on_cpu_single); please get the series from ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches-2.6.22-rc2-git7-070525-1.tar.gz You need at least tsc-unstable upto paravirt-add-a-sched_clock-paravirt for everything

i'm looking at the other things now.

your queue does not apply cleanly here:

Applying patch patches/paravirt-add-a-sched_clock-paravirt_op

patching file arch/i386/kernel/sched-clock.c
Hunk #5 FAILED at 148.
1 out of 5 hunks FAILED -- rejects in file arch/i386/kernel/sched-clock.c

could you please just send me your current sched-clock.c? (that's what i'm interested in) I'll deal with the on_cpu_single() dependency. Thanks,

Ingo

author
#32 lore raw

PATCH 1/1 sched_clock: fix preempt count imbalance

In reply to Ingo Molnar
Quoted 9 lines · Ingo Molnar
Quoted 8 lines
Quoted 2 lines

please send me your current sched-clock.c, i'll redo any remaining cleanups.

It needs at least one new preliminary patch (to add on_cpu_single); please get the series from ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches-2.6.22-rc2-git7-070525-1.tar.gz You need at least tsc-unstable upto paravirt-add-a-sched_clock-paravirt for everything

Quoted 7 lines

thanks.

you missed one patch: please pick up the NUMAQ change i did too. (i kept the printk to make sure someone notices that and actually tests thing - i dont have a NUMAQ machine to try this on.)

i'm looking at the other things now.

you introduced a crash-bug via your cleanups - the patch below fixes it.

Ingo

----------------------------->
Subject: [patch] sched_clock: fix preempt count imbalance
From: Ingo Molnar <[email protected]>

fix preempt count imbalance.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/sched-clock.c |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)
arch/i386/kernel/sched-clock.c +6 -2
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/sched-clock.c
===================================================================
@@ -164,7 +164,10 @@ static int sc_freq_event(struct notifier
 			 void *data)
 {
 	struct cpufreq_freqs *freq = data;
-	struct sc_data *sc = &per_cpu(sc_data, freq->cpu);
+	struct sc_data *sc;
+
+	preempt_disable();
+	sc = &per_cpu(sc_data, freq->cpu);
 
 	if (cpu_has(&cpu_data[freq->cpu], X86_FEATURE_CONSTANT_TSC))
 		goto out;

@@ -194,7 +197,8 @@ static int sc_freq_event(struct notifier
 		break;
 	}
 out:
-	put_cpu();
+	preempt_enable();
+
 	return NOTIFY_DONE;
 }
author
#33 lore raw

PATCH 1/1 sched_clock(): cleanups, #2

In reply to Ingo Molnar
Quoted 11 lines · Ingo Molnar
Quoted 8 lines · Andi Kleen
Quoted 2 lines

please send me your current sched-clock.c, i'll redo any remaining cleanups.

It needs at least one new preliminary patch (to add on_cpu_single); please get the series from ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches-2.6.22-rc2-git7-070525-1.tar.gz You need at least tsc-unstable upto paravirt-add-a-sched_clock-paravirt for everything

Quoted 7 lines

thanks.

you missed one patch: please pick up the NUMAQ change i did too. (i kept the printk to make sure someone notices that and actually tests thing - i dont have a NUMAQ machine to try this on.)

i'm looking at the other things now.

find below the cleanups from my first patch that didnt make it into your cleanups. (plus one more cleanup i noticed while merging the missing bits from my first patch) Goes after the bugfix i just sent. Please apply.

Ingo

---------------------->
Subject: [patch] sched_clock(): cleanups, #2
From: Ingo Molnar <[email protected]>

clean up sched-clock.c - the missing bits.

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/sched-clock.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)
arch/i386/kernel/sched-clock.c +14 -6
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/sched-clock.c
===================================================================
@@ -89,8 +89,10 @@ unsigned long long native_sched_clock(vo
 	return r;
 }
 
-/* We need to define a real function for sched_clock, to override the
-   weak default version */
+/*
+ * We need to define a real function for sched_clock, to override the
+ * weak default version
+ */
 #ifdef CONFIG_PARAVIRT
 unsigned long long sched_clock(void)
 {

@@ -119,6 +121,9 @@ static void resolve_freq(struct cpufreq_
 {
 	if (!freq->new) {
 		freq->new = cpufreq_get(freq->cpu);
+		/*
+		 * Still no frequency? Then fall back to tsc_khz:
+		 */
 		if (!freq->new)
 			freq->new = tsc_khz;
 	}

@@ -129,6 +134,7 @@ static void resync_freq(void *arg)
 {
 	struct cpufreq_freqs *freq = (void *)arg;
 	struct sc_data *sc = &__get_cpu_var(sc_data);
+
 	sc->sync_base = jiffies;
 	if (!cpu_has_tsc) {
 		sc->unstable = 1;

@@ -155,13 +161,14 @@ static void resync_freq(void *arg)
 static void resync_freq_on_cpu(void *arg)
 {
 	struct cpufreq_freqs f = { .new = 0 };
+
 	f.cpu = get_cpu();
 	resync_freq(&f);
 	put_cpu();
 }
 
-static int sc_freq_event(struct notifier_block *nb, unsigned long event,
-			 void *data)
+static int
+sc_freq_event(struct notifier_block *nb, unsigned long event, void *data)
 {
 	struct cpufreq_freqs *freq = data;
 	struct sc_data *sc;

@@ -210,8 +217,10 @@ static int __cpuinit
 sc_cpu_event(struct notifier_block *self, unsigned long event, void *hcpu)
 {
 	long cpu = (long)hcpu;
+
 	if (event == CPU_ONLINE) {
 		struct cpufreq_freqs f = { .cpu = cpu, .new = 0 };
+
 		on_cpu_single(cpu, resync_freq, &f);
 	}
 	return NOTIFY_DONE;

@@ -221,7 +230,6 @@ static __init int init_sched_clock(void)
 {
 	if (unsynchronized_tsc())
 		no_sc_for_printk = 1;
-
 	/*
 	 * On a race between the various events the initialization
 	 * might be done multiple times, but code is tolerant to

@@ -231,7 +239,7 @@ static __init int init_sched_clock(void)
 				CPUFREQ_TRANSITION_NOTIFIER);
 	hotcpu_notifier(sc_cpu_event, 0);
 	on_each_cpu(resync_freq_on_cpu, NULL, 0, 0);
+
 	return 0;
 }
 core_initcall(init_sched_clock);
-
#34 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 20 lines · Ingo Molnar
Quoted 17 lines · Ingo Molnar
Quoted 8 lines · Andi Kleen
Quoted 2 lines

please send me your current sched-clock.c, i'll redo any remaining cleanups.

It needs at least one new preliminary patch (to add on_cpu_single); please get the series from ftp://ftp.firstfloor.org/pub/ak/x86_64/quilt/patches-2.6.22-rc2-git7-070525-1.tar.gz You need at least tsc-unstable upto paravirt-add-a-sched_clock-paravirt for everything

thanks.

you missed one patch: please pick up the NUMAQ change i did too. (i kept the printk to make sure someone notices that and actually tests thing - i dont have a NUMAQ machine to try this on.)

Quoted 6 lines
Quoted 1 line

i'm looking at the other things now.

find below the cleanups from my first patch that didnt make it into your cleanups. (plus one more cleanup i noticed while merging the missing bits from my first patch) Goes after the bugfix i just sent. Please apply.

I cannot apply it as is because it changes code from the paravirtops patch too.

BTW to nitpick the original formattings you changed are all probably standard code style and the new comment is a classical i++; /* Increase i */

I'll fold the comment change into Jeremy's paravirt ops patch.

-Andi

author
#35 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 7 lines · Andi Kleen
Quoted 4 lines

find below the cleanups from my first patch that didnt make it into your cleanups. (plus one more cleanup i noticed while merging the missing bits from my first patch) Goes after the bugfix i just sent. Please apply.

I cannot apply it as is because it changes code from the paravirtops patch too.

ok - then please merge that single hunk into the paravirtops patch - and leave the other 6 hunks in this patch.

Quoted 3 lines

BTW to nitpick the original formattings you changed are all probably standard code style and the new comment is a classical i++; /* Increase i */

could you please quote the portion from my patch that you are talking about?

Ingo

#36 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 8 lines · Ingo Molnar
Quoted 5 lines · Andi Kleen
Quoted 4 lines

find below the cleanups from my first patch that didnt make it into your cleanups. (plus one more cleanup i noticed while merging the missing bits from my first patch) Goes after the bugfix i just sent. Please apply.

Quoted 5 lines
Quoted 2 lines

I cannot apply it as is because it changes code from the paravirtops patch too.

ok - then please merge that single hunk into the paravirtops patch - and leave the other 6 hunks in this patch.

I added the empty lines too.

Quoted 7 lines
Quoted 4 lines

BTW to nitpick the original formattings you changed are all probably standard code style and the new comment is a classical i++; /* Increase i */

could you please quote the portion from my patch that you are talking about?

-static int sc_freq_event(struct notifier_block *nb, unsigned long event,
- void *data)
+static int
+sc_freq_event(struct notifier_block *nb, unsigned long event, void *data)

and

+ /*
+ * Still no frequency? Then fall back to tsc_khz:
+ */

                if (!freq->new)
                        freq->new = tsc_khz;

-Andi (who hopes this thread will end soon now and we can all go back to more important issues)

#37 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 2 lines · On Fri, 25 May 2007 14:15:40 +0200 Andi Kleen <[email protected]> wrote:

-Andi (who hopes this thread will end soon now and we can all go back to more important issues)

fyi, the thread has been damn useful for me. If Ingo hadn't spotted that preempt_count() imbalance then there's a decent chance that I'd have been the first to hit it.

Time to bisect 1,000 patches: maybe an hour, if I choose the x86 tree as the first pivot point, which is likely.

Or I wouldn't have hit it, in which case a tester hits it, and someone (guess who) gets to enter into an intercontinental head-scratching session trying to work out who broke it this time, consuming the tester's time too. Plus we have a wrecked -mm and other people's code doesn't get as well-tested as it might.

All for one silly little mistake.

So. More care, please.

#38 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 12 lines · Andrew Morton
Quoted 2 lines · On Fri, 25 May 2007 14:15:40 +0200 Andi Kleen <[email protected]> wrote:

-Andi (who hopes this thread will end soon now and we can all go back to more important issues)

fyi, the thread has been damn useful for me. If Ingo hadn't spotted that preempt_count() imbalance then there's a decent chance that I'd have been the first to hit it.

Time to bisect 1,000 patches: maybe an hour, if I choose the x86 tree as the first pivot point, which is likely.

Quoted 7 lines

Or I wouldn't have hit it, in which case a tester hits it, and someone (guess who) gets to enter into an intercontinental head-scratching session trying to work out who broke it this time, consuming the tester's time too. Plus we have a wrecked -mm and other people's code doesn't get as well-tested as it might.

All for one silly little mistake.

I wonder if this was the source of the lockdep selftest failures, or the mystery hang this patch caused (IIRC)..

Daniel

#39 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 2 lines

I wonder if this was the source of the lockdep selftest failures, or the mystery hang this patch caused (IIRC)..

No, the inbalance was just on the ftp server for an hour or so. I doubt anybody except Ingo ever saw that code.

I introduced it while changing the callbacks around to make the code easier to read.

-Andi

#40 lore raw

Re: [patch] sched_clock(): cleanups, #2

In reply to Ingo Molnar
Quoted 3 lines · Ingo Molnar

ok - then please merge that single hunk into the paravirtops patch - and leave the other 6 hunks in this patch.

Note: I'm actually much more interested in applying the scheduler changes than the paravirt-ops changes. It looks like CFS is getting stable, I'd much rather have Ingo working on making a nice CFS patch on top of 2.6.22 (and the current -git tree should obviously approximate that, I don't expect any real changes to scheduler stuff), and merge that immediately after 2.6.22 is out.

Ingo: to make things easier, the _best_ split-up would be not "this is the historical series of patches leading up to CFS v15" or something like that, but it would be nice to get that final CFS version as a series of patches that do some specific things rather than as one final one. Is there any possibility that could happen?

Linus

#41 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 9 lines · Linus Torvalds
Quoted 3 lines · Ingo Molnar

ok - then please merge that single hunk into the paravirtops patch - and leave the other 6 hunks in this patch.

Note: I'm actually much more interested in applying the scheduler changes than the paravirt-ops changes. It looks like CFS is getting stable, I'd

That's ok -- that is why I'm keeping this stuff separate. paravirt ops sched_clock is just on top of the new sched_clock() but sched_clock doesn't rely on it.

Also my understanding is that CFS works even without the new sched_clock, just the new one makes it work better.

-Andi

author
#42 lore raw

Re: [patch] sched_clock(): cleanups, #2

In reply to Linus Torvalds
Quoted 5 lines · Linus Torvalds

Ingo: to make things easier, the _best_ split-up would be not "this is the historical series of patches leading up to CFS v15" or something like that, but it would be nice to get that final CFS version as a series of patches that do some specific things rather than as one final one. Is there any possibility that could happen?

sure enough, i'll work something sane out - i already have it partly split up. It will probably be something along the lines of: 'remove stuff that we can remove and still have functional scheduling' followed by an 'add minimal CFS patch' and then nicely split up 'CFS related add-ons' (like the task-stats accounting things, debugging, etc). Do you think i should try to split up the core CFS bits some more beyond this level? (that would be quite nontrivial i suspect)

Ingo

#43 lore raw

Re: [patch] sched_clock(): cleanups, #2

Quoted 6 lines · Ingo Molnar

sure enough, i'll work something sane out - i already have it partly split up. It will probably be something along the lines of: 'remove stuff that we can remove and still have functional scheduling' followed by an 'add minimal CFS patch' and then nicely split up 'CFS related add-ons' (like the task-stats accounting things, debugging, etc).

Ok, that sounds good. Some of that is obviously already in separate files already, even if they then just get #include'd into the main sched.c, so yeah, splitting it up along those lines makes sense.

Quoted 2 lines

Do you think i should try to split up the core CFS bits some more beyond this level? (that would be quite nontrivial i suspect)

I'd love to see some of that too, but I suspect that with something like the scheduler, it's hard to have any reasonable (and working!) points that make much sense. At some point, the changes to make it work incrementally get bigger than the patches themselves, and rather than show what's going on, it might _hide_ what's going on.

Linus

author
#44 lore raw

Re: [patch] x86_64: fix sched_clock()

In reply to Andi Kleen
Quoted 3 lines · Andi Kleen

It's done slightly differently now due to conflicting earlier changes, but the end result should be about what you intended. You're also still credited in the cleanup patch of course.

you totally misunderstood me. My problem isnt credit. I've got enough credit for a lifetime ;-) I'd be equally upset if this happened with someone else's patches (in fact i'd be _more_ upset about it, because then i'd also be worried about us losing a contributor).

My problem is that (and this might just be an issue of communication) you often appear as treating the x86_64 code as 'your code' instead of treating it as 'our code'. Dropping part of my patch, not even telling whether you took the patches or not, not reacting to patches in a positive way are all external signs of that.

Ingo

#45 lore raw

Re: [patch] x86_64: fix sched_clock()

In reply to Andi Kleen
Quoted 6 lines · Andi Kleen
Quoted 1 line

This? I sent it to you earlier this week:

Sorry haven't processed those yet.

Ah. The correct fix here is to clear the tsc flag in boot_cpu_data when the option is set. Will do that.

Hmm I double checked this now; tsc_disable indeed clears X86_FEATURE_TSC in identify_cpu and that should be always called before anything sched_clock related runs on a CPU.

Also the only possibly faulting division is protected by a cpu_has_tsc which even checks boot_cpu_data. this means even if the resync frequency code was called for some reason before the identify_cpu of a AP it should still work. For the BP this definitely cannot happen.

I also tried it with qemu myself (both one and two cpus) and it worked

Rusty, was this really on a standard kernel? Was it with multiple CPUs?

-Andi

#46 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 11 lines · Andi Kleen
Quoted 6 lines · Andi Kleen
Quoted 1 line

This? I sent it to you earlier this week:

Sorry haven't processed those yet.

Ah. The correct fix here is to clear the tsc flag in boot_cpu_data when the option is set. Will do that.

Hmm I double checked this now; tsc_disable indeed clears X86_FEATURE_TSC in identify_cpu and that should be always called before anything sched_clock related runs on a CPU.

Yes, agreed.

Quoted 10 lines

Also the only possibly faulting division is protected by a cpu_has_tsc which even checks boot_cpu_data. this means even if the resync frequency code was called for some reason before the identify_cpu of a AP it should still work. For the BP this definitely cannot happen.

I also tried it with qemu myself (both one and two cpus) and it worked

Rusty, was this really on a standard kernel? Was it with multiple CPUs?

No, will re-check. Drop it for now, if I can reproduce I'll produce the real fix or a decent explanation.

Thanks,
Rusty.

author
#47 lore raw

PATCH 1/1 i386, numaq: enable TSCs again

In reply to Ingo Molnar

Andi, Andrew, do you remember why we disabled TSCs on NUMAQ? It was slightly async between CPUs, right? In that case we should try the patch below.

Ingo

--------------------->
Subject: [patch] i386, numaq: enable TSCs again
From: Ingo Molnar <[email protected]>

enable TSCs on NUMAQ again. sched_clock() got improved and the scheduler treats it as a per-CPU clock, so there should be no issues from the slightly async TSCs on NUMAQ. Please holler if this is wrong ...

Signed-off-by Ingo Molnar <[email protected]>
---
 arch/i386/kernel/numaq.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
arch/i386/kernel/numaq.c +1 -2
Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/numaq.c
===================================================================
@@ -81,8 +81,7 @@ int __init get_memcfg_numaq(void)
 static int __init numaq_tsc_disable(void)
 {
 	if (num_online_nodes() > 1) {
-		printk(KERN_DEBUG "NUMAQ: disabling TSC\n");
-		tsc_disable = 1;
+		printk(KERN_DEBUG "NUMAQ: NOT disabling TSC. System still ok?\n");
 	}
 	return 0;
 }
#48 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 3 lines · Ingo Molnar

Andi, Andrew, do you remember why we disabled TSCs on NUMAQ? It was slightly async between CPUs, right? In that case we should try the patch below.

I remember. It was far beyond "slightly async;" they would drift minutes apart during reasonable amounts of uptime, though it would take at least several days to drift so far (I don't recall how long it took).

TSC synchronization is uniformly impossible on NUMA-Q. Bootlogs showing the results of the attempts are still extant. They shouldn't end up too far apart right after booting, but I don't have even ballpark estimates. I'd hazard a guess of a few seconds.

NUMA-Q's also supported mixed CPU models in hardware, though that's not really expected to be handled by Linux. I suspect DYNIX/ptx would be used by anyone interested in that.

-- wli

#49 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 8 lines · William Lee Irwin III
Quoted 3 lines · Ingo Molnar

Andi, Andrew, do you remember why we disabled TSCs on NUMAQ? It was slightly async between CPUs, right? In that case we should try the patch below.

I remember. It was far beyond "slightly async;" they would drift minutes apart during reasonable amounts of uptime, though it would take at least several days to drift so far (I don't recall how long it took).

sched_clock should handle that.

Quoted 4 lines

TSC synchronization is uniformly impossible on NUMA-Q. Bootlogs showing the results of the attempts are still extant. They shouldn't end up too far apart right after booting, but I don't have even ballpark estimates. I'd hazard a guess of a few seconds.

You should mark_tsc_instable(), but not tsc disable. In a release or two hopefully even that will be obsolete.

-Andi

#50 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 3 lines · William Lee Irwin III
Quoted 3 lines

I remember. It was far beyond "slightly async;" they would drift minutes apart during reasonable amounts of uptime, though it would take at least several days to drift so far (I don't recall how long it took).

Quoted 1 line · Andi Kleen

sched_clock should handle that.

Quoted 4 lines · William Lee Irwin III
Quoted 4 lines

TSC synchronization is uniformly impossible on NUMA-Q. Bootlogs showing the results of the attempts are still extant. They shouldn't end up too far apart right after booting, but I don't have even ballpark estimates. I'd hazard a guess of a few seconds.

Quoted 2 lines · Andi Kleen

You should mark_tsc_instable(), but not tsc disable. In a release or two hopefully even that will be obsolete.

I don't have any particular preference here. I'm just donating a memory dump. I have no intention of attempting to maintain NUMA-Q support code.

-- wli

author
#51 lore raw

Re: [patch] i386, numaq: enable TSCs again

In reply to William Lee Irwin III
Quoted 8 lines · William Lee Irwin III
Quoted 3 lines

Andi, Andrew, do you remember why we disabled TSCs on NUMAQ? It was slightly async between CPUs, right? In that case we should try the patch below.

I remember. It was far beyond "slightly async;" they would drift minutes apart during reasonable amounts of uptime, though it would take at least several days to drift so far (I don't recall how long it took).

yes, that's what i meant under 'slightly async'. Some AMD CPUs are like that too and sched_clock() now handles that fine. So we should try my patch.

Ingo

#52 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 4 lines · William Lee Irwin III
Quoted 4 lines

I remember. It was far beyond "slightly async;" they would drift minutes apart during reasonable amounts of uptime, though it would take at least several days to drift so far (I don't recall how long it took).

Quoted 3 lines · Ingo Molnar

yes, that's what i meant under 'slightly async'. Some AMD CPUs are like that too and sched_clock() now handles that fine. So we should try my patch.

Sorry, then. I took slight to mean something else. In any event I was only quantifying things. I've no opinion whatsoever on the impact of the code on NUMA-Q, only some recall of its operating characteristics.

-- wli

author
#53 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 7 lines · William Lee Irwin III
Quoted 3 lines

yes, that's what i meant under 'slightly async'. Some AMD CPUs are like that too and sched_clock() now handles that fine. So we should try my patch.

Sorry, then. I took slight to mean something else. In any event I was only quantifying things. I've no opinion whatsoever on the impact of the code on NUMA-Q, only some recall of its operating characteristics.

there's no need to apologize at all! Thanks for reminding us about the time-scale and nature of the TSC drift on NUMAQ. I was worried that maybe the TSC was totally unusable for some reason - but that's fortunately not the case. So we now have one quirk less, hopefully :-)

Ingo

#54 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 5 lines · Ingo Molnar
Quoted 4 lines · William Lee Irwin III
Quoted 3 lines

yes, that's what i meant under 'slightly async'. Some AMD CPUs are like that too and sched_clock() now handles that fine. So we should try my patch.

Quoted 8 lines
Quoted 3 lines

Sorry, then. I took slight to mean something else. In any event I was only quantifying things. I've no opinion whatsoever on the impact of the code on NUMA-Q, only some recall of its operating characteristics.

there's no need to apologize at all! Thanks for reminding us about the time-scale and nature of the TSC drift on NUMAQ. I was worried that maybe the TSC was totally unusable for some reason - but that's fortunately not the case. So we now have one quirk less, hopefully :-)

Last I remember, it was totally useless for timekeeping, but was useful for cpu-local time measurements.

John, it's still useless for time, right? Does sched_clock() really fix it?

-- Dave

#55 lore raw

Re: [patch] i386, numaq: enable TSCs again

Quoted 15 lines · Dave Hansen
Quoted 5 lines · Ingo Molnar
Quoted 4 lines · William Lee Irwin III
Quoted 3 lines

yes, that's what i meant under 'slightly async'. Some AMD CPUs are like that too and sched_clock() now handles that fine. So we should try my patch.

Quoted 8 lines
Quoted 3 lines

Sorry, then. I took slight to mean something else. In any event I was only quantifying things. I've no opinion whatsoever on the impact of the code on NUMA-Q, only some recall of its operating characteristics.

there's no need to apologize at all! Thanks for reminding us about the time-scale and nature of the TSC drift on NUMAQ. I was worried that maybe the TSC was totally unusable for some reason - but that's fortunately not the case. So we now have one quirk less, hopefully :-)

Quoted 5 lines

Last I remember, it was totally useless for timekeeping, but was useful for cpu-local time measurements.

John, it's still useless for time, right? Does sched_clock() really fix it?

Yea, on multi-node NUMAQ the TSC shouldn't be used for timekeeping.

However it should be fine for sched_clock(), or other cpu-local measurements as the TSCs are constant (no cpufreq, no deep sleep states).

-john

Acked-by on v1 1/1
#56 lore raw

Re: [patch] x86_64: fix sched_clock()

In reply to Ingo Molnar
Quoted 11 lines · Ingo Molnar

Subject: [patch] x86_64: fix sched_clock()
From: Ingo Molnar <[email protected]>

sched_clock() is totally broken on x86_64, because it is not defined by the architecture at all! It fell the victim to the opaqueness of __attribute__((weak)) and to non-testing.

the i386 version was supposed to be used. This patch fixes that. Booted and tested on x86_64 and i386.

Signed-off-by Ingo Molnar <[email protected]>
Acked-by Peter Zijlstra <[email protected]>

This does indeed solve my issue.

Quoted 38 lines
---
 arch/x86_64/kernel/Makefile |    3 ++-
 include/asm-x86_64/tsc.h    |    5 +++++
 2 files changed, 7 insertions(+), 1 deletion(-)

Index: linux-cfs-2.6.22-rc2-mm1.q/arch/x86_64/kernel/Makefile
===================================================================

arch/x86_64/kernel/Makefile +2 -1
@@ -9,7 +9,7 @@ obj-y	:= process.o signal.o entry.o trap
 		x8664_ksyms.o i387.o syscall.o vsyscall.o \
 		setup64.o bootflag.o e820.o reboot.o quirks.o i8237.o \
 		pci-dma.o pci-nommu.o alternative.o hpet.o tsc.o bugs.o \
-		perfctr-watchdog.o
+		perfctr-watchdog.o sched-clock.o
 
 obj-$(CONFIG_STACKTRACE)	+= stacktrace.o
 obj-$(CONFIG_X86_MCE)		+= mce.o therm_throt.o

@@ -49,6 +49,7 @@ obj-y				+= pcspeaker.o
 
 CFLAGS_vsyscall.o		:= $(PROFILING) -g0
 
+sched-clock-y			+= ../../i386/kernel/sched-clock.o
 therm_throt-y                   += ../../i386/kernel/cpu/mcheck/therm_throt.o
 bootflag-y			+= ../../i386/kernel/bootflag.o
 legacy_serial-y			+= ../../i386/kernel/legacy_serial.o

Index: linux-cfs-2.6.22-rc2-mm1.q/include/asm-x86_64/tsc.h
===================================================================

include/asm-x86_64/tsc.h +5 -0
@@ -1 +1,6 @@
 #include <asm-i386/tsc.h>
+
+/*
+ * To be able to share sched-clock.c between i386 and x86_64:
+ */
+#define tsc_disable 0
author
#57 lore raw

Re: [patch] x86_64: fix sched_clock()

Quoted 8 lines · Peter Zijlstra
Quoted 7 lines · Ingo Molnar

Subject: [patch] x86_64: fix sched_clock()
From: Ingo Molnar <[email protected]>

sched_clock() is totally broken on x86_64, because it is not defined by the architecture at all! It fell the victim to the opaqueness of __attribute__((weak)) and to non-testing.

Quoted 7 lines
Quoted 4 lines

the i386 version was supposed to be used. This patch fixes that. Booted and tested on x86_64 and i386.

Signed-off-by Ingo Molnar <[email protected]>
Acked-by Peter Zijlstra <[email protected]>

This does indeed solve my issue.

great! Btw., this is also accidental proof that CFS works well even with a jiffies granularity sched_clock() ;-)

Ingo

Acked-by
#58 lore raw
In reply to Ingo Molnar
Quoted 9 lines · Ingo Molnar
Quoted 2 lines · Ingo Molnar

updated version of the cleanup patch below, renamed r_s_c to resync_freq, and changed 'f' to 'freq'.

updated one below.

Ingo

Quoted 7 lines

------------------->
Subject: [patch] sched_clock(): cleanups
From: Ingo Molnar <[email protected]>

clean up sched-clock.c - mostly comment style fixes.

Signed-off-by Ingo Molnar <[email protected]>
Acked-by Peter Zijlstra <[email protected]>

Looks good.

Quoted 211 lines
---
 arch/i386/kernel/sched-clock.c |  110 +++++++++++++++++++++++++----------------
 1 file changed, 69 insertions(+), 41 deletions(-)

Index: linux-cfs-2.6.22-rc2-mm1.q/arch/i386/kernel/sched-clock.c
===================================================================

arch/i386/kernel/sched-clock.c +69 -41
@@ -60,18 +60,20 @@ DEFINE_PER_CPU(struct sc_data, sc_data) 
  */
 unsigned long long native_sched_clock(void)
 {
-	unsigned long long r;
 	struct sc_data *sc = &get_cpu_var(sc_data);
+	unsigned long long r;
+	unsigned long flags;
 
 	if (sc->unstable) {
-		unsigned long flags;
 		r = (jiffies_64 - sc->sync_base) * (1000000000 / HZ);
 		r += sc->ns_base;
 		local_irq_save(flags);
-		/* last_val is used to avoid non monotonity on a
-		   stable->unstable transition. Make sure the time
-		   never goes to before the last value returned by
-		   the TSC clock */
+		/*
+		 * last_val is used to avoid non monotonity on a
+		 * stable->unstable transition. Make sure the time
+		 * never goes to before the last value returned by
+		 * the TSC clock
+		 */
 		if (r <= sc->last_val)
 			r = sc->last_val + 1;
 		sc->last_val = r;

@@ -87,8 +89,10 @@ unsigned long long native_sched_clock(vo
 	return r;
 }
 
-/* We need to define a real function for sched_clock, to override the
-   weak default version */
+/*
+ * We need to define a real function for sched_clock, to override the
+ * weak default version
+ */
 #ifdef CONFIG_PARAVIRT
 unsigned long long sched_clock(void)
 {

@@ -101,9 +105,11 @@ unsigned long long sched_clock(void)
 
 static int no_sc_for_printk;
 
-/* printk clock: when it is known the sc results are very non monotonic
-   fall back to jiffies for printk. Other sched_clock users are supposed
-   to handle this. */
+/*
+ * printk clock: when it is known the sc results are very non monotonic
+ * fall back to jiffies for printk. Other sched_clock users are supposed
+ * to handle this.
+ */
 unsigned long long printk_clock(void)
 {
 	if (unlikely(no_sc_for_printk))

@@ -119,35 +125,46 @@ static void resync_sc_freq(struct sc_dat
 		sc->unstable = 1;
 		return;
 	}
-	/* Handle nesting, but when we're zero multiple calls in a row
-	   are ok too and not a bug */
+	/*
+	 * Handle nesting, but when we're zero multiple calls in a row
+	 * are ok too and not a bug
+	 */
 	if (sc->unstable > 0)
 		sc->unstable--;
 	if (sc->unstable)
 		return;
-	/* RED-PEN protect with seqlock? I hope that's not needed
-	   because sched_clock callers should be able to tolerate small
-	   errors. */
+	/*
+	 * RED-PEN protect with seqlock? I hope that's not needed
+	 * because sched_clock callers should be able to tolerate small
+	 * errors.
+	 */
 	sc->ns_base = ktime_to_ns(ktime_get());
 	rdtscll(sc->sync_base);
 	sc->cyc2ns_scale = (1000000 << CYC2NS_SCALE_FACTOR) / newfreq;
 }
 
-static void call_r_s_f(void *arg)
+static void __resync_freq(void *arg)
 {
-	struct cpufreq_freqs *freq = arg;
-	unsigned f = freq->new;
-	if (!f)
-		f = cpufreq_get(freq->cpu);
-	if (!f)
-		f = tsc_khz;
-	resync_sc_freq(&per_cpu(sc_data, freq->cpu), f);
+	struct cpufreq_freqs *freqp = arg;
+	unsigned freq = freqp->new;
+
+	if (!freq) {
+		freq = cpufreq_get(freqp->cpu);
+		/*
+		 * Still no frequency? Then fall back to tsc_khz:
+		 */
+		if (!freq)
+			freq = tsc_khz;
+	}
+	resync_sc_freq(&per_cpu(sc_data, freqp->cpu), freq);
 }
 
-static void call_r_s_f_here(void *arg)
+static void resync_freq(void *arg)
 {
-	struct cpufreq_freqs f = { .cpu = get_cpu(), .new = 0 };
-	call_r_s_f(&f);
+	struct cpufreq_freqs f = { .new = 0 };
+
+	f.cpu = get_cpu();
+	__resync_freq(&f);
 	put_cpu();
 }
 

@@ -155,9 +172,11 @@ static int sc_freq_event(struct notifier
 			 void *data)
 {
 	struct cpufreq_freqs *freq = data;
-	int cpu = get_cpu();
-	struct sc_data *sc = &per_cpu(sc_data, cpu);
+	struct sc_data *sc;
+	int cpu;
 
+	cpu = get_cpu();
+	sc = &per_cpu(sc_data, cpu);
 	if (cpu_has(&cpu_data[cpu], X86_FEATURE_CONSTANT_TSC))
 		goto out;
 	if (freq->old == freq->new)

@@ -167,25 +186,30 @@ static int sc_freq_event(struct notifier
 	case CPUFREQ_SUSPENDCHANGE:
 		/* Mark TSC unstable during suspend/resume */
 	case CPUFREQ_PRECHANGE:
-		/* Mark TSC as unstable until cpu frequency change is done
-		   because we don't know when exactly it will change.
-		   unstable in used as a counter to guard against races
-		   between the cpu frequency notifiers and normal resyncs */
+		/*
+		 * Mark TSC as unstable until cpu frequency change is done
+		 * because we don't know when exactly it will change.
+		 * unstable in used as a counter to guard against races
+		 * between the cpu frequency notifiers and normal resyncs
+		 */
 		sc->unstable++;
 		/* FALL THROUGH */
 	case CPUFREQ_RESUMECHANGE:
 	case CPUFREQ_POSTCHANGE:
-		/* Frequency change or resume is done -- update everything and
-		   mark TSC as stable again. */
+		/*
+		 * Frequency change or resume is done -- update everything
+		 * and mark TSC as stable again.
+		 */
 		if (cpu == freq->cpu)
 			resync_sc_freq(sc, freq->new);
 		else
-			smp_call_function_single(freq->cpu, call_r_s_f,
+			smp_call_function_single(freq->cpu, __resync_freq,
 						 freq, 0, 1);
 		break;
 	}
 out:
 	put_cpu();
+
 	return NOTIFY_DONE;
 }
 

@@ -197,9 +221,11 @@ static int __cpuinit
 sc_cpu_event(struct notifier_block *self, unsigned long event, void *hcpu)
 {
 	long cpu = (long)hcpu;
+
 	if (event == CPU_ONLINE) {
 		struct cpufreq_freqs f = { .cpu = cpu, .new = 0 };
-		smp_call_function_single(cpu, call_r_s_f, &f, 0, 1);
+
+		smp_call_function_single(cpu, __resync_freq, &f, 0, 1);
 	}
 	return NOTIFY_DONE;
 }

@@ -209,13 +235,15 @@ static __init int init_sched_clock(void)
 	if (unsynchronized_tsc())
 		no_sc_for_printk = 1;
 
-	/* On a race between the various events the initialization might be
-	   done multiple times, but code is tolerant to this */
+	/*
+	 * On a race between the various events the initialization might be
+	 * done multiple times, but code is tolerant to this
+	 */
 	cpufreq_register_notifier(&sc_freq_notifier,
 				CPUFREQ_TRANSITION_NOTIFIER);
 	hotcpu_notifier(sc_cpu_event, 0);
-	on_each_cpu(call_r_s_f_here, NULL, 0, 0);
+	on_each_cpu(resync_freq, NULL, 0, 0);
+
 	return 0;
 }
 core_initcall(init_sched_clock);
-