race condition on min/max update?

DISCUSSION Viresh Kumar 3 participants 4 messages Feb 26, 2016 1 message not in the archive

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Viresh Kumar

author

2016-02-26 02:53 UTC #1 lore raw

+Rafael and linux-pm list

Quoted 6 lines · Steve Muckle

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.

Quoted 7 lines

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.

Signature · 1 line

viresh

Rafael J. Wysocki

2016-02-26 14:09 UTC #2 lore raw

Quoted 14 lines · Viresh Kumar

+Rafael and linux-pm list
Quoted 11 lines · Steve Muckle
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?

Quoted 8 lines

Quoted 2 lines
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

Viresh Kumar

author

2016-02-26 14:37 UTC #3 lore raw

Quoted 4 lines · Rafael J. Wysocki

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.

Signature · 1 line

viresh

Rafael J. Wysocki

2016-02-26 21:05 UTC #4 lore raw

Quoted 7 lines · Viresh Kumar

Quoted 4 lines · Rafael J. Wysocki
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