Messages in this thread | | | Date | Thu, 6 Jul 2023 16:23:51 -0700 | From | Ricardo Neri <> | Subject | Re: [PATCH v4 14/24] thermal: intel: hfi: Store per-CPU IPCC scores |
| |
On Thu, Jun 29, 2023 at 08:53:31PM +0200, Rafael J. Wysocki wrote: > On Tue, Jun 13, 2023 at 6:23 AM Ricardo Neri > <ricardo.neri-calderon@linux.intel.com> wrote: > > > > The scheduler reads the IPCC scores when balancing load. These reads can > > occur frequently and originate from many CPUs. Hardware may also > > occasionally update the HFI table. Controlling access with locks would > > cause contention. > > > > Cache the IPCC scores in separate per-CPU variables that the scheduler can > > use. Use a seqcount to synchronize memory accesses to these cached values. > > This eliminates the need for locks, as the sequence counter provides the > > memory ordering required to prevent the use of stale data. > > > > The HFI delayed workqueue guarantees that only one CPU writes the cached > > IPCC scores. The frequency of updates is low (every CONFIG_HZ jiffies or > > less), and the number of writes per update is in the order of tens. Writes > > should not starve reads. > > > > Only cache IPCC scores in this changeset. A subsequent changeset will > > use these scores. > > > > 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: Perry Yuan <Perry.Yuan@amd.com> > > 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: Zhao Liu <zhao1.liu@linux.intel.com> > > Cc: x86@kernel.org > > Cc: linux-pm@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org > > Suggested-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > > --- > > Changes since v3: > > * As Rafael requested, I reworked the memory ordering of the cached IPCC > > scores. I selected a seqcount as is less expensive than a memory > > barrier, which is not necessary anyways. > > * Made alloc_hfi_ipcc_scores() return -ENOMEM on allocation failure. > > (Rafael) > > * Added a comment to describe hfi_ipcc_scores. (Rafael) > > > > Changes since v2: > > * Only create these per-CPU variables when Intel Thread Director is > > supported. > > > > Changes since v1: > > * Added this patch. > > --- > > drivers/thermal/intel/intel_hfi.c | 66 +++++++++++++++++++++++++++++++ > > 1 file changed, 66 insertions(+) > > > > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c > > index 20ee4264dcd4..d822ed0bb5c1 100644 > > --- a/drivers/thermal/intel/intel_hfi.c > > +++ b/drivers/thermal/intel/intel_hfi.c > > @@ -29,9 +29,11 @@ > > #include <linux/kernel.h> > > #include <linux/math.h> > > #include <linux/mutex.h> > > +#include <linux/percpu.h> > > #include <linux/percpu-defs.h> > > #include <linux/printk.h> > > #include <linux/processor.h> > > +#include <linux/seqlock.h> > > #include <linux/slab.h> > > #include <linux/spinlock.h> > > #include <linux/string.h> > > @@ -180,6 +182,62 @@ static struct workqueue_struct *hfi_updates_wq; > > #define HFI_UPDATE_INTERVAL HZ > > #define HFI_MAX_THERM_NOTIFY_COUNT 16 > > > > +/* A cache of the HFI perf capabilities for lockless access. */ > > +static int __percpu *hfi_ipcc_scores; > > +/* Sequence counter for hfi_ipcc_scores */ > > +static seqcount_t hfi_ipcc_seqcount = SEQCNT_ZERO(hfi_ipcc_seqcount); > > + > > +static int alloc_hfi_ipcc_scores(void) > > +{ > > + if (!cpu_feature_enabled(X86_FEATURE_ITD)) > > + return 0; > > + > > + hfi_ipcc_scores = __alloc_percpu(sizeof(*hfi_ipcc_scores) * > > + hfi_features.nr_classes, > > + sizeof(*hfi_ipcc_scores)); > > + > > + return hfi_ipcc_scores ? 0 : -ENOMEM; > > +} > > This doesn't need to return an int. It could be a bool function > returning !!hfi_ipcc_scores (or false for the feature missing case).
Sure Rafael, I can make this change.
> > Apart from this minor thing, the patch looks reasonable to me.
Thank you for your review!
| |