Re: clock drift in set_task_cpu()

[Jack Daniel <wanders.thirst@gmail.com>]

Hi Peter,

As a follow up on this...

On Wed, Jul 21, 2010 at 5:10 PM, Jack Daniel <wanders.thirst@gmail.com> wrote:
> Hi Peter/Ingo,
>
> I have a query with the kernel code that was changed not too long time
> back in v2.6.33-rc1 commit id 5afcdab706d6002cb02b567ba46e650215e694e8
> [tip:sched/urgent] sched: Remove rq->clock coupling from
> set_task_cpu()
>
> void set_task_cpu(struct task_struct *p, unsigned int new_cpu)
> {
> int old_cpu = task_cpu(p);
> struct rq *old_rq = cpu_rq(old_cpu), *new_rq = cpu_rq(new_cpu);
> struct cfs_rq *old_cfsrq = task_cfs_rq(p),
>      *new_cfsrq = cpu_cfs_rq(old_cfsrq, new_cpu);
> u64 clock_offset;
>
> clock_offset = old_rq->clock - new_rq->clock;
> ---
>
> On a Xeon 55xx with 8 CPU's, I found out the new_rq->clock value is
> sometimes larger than old_rq->clock and so clock_offset tends to warp
> around leading to incorrect values. You have very correctly noted in
> the commit header that all functions that access set_task_cpu() must
> do so after a call to sched_clock_remote(), in this case the function
> is sched_fork(). I validated by adding update_rq_clock(old_rq); into
> set_task_cpu() and that seems to fix the issue. But I noticed that
> since CONFIG_HAVE_UNSTABLE_SCHED_CLOCK is already set, if
> (sched_clock_stable)  in sched_clock_cpu() will yield to true and the
> flow never gets to sched_clock_remote() or sched_clock_local().
>
> What do you think is the best way to approach the problem *assuming
> the older kernel*, since I believe the problem still exists? That is
> to reinstate your axiom ".... which should ensure the observed time
> between these two cpus is monotonic"
>
> 1) CONFIG_HAVE_UNSTABLE_SCHED_CLOCK cannot be disabled since it is set
> by default for x86
> 2) Does one create a new function with just this line of code?
> fix_clock_drift()
> {
> if (cpu != smp_processor_id())
>                clock = sched_clock_remote(scd);
>        else
>                clock = sched_clock_local(scd);
>
>        return clock;
> }
>

I bet you would have had come across this problem and hence chose to
surgically remove the impeding code with commit 5afcdab. I now think
it was a good choice but the right thing would have been to correct
the problem itself. I think this code should have solved the problem.

diff --git a/kernel/sched.c b/kernel/sched.c
index 1d39b00..5fd63f2 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -2068,6 +2068,13 @@ void set_task_cpu(struct task_struct *p,
unsigned int new_cpu)
        struct cfs_rq *old_cfsrq = task_cfs_rq(p),
                      *new_cfsrq = cpu_cfs_rq(old_cfsrq, new_cpu);
        u64 clock_offset;
+       unsigned long flags;
+
+       rmb();
+       local_irq_save(flags);
+       update_rq_clock(old_rq);
+       update_rq_clock(new_rq);
+       local_irq_restore(flags);


Thanks and regards,
Jack