Re: [PATCH V2 5/6] cpufreq: governor: replace per-cpu delayed work with timers

[Rafael J. Wysocki]

On Monday, December 07, 2015 01:20:27 PM Viresh Kumar wrote:
> On 07-12-15, 02:28, Rafael J. Wysocki wrote:
> > What about if that happens in parallel with the decrementation in
> > dbs_work_handler()?
> > 
> > Is there anything preventing that from happening?
> 
> Hmmm, you are right. Following is required for that.
> 
> 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);
>  }

OK, so can you please send an updated patch with the above change folded in?

> > That aside, I think you could avoid using the spinlock altogether if the
> > counter was atomic (and which would make the above irrelevant too).
> > 
> > Say, skip_work is atomic the the relevant code in dbs_timer_handler() is
> > written as
> > 
> > 	atomic_inc(&shared->skip_work);
> > 	smp_mb__after_atomic();
> > 	if (atomic_read(&shared->skip_work) > 1)
> > 		atomic_dec(&shared->skip_work);
> > 	else
> 
> 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.

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.

> 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,
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.

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/