Re: [PATCH][experimantal] cpufreq: governor: Use an atomic variable for synchronization

[Rafael J. Wysocki]

On Tuesday, December 08, 2015 07:06:33 PM Viresh Kumar wrote:
> On 08-12-15, 14:30, Rafael J. Wysocki wrote:
> > OK, but instead of relying on the spinlock to wait for the already running
> 
> That's the purpose of the spinlock, not a side-effect.
> 
> > dbs_timer_handler() in gov_cancel_work() (which is really easy to overlook
> > and should at least be mentioned in a comment) we can wait for it explicitly.
> 
> I agree, and I will add explicit comment about it.
> 
> > That is, if the relevant code in gov_cancel_work() is like this:
> > 
> > 
> > 	atomic_inc(&shared->skip_work);
> > 	gov_cancel_timers(shared->policy);
> > 	cancel_work_sync(&shared->work);
> > 	gov_cancel_timers(shared->policy);
> 
> Apart from it being *really* ugly (we should know exactly what should
> be done, it rather looks like hit and try),

We know what should be done.  We need to wait for the timer function to
complete, then cancel the work item spawned by it (if any) and then
cancel the timers set by that work item.

> it is still racy.
> 
> > 	atomic_set(&shared->skip_work, 0);
> > 
> > then the work item should not be leaked behind the cancel_work_sync() any more
> > AFAICS.
> 
> Suppose queue_work() isn't done within the spin lock.
> 
> CPU0                                            CPU1
> 
> cpufreq_governor_stop()                         dbs_timer_handler()
> -> gov_cancel_work()                            -> lock
>                                                 -> shared->skip_work++, as skip_work was 0. //skip_work=1
>                                                 -> unlock
>    -> lock
>    -> shared->skip_work++; //skip_work=2

(*)

>    -> unlock
>                                                 -> queue_work();
>    -> gov_cancel_timers(shared->policy);
>                                                 dbs_work_handler();
>                                                 -> queue-timers again (as we aren't checking skip_work here)

skip_work = 1 (because dbs_work_handler() decrements it).

>    -> cancel_work_sync(&shared->work);
>                                                 dbs_timer_handler()
>                                                 -> lock
>                                                 -> shared->skip_work++, as skip_work was 0.

No, it wasn't 0, it was 1, because (*) incremented it
and it has only been decremented once by dbs_work_handler().

> //skip_work=1
>                                                 -> unlock

And the below won't happen.

>                                                 ->queue_work()
>
>    -> gov_cancel_timers(shared->policy);
>    -> shared->skip_work = 0;
> 
> 
> And we have the same situation again.

I don't really think so.

Thanks,
Rafael

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