balloons: set frequency scaling governor only when requested

[fmuyassarov]

Avoid enforcing the frequency scaling governor if the user hasn't explicitly requested it. Previously, we attempted to set it regardless, leading to unnecessary error logs. Furthermore, fix formatting issue when logging error case.

[fmuyassarov]

Thanks @askervin for spotting the bug. PTAL
cc @klihub

[fmuyassarov]

@klihub I have now removed the leftover loop.

[askervin]

Two nits, otherwise looks good.

[fmuyassarov]

@klihub addressed the comments. PTAL

Needs a re-review.

[askervin]

Looking at the governor solution in high level (first implementation and this patch), I'm starting to think that we are adding unnecessary complexity. And I'm slightly worried about it, as we should be adding here other controls as well. If we'd follow the same pattern then, there would be possibly third enforeCpufreqXXX (C-state controls and/or something else) that would needed to be called in the sequence of these two. Currently the two already have different assumption on who is checking if the class exists.

Instead of this complexity and introducing new function --- please tell me if I'm mistaken --- we could add only a single if to the original enforeCpufreq() after checking that the class exists. If governor is defined, then set it to the CPUs, and that's it. We would not need to introduce extra function, or have different error paths.

What do you think, @fmuyassarov and @klihub , could this be stripped down to single "if", which would make much easier to add new controls in the future in the same two-line manner?

[fmuyassarov]

Looking at the governor solution in high level (first implementation and this patch), I'm starting to think that we are adding unnecessary complexity. And I'm slightly worried about it, as we should be adding here other controls as well. If we'd follow the same pattern then, there would be possibly third enforeCpufreqXXX (C-state controls and/or something else) that would needed to be called in the sequence of these two. Currently the two already have different assumption on who is checking if the class exists.

Instead of this complexity and introducing new function --- please tell me if I'm mistaken --- we could add only a single if to the original enforeCpufreq() after checking that the class exists. If governor is defined, then set it to the CPUs, and that's it. We would not need to introduce extra function, or have different error paths.

What do you think, @fmuyassarov and @klihub , could this be stripped down to single "if", which would make much easier to add new controls in the future in the same two-line manner?

I think it makes sense.

[klihub]

Looking at the governor solution in high level (first implementation and this patch), I'm starting to think that we are adding unnecessary complexity. And I'm slightly worried about it, as we should be adding here other controls as well. If we'd follow the same pattern then, there would be possibly third enforeCpufreqXXX (C-state controls and/or something else) that would needed to be called in the sequence of these two. Currently the two already have different assumption on who is checking if the class exists.

Instead of this complexity and introducing new function --- please tell me if I'm mistaken --- we could add only a single if to the original enforeCpufreq() after checking that the class exists. If governor is defined, then set it to the CPUs, and that's it. We would not need to introduce extra function, or have different error paths.

What do you think, @fmuyassarov and @klihub , could this be stripped down to single "if", which would make much easier to add new controls in the future in the same two-line manner?

Sounds like a really good idea to me. We should consider the governor part of the freq. setting and things get much cleaner.

Let's refactor this based on the latest suggestion by @askervin.

[fmuyassarov]

@askervin @klihub I have refactored the code now. PTAL.

[askervin]

In my opinion, we should not introduce a fatal failure due to missing parameters for a CPU class.

[klihub]

In my opinion, we should not introduce a fatal failure due to missing parameters for a CPU class.

+1. Definitely not.

[fmuyassarov]

ping @askervin

[askervin]

LGTM, Thanks @fmuyassarov!