[PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier() - Kernel

This is a discussion on [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier() - Kernel ; 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. Signed-off-by: Karsten Wiese --- Hi, ...

+ Reply to Thread
Results 1 to 3 of 3

Thread: [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier()

  1. [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier()


    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.

    Signed-off-by: Karsten Wiese
    ---

    Hi,

    the codepath changed by patch is untested as my smp's cpu has
    X86_FEATURE_CONSTANT_TSC set.
    A printk() inserted at the beginning of time_cpufreq_notifier() showed regular
    cases of "smp_processor_id() != freq->cpu".

    Related mail/thread: http://lkml.org/lkml/2007/12/7/130.

    Thanks,
    Karsten

    arch/x86/kernel/tsc_32.c | 4 +---
    arch/x86/kernel/tsc_64.c | 4 +---
    2 files changed, 2 insertions(+), 6 deletions(-)

    diff --git a/arch/x86/kernel/tsc_32.c b/arch/x86/kernel/tsc_32.c
    index d7498b3..2f5f9a4 100644
    --- a/arch/x86/kernel/tsc_32.c
    +++ b/arch/x86/kernel/tsc_32.c
    @@ -256,9 +256,7 @@ time_cpufreq_notifier(struct notifier_block *nb, unsigned long val, void *data)
    ref_freq, freq->new);
    if (!(freq->flags & CPUFREQ_CONST_LOOPS)) {
    tsc_khz = cpu_khz;
    - preempt_disable();
    - set_cyc2ns_scale(cpu_khz, smp_processor_id());
    - preempt_enable();
    + set_cyc2ns_scale(cpu_khz, freq->cpu);
    /*
    * TSC based sched_clock turns
    * to junk w/ cpufreq
    diff --git a/arch/x86/kernel/tsc_64.c b/arch/x86/kernel/tsc_64.c
    index 01fc9f0..bd2e2c3 100644
    --- a/arch/x86/kernel/tsc_64.c
    +++ b/arch/x86/kernel/tsc_64.c
    @@ -149,9 +149,7 @@ static int time_cpufreq_notifier(struct notifier_block *nb, unsigned long val,
    mark_tsc_unstable("cpufreq changes");
    }

    - preempt_disable();
    - set_cyc2ns_scale(tsc_khz_ref, smp_processor_id());
    - preempt_enable();
    + set_cyc2ns_scale(tsc_khz_ref, freq->cpu);

    return 0;
    }
    --
    1.5.3.3
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  2. Re: [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier()


    * Karsten Wiese wrote:

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


    thanks Karsten, applied.

    Ingo
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

  3. Re: [PATCH] x86: Fix call to set_cyc2ns_scale() from time_cpufreq_notifier()

    Karsten Wiese 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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

+ Reply to Thread