public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Andi Kleen <andi@firstfloor.org>
To: Karsten Wiese <fzu@wemgehoertderstaat.de>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@elte.hu>,
	Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier()
Date: Mon, 07 Apr 2008 14:33:51 +0200	[thread overview]
Message-ID: <87r6dhrgi8.fsf@basil.nowhere.org> (raw)
In-Reply-To: <200804071214.45791.fzu@wemgehoertderstaat.de> (Karsten Wiese's message of "Mon, 7 Apr 2008 12:14:45 +0200")

Karsten Wiese <fzu@wemgehoertderstaat.de> writes:

> In time_cpufreq_notifier() the cpu id to act upon is held in freq->cpu. Use it
> instead of smp_processor_id() in the call to set_cyc2ns_scale().
> This makes the preempt_*able() unnecessary and lets set_cyc2ns_scale() update
> the intended cpu's cyc2ns.

It is still wrong because set_cyc2ns_scale() needs to execute on the target CPU
because it does rdtsc on that CPU. If you're on another CPU you'll
get the wrong values if the TSCs are not synchronized.

Still wrong as just disabling preemption around the call is not correct,
it would need to cover the complete notifier event and also the cpufreq
code would need to guarantee it is always already running on the correct
CPU (not sure that is the case currently)

The whole thing needs to be either preempt disabled or the RDTSC update code
needs to be moved into a smp_call_function_simple()  [my old 
"redesigned sched_clock code" did that]

Using smp_call_function_simple() would be probably the right fix,
perhaps conditional on a check for unsync TSC.

-Andi

      parent reply	other threads:[~2008-04-07 12:34 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-04-07 10:14 [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier() Karsten Wiese
2008-04-07 10:26 ` Ingo Molnar
2008-04-07 12:33 ` Andi Kleen [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87r6dhrgi8.fsf@basil.nowhere.org \
    --to=andi@firstfloor.org \
    --cc=fzu@wemgehoertderstaat.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@elte.hu \
    --cc=tglx@linutronix.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox