On Sat, Jan 8, 2022 at 4:46 AM Ricardo Neri
<ricardo.neri-calderon@xxxxxxxxxxxxxxx> wrote:
>
> From: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
>
> When the hardware issues an HFI event, relay a notification to user space.
> This allows user space to respond by reading performance and efficiency of
> each CPU and take appropriate action.
>
> For example, when performance and efficiency of a CPU is 0, user space can
> either offline the CPU or inject idle. Also, if user space notices a
> downward trend in performance, it may proactively adjust power limits to
> avoid future situations in which performance drops to 0.
>
> To avoid excessive notifications, the rate is limited by one HZ per event.
> To limit the netlink message size, parameters for only 16 CPUs at max are
> sent in one message. If there are more than 16 CPUs, issue as many messages
> as needed to notify the status of all CPUs.
>
> In the HFI specification, both performance and efficiency capabilities are
> set in the [0, 255] range. The existing implementations of HFI hardware
> do not scale the maximum values to 255. Since userspace cares about
> capability values that are either 0 or show a downward/upward trend, this
> fact does not matter much. Relative changes in capabilities are enough. To
> comply with the thermal netlink ABI, scale both performance and efficiency
> capabilities to the [0, 1023] interval.
>
> Cc: Andi Kleen <ak@xxxxxxxxxxxxxxx>
> Cc: Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx>
> Cc: Lukasz Luba <lukasz.luba@xxxxxxx>
> Cc: Tim Chen <tim.c.chen@xxxxxxxxxxxxxxx>
> Cc: "Ravi V. Shankar" <ravi.v.shankar@xxxxxxxxx>
> Reviewed-by: Len Brown <len.brown@xxxxxxxxx>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@xxxxxxxxxxxxxxx>
> ---
> Changes since v3:
> * None
>
> Changes since v2:
> * None
>
> Changes since v1:
> * Made get_one_hfi_cap() return void. Removed unnecessary checks.
> (Rafael)
> * Replaced raw_spin_[un]lock_irq[restore|save]() with raw_spin_
> [un]lock_irq() in get_one_hfi_cap(). This function is only called from
> a workqueue and there is no need to save and restore irq flags.
> * Scaled performance and energy efficiency values to a [0, 1023] interval
> when reporting values to user space via thermal netlink notifications.
> (Lucasz).
> * Reworded commit message to comment on the scaling of HFI capabilities
> to comply with the proposed thermal netlink ABI.
> ---
> drivers/thermal/intel/Kconfig | 1 +
> drivers/thermal/intel/intel_hfi.c | 57 ++++++++++++++++++++++++++++++-
> 2 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> index e9d2925227d4..6cf3fe36a4ae 100644
> --- a/drivers/thermal/intel/Kconfig
> +++ b/drivers/thermal/intel/Kconfig
> @@ -104,6 +104,7 @@ config INTEL_HFI_THERMAL
> bool "Intel Hardware Feedback Interface"
> depends on CPU_SUP_INTEL
> depends on X86_THERMAL_VECTOR
> + select THERMAL_NETLINK
> help
> Select this option to enable the Hardware Feedback Interface. If
> selected, hardware provides guidance to the operating system on
> diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> index 1a08c58f26f6..9fd66f176948 100644
> --- a/drivers/thermal/intel/intel_hfi.c
> +++ b/drivers/thermal/intel/intel_hfi.c
> @@ -40,6 +40,7 @@
>
> #include <asm/msr.h>
>
> +#include "../thermal_core.h"
> #include "intel_hfi.h"
>
> #define THERM_STATUS_CLEAR_PKG_MASK (BIT(1) | BIT(3) | BIT(5) | BIT(7) | \
> @@ -162,6 +163,60 @@ static struct hfi_features hfi_features;
> static DEFINE_MUTEX(hfi_instance_lock);
>
> #define HFI_UPDATE_INTERVAL HZ
> +#define HFI_MAX_THERM_NOTIFY_COUNT 16
> +
> +static void get_one_hfi_cap(struct hfi_instance *hfi_instance, s16 index,
> + struct hfi_cpu_data *hfi_caps)
> +{
> + struct hfi_cpu_data *caps;
> +
> + /* Find the capabilities of @cpu */
> + raw_spin_lock_irq(&hfi_instance->table_lock);
> + caps = hfi_instance->data + index * hfi_features.cpu_stride;
> + memcpy(hfi_caps, caps, sizeof(*hfi_caps));
> + raw_spin_unlock_irq(&hfi_instance->table_lock);
> +}
> +
> +/*
> + * Call update_capabilities() when there are changes in the HFI table.
> + */
> +static void update_capabilities(struct hfi_instance *hfi_instance)
> +{
> + struct cpu_capability cpu_caps[HFI_MAX_THERM_NOTIFY_COUNT];
> + int i = 0, cpu;
> +
Wouldn't it be better to hold hfi_instance_lock for the duration of this loop?
Surely, CPU offline or online during it can be confusing.
> + for_each_cpu(cpu, hfi_instance->cpus) {
> + struct hfi_cpu_data caps;
> + s16 index;
> +
> + /*
> + * We know index is valid because this CPU is present
> + * in this instance.
> + */
> + index = per_cpu(hfi_cpu_info, cpu).index;
> +
> + get_one_hfi_cap(hfi_instance, index, &caps);
> +
> + cpu_caps[i].cpu = cpu;
> +
> + /*
> + * Scale performance and energy efficiency to
> + * the [0, 1023] interval that thermal netlink uses.
> + */
> + cpu_caps[i].performance = caps.perf_cap << 2;
> + cpu_caps[i].efficiency = caps.ee_cap << 2;
> + ++i;
> +
> + if (i >= HFI_MAX_THERM_NOTIFY_COUNT) {
> + thermal_genl_cpu_capability_event(HFI_MAX_THERM_NOTIFY_COUNT,
> + cpu_caps);
> + i = 0;
> + }
> + }
> +
> + if (i)
> + thermal_genl_cpu_capability_event(i, cpu_caps);
> +}
>
> static void hfi_update_work_fn(struct work_struct *work)
> {
> @@ -172,7 +227,7 @@ static void hfi_update_work_fn(struct work_struct *work)
> if (!hfi_instance)
> return;
>
> - /* TODO: Consume update here. */
> + update_capabilities(hfi_instance);
> }
>
> void intel_hfi_process_event(__u64 pkg_therm_status_msr_val)
> --
> 2.17.1
>
