Turn CpuFrequencyHz into a struct and reformat parts of code

Now, accidental pasing of int instead of a frequency is detected by the type system, and it's easier to print the frequency as a string.

src/config.cpp

@@ -560,14 +560,14 @@ void Config::create_scheduler_objects(const CgroupConfig &c,
             auto budget = chrono::milliseconds(yprocess["budget"].as<int>());
             auto budget_jitter = chrono::milliseconds(yprocess["jitter"].as<int>());
             // the freqs are already checked during validation, they should be correct
-            auto _a53_freq =
-              yprocess["_a53_freq"]
-                ? std::optional(1000 * 1000 * yprocess["_a53_freq"].as<unsigned int>())
-                : std::nullopt;
-            auto _a72_freq =
-              yprocess["_a72_freq"]
-                ? std::optional(1000 * 1000 * yprocess["_a72_freq"].as<unsigned int>())
-                : std::nullopt;
+            auto _a53_freq = yprocess["_a53_freq"]
+                               ? std::optional(CpuFrequencyHz{
+                                   1000 * 1000 * yprocess["_a53_freq"].as<unsigned int>() })
+                               : std::nullopt;
+            auto _a72_freq = yprocess["_a72_freq"]
+                               ? std::optional(CpuFrequencyHz{
+                                   1000 * 1000 * yprocess["_a72_freq"].as<unsigned int>() })
+                               : std::nullopt;
             partitions.back().add_process(c.loop,
                                           yprocess["cmd"].as<string>(),
                                           process_cwd,

src/cpufreq_policy.hpp

@@ -19,7 +19,27 @@ using std::runtime_error;
 using std::string;
 
 /** CPU core frequency, in Hz. */
-using CpuFrequencyHz = uint64_t;
+// using CpuFrequencyHz = uint64_t;
+struct CpuFrequencyHz
+{
+    uint64_t freq;
+    explicit CpuFrequencyHz(uint64_t freq)
+        : freq{ freq }
+    {}
+    bool operator==(const CpuFrequencyHz &rhs) const { return freq == rhs.freq; }
+    operator uint64_t() const { return freq; } // NOLINT(google-explicit-constructor)
+};
+
+template<>
+struct fmt::formatter<CpuFrequencyHz> : fmt::formatter<uint64_t>
+{
+    template<typename FormatContext>
+    auto format(CpuFrequencyHz const &freq, FormatContext &ctx)
+    {
+        return fmt::format_to(
+          ctx.out(), "{}{} MHz", freq.freq % 1000000 == 0 ? "" : "~", freq.freq / 1000000);
+    }
+};
 
 /**
  * NOTE: This class assumes that `cpufreq` is supported and write-able,
@@ -62,6 +82,8 @@ class CpufreqPolicy
         , max_frequency{ read_freq_file(policy_dir / "scaling_max_freq") }
         , available_frequencies{ read_available_frequencies() }
         , affected_cores{ read_affected_cpus() }
+        // sometimes, the policy does not affect any cores (e.g. all controlled cores are offline);
+        //  attempting to set the frequency would then cause an IO error
         , active{ affected_cores.count() > 0 }
     {
         if (!active) {
@@ -91,8 +113,8 @@ class CpufreqPolicy
                       "available: '{}')",
                       affected_cores.as_list(),
                       name,
-                      freq_to_str(min_frequency),
-                      freq_to_str(max_frequency),
+                      min_frequency,
+                      max_frequency,
                       get_available_freq_str());
     }
 
@@ -120,7 +142,7 @@ class CpufreqPolicy
         // this check is only called in debug builds, because cpufreq does its own checking
         RUN_DEBUG(validate_frequency(freq));
 
-        TRACE("Changing CPU frequency to '{}' for '{}'", freq_to_str(freq), name);
+        TRACE("Changing CPU frequency to '{}' for '{}'", freq, name);
         // cpufreq uses kHz, we have Hz
         auto freq_str = std::to_string(freq / 1000);
         CHECK_MSG(write(fd_freq, freq_str.c_str(), freq_str.size()),
@@ -137,11 +159,10 @@ class CpufreqPolicy
               "provide it, most notably the `intel_pstate` driver used on Intel processors)");
         }
         if (index >= available_frequencies->size()) {
-            throw runtime_error("Tried to get an out-of-bounds frequency #'" +
-                                std::to_string(index) + "' for `cpufreq` policy '" + name +
-                                "' (there are only '" +
-                                std::to_string(available_frequencies->size()) +
-                                "' defined frequencies)");
+            throw runtime_error(
+              "Tried to get an out-of-bounds frequency #'" + std::to_string(index) +
+              "' for `cpufreq` policy '" + name + "' (there are only '" +
+              std::to_string(available_frequencies->size()) + "' defined frequencies)");
         }
         return available_frequencies->at(index);
     }
@@ -153,23 +174,12 @@ class CpufreqPolicy
     }
 
 private: ///////////////////////////////////////////////////////////////////////////////////////////
-    static string freq_to_str(CpuFrequencyHz freq)
-    {
-        return std::to_string(freq / 1000000) + " MHz";
-    }
-
     string get_available_freq_str()
     {
         if (!available_frequencies) {
             return "unknown";
         }
-        string str;
-        for (CpuFrequencyHz freq : available_frequencies.value()) {
-            str += freq_to_str(freq) + ", ";
-        }
-        // remove trailing ", "
-        str.resize(str.size() - 2);
-        return str;
+        return fmt::format("{}", fmt::join(available_frequencies.value(), ", "));
     }
 
     void validate_frequency(CpuFrequencyHz freq)
@@ -233,10 +243,10 @@ class CpufreqPolicy
         }
 
         std::vector<CpuFrequencyHz> freqs;
-        CpuFrequencyHz freq_khz;
+        uint64_t freq_khz;
         while (is_freq >> freq_khz) {
             // convert from kHz (used by cpufreq) to Hz
-            freqs.push_back(freq_khz * 1000);
+            freqs.emplace_back(freq_khz * 1000);
         }
         ASSERT(is_freq.eof());
         return std::make_optional(freqs);
@@ -257,9 +267,9 @@ class CpufreqPolicy
     static CpuFrequencyHz read_freq_file(const fs::path &path)
     {
         auto is = file_open<std::ifstream>(path);
-        CpuFrequencyHz freq;
+        uint64_t freq;
         is >> freq;
         // cpufreq uses kHz, we want Hz
-        return freq * 1000;
+        return CpuFrequencyHz{ freq * 1000 };
     }
 };

src/partition.cpp

@@ -27,8 +27,8 @@ void Partition::add_process(ev::loop_ref loop,
                             const optional<filesystem::path> &working_dir,
                             chrono::milliseconds budget,
                             chrono::milliseconds budget_jitter,
-                            std::optional<unsigned int> a53_freq,
-                            std::optional<unsigned int> a72_freq,
+                            std::optional<CpuFrequencyHz> a53_freq,
+                            std::optional<CpuFrequencyHz> a72_freq,
                             bool has_initialization)
 {
     processes.emplace_back(loop,

src/partition.hpp

@@ -73,8 +73,8 @@ class Partition
                      const std::optional<std::filesystem::path> &working_dir,
                      std::chrono::milliseconds budget,
                      std::chrono::milliseconds budget_jitter,
-                     std::optional<unsigned int> a53_freq,
-                     std::optional<unsigned int> a72_freq,
+                     std::optional<CpuFrequencyHz> a53_freq,
+                     std::optional<CpuFrequencyHz> a72_freq,
                      bool has_initialization);
 
     /** Spawns system processes for all added Process instances. */

src/power_policy/imx8_alternating.hpp

@@ -13,17 +13,23 @@ class PowerPolicy_Imx8_Alternating : public PmPowerPolicy
 private:
     CpufreqPolicy &a53_pol = pm.get_policy("policy0");
     CpufreqPolicy &a72_pol = pm.get_policy("policy4");
-    CpuFrequencyHz f1_a53 = 0;
-    CpuFrequencyHz f2_a53 = 0;
-    CpuFrequencyHz f1_a72 = 0;
-    CpuFrequencyHz f2_a72 = 0;
+    CpuFrequencyHz f1_a53;
+    CpuFrequencyHz f2_a53;
+    CpuFrequencyHz f1_a72;
+    CpuFrequencyHz f2_a72;
     bool f1_next = true;
 
 public:
     PowerPolicy_Imx8_Alternating(const std::string &a53_f1_str,
                                  const std::string &a53_f2_str,
                                  const std::string &a72_f1_str,
                                  const std::string &a72_f2_str)
+    try
+      : PmPowerPolicy{}
+      , f1_a53{ a53_pol.get_freq(std::stoul(a53_f1_str)) }
+      , f2_a53{ a53_pol.get_freq(std::stoul(a53_f2_str)) }
+      , f1_a72{ a72_pol.get_freq(std::stoul(a72_f1_str)) }
+      , f2_a72{ a72_pol.get_freq(std::stoul(a72_f2_str)) }
     {
         for (auto &p : pm.policy_iter()) {
             if (!p.available_frequencies) {
@@ -32,20 +38,12 @@ class PowerPolicy_Imx8_Alternating : public PmPowerPolicy
             }
         }
 
-        try {
-            f1_a53 = a53_pol.get_freq(std::stoul(a53_f1_str));
-            f2_a53 = a53_pol.get_freq(std::stoul(a53_f2_str));
-
-            f1_a72 = a72_pol.get_freq(std::stoul(a72_f1_str));
-            f2_a72 = a72_pol.get_freq(std::stoul(a72_f2_str));
-        } catch (...) {
-            throw_with_nested(
-              runtime_error("All power policy arguments must be integers in the range <0, 3>"));
-        }
-
         // set fixed frequency if both "alternating" frequencies are the same
         if (f1_a53 == f2_a53) a53_pol.write_frequency(f1_a53);
         if (f1_a72 == f2_a72) a72_pol.write_frequency(f1_a72);
+    } catch (...) {
+        throw_with_nested(
+          runtime_error("All power policy arguments must be integers in the range <0, 3>"));
     }
 
     void on_window_start(Window &) override

src/power_policy/imx8_fixed.hpp

@@ -13,7 +13,7 @@ class PowerPolicy_Imx8_Fixed : public PmPowerPolicy
 {
 public:
     /** *_freq_i - integer, range <0,3> */
-    PowerPolicy_Imx8_Fixed(const std::string& a53_freq_i, const std::string& a72_freq_i)
+    PowerPolicy_Imx8_Fixed(const std::string &a53_freq_i, const std::string &a72_freq_i)
     {
         auto &a53_pol = pm.get_policy("policy0");
         auto &a72_pol = pm.get_policy("policy4");
@@ -25,15 +25,12 @@ class PowerPolicy_Imx8_Fixed : public PmPowerPolicy
             }
         }
 
-        CpuFrequencyHz a53_freq, a72_freq;
         try {
-            a53_freq = a53_pol.get_freq(std::stoul(a53_freq_i));
-            a72_freq = a72_pol.get_freq(std::stoul(a72_freq_i));
-        } catch (...) {
-            throw runtime_error("Both power policy arguments must be integers in the range <0, 3>");
+            a53_pol.write_frequency_i(std::stoul(a53_freq_i));
+            a72_pol.write_frequency_i(std::stoul(a72_freq_i));
+        } catch (std::invalid_argument &) {
+            std::throw_with_nested(std::runtime_error(
+              "Both power policy arguments must be integers in the range <0, 3>"));
         }
-
-        a53_pol.write_frequency(a53_freq);
-        a72_pol.write_frequency(a72_freq);
     }
 };

