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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
