Re: race condition on min/max update?

Re: race condition on min/max update?

[Viresh Kumar]

+Rafael and linux-pm list

On 24-02-16, 12:36, Steve Muckle wrote:
> Hi Viresh,
> 
> A while back you mentioned that it is not required to hold policy->rwsem
> when calling __cpufreq_driver_target. This simplified things for us a
> bit in schedfreq.
> 
> But if policy->rwsem is not held, how does the frequency transition
> process protect itself against policy->min or policy->max changing?
> Don't you have a race there where it'd be possible to end up with the
> frequency being set to a rate lower than min or higher than max?
> 
> I've taken a cursory look through the code and don't see what would
> prevent this from happening.

It seems that we were not being consistent with this stuff and might have
ignored this race altogether. On some cases we do update the frequency with
policy->rwsem and in others we don't.

It requires a closer look and cleanup I believe.

-- 
viresh

Re: race condition on min/max update?

[Rafael J. Wysocki]

On Friday, February 26, 2016 08:23:15 AM Viresh Kumar wrote:
> +Rafael and linux-pm list
> 
> On 24-02-16, 12:36, Steve Muckle wrote:
> > Hi Viresh,
> > 
> > A while back you mentioned that it is not required to hold policy->rwsem
> > when calling __cpufreq_driver_target. This simplified things for us a
> > bit in schedfreq.
> > 
> > But if policy->rwsem is not held, how does the frequency transition
> > process protect itself against policy->min or policy->max changing?
> > Don't you have a race there where it'd be possible to end up with the
> > frequency being set to a rate lower than min or higher than max?
> > 
> > I've taken a cursory look through the code and don't see what would
> > prevent this from happening.
> 
> It seems that we were not being consistent with this stuff and might have
> ignored this race altogether. On some cases we do update the frequency with
> policy->rwsem and in others we don't.
> 
> It requires a closer look and cleanup I believe.

A min/max update should trigger ->governor(limits) and then the governor
is responsible for taking care of the race (if it needs to).

At least that's how it should work IMO.

Thanks,
Rafael

Re: race condition on min/max update?

[Viresh Kumar]

On 26-02-16, 15:11, Rafael J. Wysocki wrote:
> A min/max update should trigger ->governor(limits) and then the governor
> is responsible for taking care of the race (if it needs to).
> 
> At least that's how it should work IMO.

Urg, simple thing.

-- 
viresh

Re: race condition on min/max update?

[Rafael J. Wysocki]

On Fri, Feb 26, 2016 at 3:37 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 26-02-16, 15:11, Rafael J. Wysocki wrote:
>> A min/max update should trigger ->governor(limits) and then the governor
>> is responsible for taking care of the race (if it needs to).
>>
>> At least that's how it should work IMO.
>
> Urg, simple thing.

It's not particularly clean, though, because ->governor(LIMITS) is
called after min/max have been updated, so the governor may end up
using a frequency that's completely off for a little while between
updating min/max and calling ->governor(LIMITS) (the LIMITS call
really is something like a "you have new min and max, fix up yourself"
notification).  That's why I added a check for min <= max to the
schedutil prototype.

Thanks,
Rafael