Re: [PATCH V2 Resend 4/4] timer: Migrate running timer

[Viresh Kumar]

On 27 November 2012 19:17, Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
> On Tue, 2012-11-06 at 16:08 +0530, Viresh Kumar wrote:

>> diff --git a/kernel/timer.c b/kernel/timer.c
>> @@ -729,6 +730,12 @@ __mod_timer(struct timer_list *timer, unsigned long expires,
>>
>>       base = lock_timer_base(timer, &flags);
>>
>> +     if (timer->sched_del) {
>> +             /* Don't schedule it again, as it is getting deleted */
>> +             ret = -EBUSY;
>> +             goto out_unlock;
>> +     }
>> +
>>       ret = detach_if_pending(timer, base, false);
>>       if (!ret && pending_only)
>>               goto out_unlock;
>> @@ -746,21 +753,12 @@ __mod_timer(struct timer_list *timer, unsigned long expires,
>>       new_base = per_cpu(tvec_bases, cpu);
>>
>>       if (base != new_base) {
>> -             /*
>> -              * We are trying to schedule the timer on the local CPU.
>> -              * However we can't change timer's base while it is running,
>> -              * otherwise del_timer_sync() can't detect that the timer's
>> -              * handler yet has not finished. This also guarantees that
>> -              * the timer is serialized wrt itself.
>> -              */
>> -             if (likely(base->running_timer != timer)) {
>> -                     /* See the comment in lock_timer_base() */
>> -                     timer_set_base(timer, NULL);
>> -                     spin_unlock(&base->lock);
>> -                     base = new_base;
>> -                     spin_lock(&base->lock);
>> -                     timer_set_base(timer, base);
>> -             }
>> +             /* See the comment in lock_timer_base() */
>> +             timer_set_base(timer, NULL);
>> +             spin_unlock(&base->lock);
>> +             base = new_base;
>> +             spin_lock(&base->lock);
>> +             timer_set_base(timer, base);
>>       }

> I don't think this is good enough. For one thing, it doesn't handle
> try_to_del_timer_sync() or even del_timer_sync() for that matter. As
> that may return success when the timer happens to be running on another
> CPU.
>
> We have this:
>
>         CPU0                    CPU1
>         ----                    ----
>    timerA (running)
>    mod_timer(timerA)
>    [ migrate to CPU2 ]
>    release timer base lock
>                            del_timer_sync(timerA)
>                            timer->sched_del = true
>                            try_to_del_timer_sync(timerA)
>                                 base(CPU2)->timer != timerA
>                                 [TRUE!]
>   timerA (finishes)
>
> Fail!

Hi Steven/Thomas,

I came back to this patch after completing some other stuff and posting
wq part of this patchset separately.

I got your point and understand how this would fail.

@Thomas: I need your opinion first. Do you like this concept of migrating
running timer or not? Or you see some basic problem with this concept?

If no (i.e. i can go ahead with another version), then i have some solution to
fix earlier problems reported by Steven:

The problem lies with del_timer_sync() which just checks
base->running_timer != timer to check if timer is currently running or not.

What if we add another variable in struct timer_list, that will store if we are
running timer callback or not. And so, before we call callback in timer core,
we will set this variable and will reset it after finishing callback.

del_timer_sync() will have something like:

if (base->running_timer != timer)
    remove timer and return;
else if (timer->running_callback)
    go back to its loop...

So, with my existing patch + this change, del_timer_sync() will not return
back unless the callback is completed on CPU0.

But what can happen now is base->running_timer == timer can be true
for two cpus simultaneously cpu0 (running callback) and cpu2 (running hardware
timer). Will that cause any issues?

--
viresh
--
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/