| Message ID | 20240429234152.16230-5-ricardo.neri-calderon@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | intel: thermal: hfi: Add debugfs files for tuning | expand |
On Mon, 2024-04-29 at 16:41 -0700, Ricardo Neri wrote: > The number of updated CPU capabilities per netlink event is hard- > coded to > 16. On systems with more than 16 it takes more than one thermal > netlink > event to relay all the new capabilities when processing an HFI > interrupt. > This adds unnecessary overhead. > > Make the number of updated capabilities per event tuneable via > debugfs. > Users can then experiment with different values. > Is there a limitation about the number of CPUs supported in one netlink event? IMO, we still have to use a fixed number here because debugfs can be changed by someone else, and userspace application like intel-lpmd cannot make assumption that the netlink message follows what it set. or can we append one magic item in the end of one update? userspace can just check the magic item no matter the number of CPU per netlink event. thanks, rui > We already take the hfi_instance_lock when submitting thermal netlink > updates. Use it to serialize debugfs accesses to > hfi_therm_notify_count. > > Suggested-by: Srinivas Pandruvada > <srinivas.pandruvada@linux.intel.com> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> > --- > Cc: Len Brown <len.brown@intel.com> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> > Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > Cc: Zhang Rui <rui.zhang@intel.com> > Cc: linux-pm@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > drivers/thermal/intel/intel_hfi.c | 34 ++++++++++++++++++++++++++--- > -- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/thermal/intel/intel_hfi.c > b/drivers/thermal/intel/intel_hfi.c > index d6d3544509fc..d5163b9766c0 100644 > --- a/drivers/thermal/intel/intel_hfi.c > +++ b/drivers/thermal/intel/intel_hfi.c > @@ -175,6 +175,7 @@ static struct workqueue_struct *hfi_updates_wq; > > /* Keep this variable 8-byte aligned to get atomic accesses. */ > static unsigned long hfi_update_delay = HFI_UPDATE_DELAY; > +static int hfi_thermnl_caps_per_event = HFI_THERMNL_CAPS_PER_EVENT; > > #ifdef CONFIG_DEBUG_FS > static int hfi_update_delay_get(void *data, u64 *val) > @@ -205,6 +206,25 @@ static int hfi_update_delay_set(void *data, u64 > val) > DEFINE_DEBUGFS_ATTRIBUTE(hfi_update_delay_fops, > hfi_update_delay_get, > hfi_update_delay_set, "%llu\n"); > > +static int hfi_thermnl_caps_per_event_get(void *data, u64 *val) > +{ > + mutex_lock(&hfi_instance_lock); > + *val = hfi_thermnl_caps_per_event; > + mutex_unlock(&hfi_instance_lock); > + return 0; > +} > + > +static int hfi_thermnl_caps_per_event_set(void *data, u64 val) > +{ > + mutex_lock(&hfi_instance_lock); > + hfi_thermnl_caps_per_event = val; > + mutex_unlock(&hfi_instance_lock); > + return 0; > +} > + > +DEFINE_DEBUGFS_ATTRIBUTE(hfi_thermnl_caps_per_event_fops, > + hfi_thermnl_caps_per_event_get, > + hfi_thermnl_caps_per_event_set, "%llu\n"); > static struct dentry *hfi_debugfs_dir; > > static void hfi_debugfs_unregister(void) > @@ -226,6 +246,11 @@ static void hfi_debugfs_register(void) > if (!f) > goto err; > > + f = debugfs_create_file("thermnl_caps_per_event", 0644, > hfi_debugfs_dir, > + NULL, > &hfi_thermnl_caps_per_event_fops); > + if (!f) > + goto err; > + > return; > err: > hfi_debugfs_unregister(); > @@ -286,16 +311,15 @@ static void update_capabilities(struct > hfi_instance *hfi_instance) > > get_hfi_caps(hfi_instance, cpu_caps); > > - if (cpu_count < HFI_THERMNL_CAPS_PER_EVENT) > + if (cpu_count < hfi_thermnl_caps_per_event) > goto last_cmd; > > /* Process complete chunks of HFI_THERMNL_CAPS_PER_EVENT > capabilities. */ > for (i = 0; > - (i + HFI_THERMNL_CAPS_PER_EVENT) <= cpu_count; > - i += HFI_THERMNL_CAPS_PER_EVENT) > - > thermal_genl_cpu_capability_event(HFI_THERMNL_CAPS_PER_ > EVENT, > + (i + hfi_thermnl_caps_per_event) <= cpu_count; > + i += hfi_thermnl_caps_per_event) > + thermal_genl_cpu_capability_event(hfi_thermnl_caps_pe > r_event, > &cpu_caps[i]); > - > cpu_count = cpu_count - i; > > last_cmd:
On Tue, Apr 30, 2024 at 05:07:54AM +0000, Zhang, Rui wrote: > On Mon, 2024-04-29 at 16:41 -0700, Ricardo Neri wrote: > > The number of updated CPU capabilities per netlink event is hard- > > coded to > > 16. On systems with more than 16 it takes more than one thermal > > netlink > > event to relay all the new capabilities when processing an HFI > > interrupt. > > This adds unnecessary overhead. > > > > Make the number of updated capabilities per event tuneable via > > debugfs. > > Users can then experiment with different values. > > > Is there a limitation about the number of CPUs supported in one netlink > event? IIUC, the only limit is the size of the buffer for the message. intel_hfi allocates the message based on the number of CPUs of the HFI instance. > > IMO, we still have to use a fixed number here because debugfs can be > changed by someone else, and userspace application like intel-lpmd > cannot make assumption that the netlink message follows what it set. but you don't know how many messages with 16-CPUs payload you will receive for a single update, no? Yes, you can infer it from the number of online CPUs, but still. But yes, now lpmd would receive an unknown number of messages with payloads of unknown size. > > or can we append one magic item in the end of one update? > userspace can just check the magic item no matter the number of CPU per > netlink event. AFAIK, only HFI and lmpd use the CPU capabilities thermal netlink message. I guess it could be done.
diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c index d6d3544509fc..d5163b9766c0 100644 --- a/drivers/thermal/intel/intel_hfi.c +++ b/drivers/thermal/intel/intel_hfi.c @@ -175,6 +175,7 @@ static struct workqueue_struct *hfi_updates_wq; /* Keep this variable 8-byte aligned to get atomic accesses. */ static unsigned long hfi_update_delay = HFI_UPDATE_DELAY; +static int hfi_thermnl_caps_per_event = HFI_THERMNL_CAPS_PER_EVENT; #ifdef CONFIG_DEBUG_FS static int hfi_update_delay_get(void *data, u64 *val) @@ -205,6 +206,25 @@ static int hfi_update_delay_set(void *data, u64 val) DEFINE_DEBUGFS_ATTRIBUTE(hfi_update_delay_fops, hfi_update_delay_get, hfi_update_delay_set, "%llu\n"); +static int hfi_thermnl_caps_per_event_get(void *data, u64 *val) +{ + mutex_lock(&hfi_instance_lock); + *val = hfi_thermnl_caps_per_event; + mutex_unlock(&hfi_instance_lock); + return 0; +} + +static int hfi_thermnl_caps_per_event_set(void *data, u64 val) +{ + mutex_lock(&hfi_instance_lock); + hfi_thermnl_caps_per_event = val; + mutex_unlock(&hfi_instance_lock); + return 0; +} + +DEFINE_DEBUGFS_ATTRIBUTE(hfi_thermnl_caps_per_event_fops, + hfi_thermnl_caps_per_event_get, + hfi_thermnl_caps_per_event_set, "%llu\n"); static struct dentry *hfi_debugfs_dir; static void hfi_debugfs_unregister(void) @@ -226,6 +246,11 @@ static void hfi_debugfs_register(void) if (!f) goto err; + f = debugfs_create_file("thermnl_caps_per_event", 0644, hfi_debugfs_dir, + NULL, &hfi_thermnl_caps_per_event_fops); + if (!f) + goto err; + return; err: hfi_debugfs_unregister(); @@ -286,16 +311,15 @@ static void update_capabilities(struct hfi_instance *hfi_instance) get_hfi_caps(hfi_instance, cpu_caps); - if (cpu_count < HFI_THERMNL_CAPS_PER_EVENT) + if (cpu_count < hfi_thermnl_caps_per_event) goto last_cmd; /* Process complete chunks of HFI_THERMNL_CAPS_PER_EVENT capabilities. */ for (i = 0; - (i + HFI_THERMNL_CAPS_PER_EVENT) <= cpu_count; - i += HFI_THERMNL_CAPS_PER_EVENT) - thermal_genl_cpu_capability_event(HFI_THERMNL_CAPS_PER_EVENT, + (i + hfi_thermnl_caps_per_event) <= cpu_count; + i += hfi_thermnl_caps_per_event) + thermal_genl_cpu_capability_event(hfi_thermnl_caps_per_event, &cpu_caps[i]); - cpu_count = cpu_count - i; last_cmd:
The number of updated CPU capabilities per netlink event is hard-coded to 16. On systems with more than 16 it takes more than one thermal netlink event to relay all the new capabilities when processing an HFI interrupt. This adds unnecessary overhead. Make the number of updated capabilities per event tuneable via debugfs. Users can then experiment with different values. We already take the hfi_instance_lock when submitting thermal netlink updates. Use it to serialize debugfs accesses to hfi_therm_notify_count. Suggested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> --- Cc: Len Brown <len.brown@intel.com> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> Cc: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> Cc: Zhang Rui <rui.zhang@intel.com> Cc: linux-pm@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- drivers/thermal/intel/intel_hfi.c | 34 ++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)