Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Mon, 27 Mar 2023 19:03:08 +0200 | Subject | Re: [PATCH v3 21/24] thermal: intel: hfi: Implement model-specific checks for task classification |
| |
On Tue, Feb 7, 2023 at 6:02 AM Ricardo Neri <ricardo.neri-calderon@linux.intel.com> wrote: > > In Alder Lake and Raptor Lake, the result of thread classification is more > accurate when only one SMT sibling is busy. Classification results for > class 2 and 3 are always reliable. > > To avoid unnecessary migrations, only update the class of a task if it has > been the same during 4 consecutive user ticks. > > Cc: Ben Segall <bsegall@google.com> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com> > Cc: Ionela Voinescu <ionela.voinescu@arm.com> > Cc: Joel Fernandes (Google) <joel@joelfernandes.org> > Cc: Len Brown <len.brown@intel.com> > Cc: Lukasz Luba <lukasz.luba@arm.com> > Cc: Mel Gorman <mgorman@suse.de> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Tim C. Chen <tim.c.chen@intel.com> > Cc: Valentin Schneider <vschneid@redhat.com> > Cc: x86@kernel.org > Cc: linux-pm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > --- > Changes since v2: > * None > > Changes since v1: > * Adjusted the result the classification of Intel Thread Director to start > at class 1. Class 0 for the scheduler means that the task is > unclassified. > * Used the new names of the IPC classes members in task_struct. > * Reworked helper functions to use sched_smt_siblings_idle() to query > the idle state of the SMT siblings of a CPU. > --- > drivers/thermal/intel/intel_hfi.c | 60 ++++++++++++++++++++++++++++++- > 1 file changed, 59 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c > index 35d947f47550..fdb53e4cabc1 100644 > --- a/drivers/thermal/intel/intel_hfi.c > +++ b/drivers/thermal/intel/intel_hfi.c > @@ -40,6 +40,7 @@ > #include <linux/workqueue.h> > > #include <asm/msr.h> > +#include <asm/intel-family.h> > > #include "../thermal_core.h" > #include "intel_hfi.h" > @@ -209,9 +210,64 @@ static int __percpu *hfi_ipcc_scores; > */ > #define HFI_UNCLASSIFIED_DEFAULT 1 > > +#define CLASS_DEBOUNCER_SKIPS 4 > + > +/** > + * debounce_and_update_class() - Process and update a task's classification > + * > + * @p: The task of which the classification will be updated > + * @new_ipcc: The new IPC classification > + * > + * Update the classification of @p with the new value that hardware provides. > + * Only update the classification of @p if it has been the same during > + * CLASS_DEBOUNCER_SKIPS consecutive ticks. > + */ > +static void debounce_and_update_class(struct task_struct *p, u8 new_ipcc) > +{ > + u16 debounce_skip; > + > + /* The class of @p changed. Only restart the debounce counter. */ > + if (p->ipcc_tmp != new_ipcc) { > + p->ipcc_cntr = 1; > + goto out; > + } > + > + /* > + * The class of @p did not change. Update it if it has been the same > + * for CLASS_DEBOUNCER_SKIPS user ticks. > + */ > + debounce_skip = p->ipcc_cntr + 1; > + if (debounce_skip < CLASS_DEBOUNCER_SKIPS) > + p->ipcc_cntr++; > + else > + p->ipcc = new_ipcc; > + > +out: > + p->ipcc_tmp = new_ipcc; > +}
Why does the code above belong to the Intel HFI driver? It doesn't look like there is anything driver-specific in it.
> + > +static bool classification_is_accurate(u8 hfi_class, bool smt_siblings_idle) > +{ > + switch (boot_cpu_data.x86_model) { > + case INTEL_FAM6_ALDERLAKE: > + case INTEL_FAM6_ALDERLAKE_L: > + case INTEL_FAM6_RAPTORLAKE: > + case INTEL_FAM6_RAPTORLAKE_P: > + case INTEL_FAM6_RAPTORLAKE_S: > + if (hfi_class == 3 || hfi_class == 2 || smt_siblings_idle) > + return true; > + > + return false; > + > + default: > + return true; > + } > +} > + > void intel_hfi_update_ipcc(struct task_struct *curr) > { > union hfi_thread_feedback_char_msr msr; > + bool idle; > > /* We should not be here if ITD is not supported. */ > if (!cpu_feature_enabled(X86_FEATURE_ITD)) { > @@ -227,7 +283,9 @@ void intel_hfi_update_ipcc(struct task_struct *curr) > * 0 is a valid classification for Intel Thread Director. A scheduler > * IPCC class of 0 means that the task is unclassified. Adjust. > */ > - curr->ipcc = msr.split.classid + 1; > + idle = sched_smt_siblings_idle(task_cpu(curr)); > + if (classification_is_accurate(msr.split.classid, idle)) > + debounce_and_update_class(curr, msr.split.classid + 1); > }
I still think that this function should just return a number, possibly including a special "no IPCC" value. It may be passed a bool argument indicating whether or not the SMT siblings are idle.
> > unsigned long intel_hfi_get_ipcc_score(unsigned short ipcc, int cpu) > --
| |