| Message ID | 20151207075027.GC3294@ubuntu |
|---|---|
| State | New |
| Headers | show |
On 08-12-15, 00:17, Rafael J. Wysocki wrote: > In fact, I've already folded the above changes into the $subject patch (but this > is an exception). Yeah I know, I would have sent a patch this morning though. -- viresh -- 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/
On 07-12-15, 23:43, Rafael J. Wysocki wrote: > On Monday, December 07, 2015 01:20:27 PM Viresh Kumar wrote: > > At this point we might end up decrementing skip_work from > > gov_cancel_work() and then cancel the work which we haven't queued > > yet. And the end result will be that the work is still queued while > > gov_cancel_work() has finished. > > I'm not quite sure how that can happen. I will describe that towards the end of this email. > There is a bug in this code snippet, but it may cause us to fail to queue > the work at all, so the incrementation and the check need to be done > under the spinlock. What bug ? > > And we have to keep the atomic operation, as well as queue_work() > > within the lock. > > Putting queue_work() under the lock doesn't prevent any races from happening, Then I am not able to think about it properly, but I will at least present my case here :) > because only one of the CPUs can execute that part of the function anyway. > > > > queue_work(system_wq, &shared->work); > > > > > > and the remaining incrementation and decrementation of skip_work are replaced > > > with the corresponding atomic operations, it still should work, no? > > Well, no, the above wouldn't work. > > But what about something like this instead: > > if (atomic_inc_return(&shared->skip_work) > 1) > atomic_dec(&shared->skip_work); > else > queue_work(system_wq, &shared->work); > > (plus the changes requisite replacements in the other places)? > > Only one CPU can see the result of the atomic_inc_return() as 1 and this is the > only one that will queue up the work item, unless I'm missing anything super > subtle. Looks like you are talking about the race between different timer handlers, which race against queuing the work. Sorry if you are not. But I am not talking about that thing.. 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 -> cancel_work_sync(&shared->work); -> queue_work(); -> gov_cancel_timers(shared->policy); -> shared->skip_work = 0; dbs_work_handler(); And according to how I understand it, we are screwed up at this point. And its the same old bug which I fixed recently (which we hacked up by using gov-lock earlier). The work handler is still active after the policy-governor is stopped. And your latest patch looks wrong for the same reason ... -- viresh -- 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/
On 08-12-15, 14:18, Rafael J. Wysocki wrote: > Well, if the timer function runs on all CPUs at the same time, they all > can see skip_work > 1 and none of them will queue the work. You are talking about code after my patch, right? Will will all of them see it > 1? At least one of them will see it 0 and queue the work, unless the governor is stopped completely. > You are right, I've overlooked that race (but then it is rather easy to > overlook). Yeah, we (at least I) took a long time to understand that this was the real problem we always had and so fixed it recently. -- viresh -- 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/
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index c9e420bd0eec..d8a89e653933 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -230,6 +230,7 @@ static void dbs_work_handler(struct work_struct *work) struct dbs_data *dbs_data; unsigned int sampling_rate, delay; bool eval_load; + unsigned long flags; policy = shared->policy; dbs_data = policy->governor_data; @@ -257,7 +258,10 @@ static void dbs_work_handler(struct work_struct *work) delay = dbs_data->cdata->gov_dbs_timer(policy, eval_load); mutex_unlock(&shared->timer_mutex); + spin_lock_irqsave(&shared->timer_lock, flags); shared->skip_work--; + spin_unlock_irqrestore(&shared->timer_lock, flags); + gov_add_timers(policy, delay); }