src/process.cpp

@@ -23,14 +23,15 @@ Process::Process(ev::loop_ref loop,
                  std::optional<std::filesystem::path> working_dir,
                  milliseconds budget,
                  milliseconds budget_jitter,
-                 std::optional<unsigned int> a53_freq,
-                 std::optional<unsigned int> a72_freq,
+                 std::optional<CpuFrequencyHz> a53_freq,
+                 std::optional<CpuFrequencyHz> a72_freq,
                  bool has_initialization)
     : part(partition)
     , a53_freq{ a53_freq }
     , a72_freq{ a72_freq }
     // budget +- (jitter / 2)
-    , jitter_distribution_ms(-budget_jitter.count() / 2, budget_jitter.count() - budget_jitter.count() / 2)
+    , jitter_distribution_ms(-budget_jitter.count() / 2,
+                             budget_jitter.count() - budget_jitter.count() / 2)
     , loop(loop)
     , efd_continue(CHECK(eventfd(0, EFD_SEMAPHORE)))
     , cge(loop,
@@ -77,7 +78,8 @@ void Process::exec()
         // END CHILD PROCESS
     } else {
         // PARENT PROCESS
-        logger_process->debug("Running '{}' as PID '{}' (partition '{}')", argv, pid, part.get_name());
+        logger_process->debug(
+          "Running '{}' as PID '{}' (partition '{}')", argv, pid, part.get_name());
         child_w.start(pid, 0);
         // TODO: shouldn't we do this in the child process, so that we know the process
         //  is frozen before we start the command? as it is now, I think there's a possible
@@ -165,7 +167,8 @@ void Process::set_remaining_budget(milliseconds next_budget)
     ASSERT(next_budget > next_budget.zero());
     ASSERT(next_budget < budget);
     actual_budget = next_budget;
-    TRACE_PROCESS("Next budget for process '{}' shortened to '{} milliseconds'.", pid, next_budget.count());
+    TRACE_PROCESS(
+      "Next budget for process '{}' shortened to '{} milliseconds'.", pid, next_budget.count());
 }
 
 void Process::reset_budget()

src/process.hpp

@@ -11,6 +11,7 @@
 #include "cgroup.hpp"
 #include "evfd.hpp"
 #include "timerfd.hpp"
+#include "cpufreq_policy.hpp"
 
 class Partition;
 
@@ -32,8 +33,8 @@ class Process
             std::optional<std::filesystem::path> working_dir,
             std::chrono::milliseconds budget,
             std::chrono::milliseconds budget_jitter,
-            std::optional<unsigned int> a53_freq,
-            std::optional<unsigned int> a72_freq,
+            std::optional<CpuFrequencyHz> a53_freq,
+            std::optional<CpuFrequencyHz> a72_freq,
             bool has_initialization = false);
 
     /** Spawns the underlying system process. */
@@ -79,8 +80,8 @@ class Process
     void mark_uncompleted();
 
     Partition &part;
-    const std::optional<unsigned int> a53_freq;
-    const std::optional<unsigned int> a72_freq;
+    const std::optional<CpuFrequencyHz> a53_freq;
+    const std::optional<CpuFrequencyHz> a72_freq;
 
 private:
     std::uniform_int_distribution<long> jitter_distribution_ms;