client: improve implementation of CPU throttling #5233
Conversation
Old: if target CPU usage is (say) 1%, we'd unthrottle, sleep 1 sec, throttle, and sleep 99 sec. A change to the usage pref wouldn't be enforced during that sleep. New: do something every second; changes are seen immediately
|
The code looks fine to me. |
|
I'd say 'inconclusive'. Testing on Windows 7, I couldn't reproduce the stated problem with client 7.22.1: there didn't seem to be a delay when switching back from 1% to 100% of time. Having said that, it isn't obvious when CPU usage is paused - the task status in the Manager display (Advanced - tasks) remains as 'Running' throughout, and the only event logged is the 'Resume' (twice per task) when cpu_sched logging is enabled. The only clue is that elapsed time stops incrementing.
I switched to the artifact for this PR, with some difficulty: Windows Security Essentials flagged the client archive as a virus (Manager and apps were clean). I had to download it on a Linux machine and email it to myself ... I couldn't detect any change in behaviour between old and new. |
|
@RichardHaselgrove, thank you for testing this.
I have asked the reported of this issue to test this on their side.
Definitely a false positive detection. If you have any doubts - you can use this service to check any file with several endpoint security software: https://www.virustotal.com/gui/home/upload |
| if (limit >= 100) { | ||
| if (gstate.tasks_throttled) { | ||
| gstate.active_tasks.unsuspend_all(SUSPEND_REASON_CPU_THROTTLE); |
There was a problem hiding this comment.
Does this need the mutex locking?
| } | ||
| x -= 100; | ||
| } else { | ||
| if (!gstate.tasks_throttled) { |
There was a problem hiding this comment.
Is there still a problem that changing prefs triggers a reschedule, which will resume any suspended task without resetting tasks_throttled, so we don’t get back in here even if we should?
which can run jobs that are currently throttled. Clear the tasks_throttled flag, so that the throttling logic can throttle these again if needed.
|
I think I fixed the problem Brian pointed out, and one or two others. Please test again. |
Old: if target CPU usage is (say) 1%,
we'd unthrottle, sleep 1 sec, throttle, and sleep 99 sec. A change to the usage pref wouldn't be enforced during that sleep.
New: do something every second; changes are seen immediately.
This makes a difference only when setting is very close to 0 or 100
Fixes #5231