Messages in this thread | | | From | "Zhang, Rui" <> | Subject | Re: [PATCH 2/4] thermal: intel: hfi: Tune the HFI thermal netlink event delay via debugfs | Date | Tue, 30 Apr 2024 04:52:52 +0000 |
| |
On Mon, 2024-04-29 at 16:41 -0700, Ricardo Neri wrote: > THe delay between an HFI interrupt and its corresponding thermal
s/THe/The
> netlink > event has so far been hard-coded to CONFIG_HZ jiffies. This may not > suit > the needs of all hardware configurations or listeners of events. > > If the update delay is too long, much of the information of > consecutive > hardware updates will be lost as the HFI delayed workqueue posts a > new > thermal netlink event only when there are no previous pending events. > If > the delay is too short, multiple, events may overwhelm listeners. > > Listeners are better placed to determine the delay of events. They > know how > quickly they can act on them effectively. They may also want to > experiment > with different values. > > Start a debugfs interface to tune the notification delay.
Why this is implemented as debugfs rather than a module param?
thanks, rui > > Keep things simple and do not add extra locking or memory barriers. > This > may result in the HFI interrupt ocassionally queueing work using > stale > delay values, if at all. This should not be a problem: the debugfs > file > will update the delay value atomically, we do not expect users to > update > the delay value frequently, and the delayed workqueue operates in > jiffies > resolution. > > 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 | 77 > ++++++++++++++++++++++++++++++- > 1 file changed, 76 insertions(+), 1 deletion(-) > > diff --git a/drivers/thermal/intel/intel_hfi.c > b/drivers/thermal/intel/intel_hfi.c > index e2b82d71ab6b..24d708268c68 100644 > --- a/drivers/thermal/intel/intel_hfi.c > +++ b/drivers/thermal/intel/intel_hfi.c > @@ -43,6 +43,10 @@ > > #include <asm/msr.h> > > +#ifdef CONFIG_DEBUG_FS > +#include <linux/debugfs.h> > +#endif > + > #include "intel_hfi.h" > #include "thermal_interrupt.h" > > @@ -169,6 +173,70 @@ static struct workqueue_struct *hfi_updates_wq; > #define HFI_UPDATE_DELAY HZ > #define HFI_MAX_THERM_NOTIFY_COUNT 16 > > +/* Keep this variable 8-byte aligned to get atomic accesses. */ > +static unsigned long hfi_update_delay = HFI_UPDATE_DELAY; > + > +#ifdef CONFIG_DEBUG_FS > +static int hfi_update_delay_get(void *data, u64 *val) > +{ > + mutex_lock(&hfi_instance_lock); > + *val = jiffies_to_msecs(hfi_update_delay); > + mutex_unlock(&hfi_instance_lock); > + return 0; > +} > + > +static int hfi_update_delay_set(void *data, u64 val) > +{ > + /* > + * The mutex only serializes access to the debugfs file. > + * > + * hfi_process_event() loads @hfi_update_delay from interrupt > context. > + * We could have serialized accesses with a spinlock or a > memory barrier. > + * But this is a debug feature, the store of > @hfi_update_delay is > + * atomic, and will seldom change. A few loads of > @hfi_update_delay may > + * see stale values but the updated value will be seen > eventually. > + */ > + mutex_lock(&hfi_instance_lock); > + hfi_update_delay = msecs_to_jiffies(val); > + mutex_unlock(&hfi_instance_lock); > + return 0; > +} > + > +DEFINE_DEBUGFS_ATTRIBUTE(hfi_update_delay_fops, > hfi_update_delay_get, > + hfi_update_delay_set, "%llu\n"); > + > +static struct dentry *hfi_debugfs_dir; > + > +static void hfi_debugfs_unregister(void) > +{ > + debugfs_remove_recursive(hfi_debugfs_dir); > + hfi_debugfs_dir = NULL; > +} > + > +static void hfi_debugfs_register(void) > +{ > + struct dentry *f; > + > + hfi_debugfs_dir = debugfs_create_dir("intel_hfi", NULL); > + if (!hfi_debugfs_dir) > + return; > + > + f = debugfs_create_file("update_delay_ms", 0644, > hfi_debugfs_dir, > + NULL, &hfi_update_delay_fops); > + if (!f) > + goto err; > + > + return; > +err: > + hfi_debugfs_unregister(); > +} > + > +#else > +static void hfi_debugfs_register(void) > +{ > +} > +#endif /* CONFIG_DEBUG_FS */ > + > static void get_hfi_caps(struct hfi_instance *hfi_instance, > struct thermal_genl_cpu_caps *cpu_caps) > { > @@ -321,8 +389,13 @@ void intel_hfi_process_event(__u64 > pkg_therm_status_msr_val) > raw_spin_unlock(&hfi_instance->table_lock); > raw_spin_unlock(&hfi_instance->event_lock); > > + /* > + * debugfs may atomically store @hfi_update_delay without > locking. The > + * updated value may not be immediately observed. See note in > + * hfi_update_delay_set(). > + */ > queue_delayed_work(hfi_updates_wq, &hfi_instance- > >update_work, > - HFI_UPDATE_DELAY); > + hfi_update_delay); > } > > static void init_hfi_cpu_index(struct hfi_cpu_info *info) > @@ -708,6 +781,8 @@ void __init intel_hfi_init(void) > > register_syscore_ops(&hfi_pm_ops); > > + hfi_debugfs_register(); > + > return; > > err_nl_notif:
| |