| Message ID | 20240131120535.933424-4-stanislaw.gruszka@linux.intel.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | thermal/netlink/intel_hfi: Enable HFI feature only when required | expand |
On Wed, Jan 31, 2024 at 01:05:35PM +0100, Stanislaw Gruszka wrote: > Enable and disable hardware feedback interface (HFI) when user space > handler is present. For example, enable HFI, when intel-speed-select or > Intel Low Power daemon is running and subscribing to thermal netlink > events,. When user space handlers exit or remove subscription for > thermal netlink events, disable HFI. I'd rephrase as: Enable and disable HFI when a user space handler is running and listening to thermal netlink events (e.g., intel-speed-select, Intel Low Power daemon). When the user space handlers exit or remove subscription, disable HFI. > > Summary of changes: > > - When CPU is online, instead of blindly enabling HFI by calling > hfi_enable(), check if the thermal netlink group has any listener. > This will make sure that HFI is not enabled by default during boot > time. s/by calling hfi_enable()// > > - Register a netlink notifier. > > - In the notifier process reason code NETLINK_CHANGE and s/notifier/notifier,/ > NETLINK_URELEASE. If thermal netlink group has any listener enable > HFI on all packages. If there are no listener disable HFI on all > packages. > > - Actual processing to enable/disable matches what is done in > suspend/resume callbacks. So, create two functions hfi_do_enable() s/So,// > and hfi_do_disable(), which can be called from the netlink notifier > callback and suspend/resume callbacks. > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> > --- > drivers/thermal/intel/intel_hfi.c | 82 +++++++++++++++++++++++++++---- > 1 file changed, 73 insertions(+), 9 deletions(-) > > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c > index 3b04c6ec4fca..50601f75f6dc 100644 > --- a/drivers/thermal/intel/intel_hfi.c > +++ b/drivers/thermal/intel/intel_hfi.c > @@ -30,6 +30,7 @@ > #include <linux/kernel.h> > #include <linux/math.h> > #include <linux/mutex.h> > +#include <linux/netlink.h> > #include <linux/percpu-defs.h> > #include <linux/printk.h> > #include <linux/processor.h> > @@ -480,7 +481,8 @@ void intel_hfi_online(unsigned int cpu) > /* Enable this HFI instance if this is its first online CPU. */ > if (cpumask_weight(hfi_instance->cpus) == 1) { > hfi_set_hw_table(hfi_instance); > - hfi_enable(); > + if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) > + hfi_enable(); You could avoid the extra level of indentation if you did: if (cpumask_weight() == 1 && has_listeners()) You would need to also update the comment. My two cents.
On Wed, Jan 31, 2024 at 05:48:08PM -0800, Ricardo Neri wrote: > > drivers/thermal/intel/intel_hfi.c | 82 +++++++++++++++++++++++++++---- > > 1 file changed, 73 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c > > index 3b04c6ec4fca..50601f75f6dc 100644 > > --- a/drivers/thermal/intel/intel_hfi.c > > +++ b/drivers/thermal/intel/intel_hfi.c > > @@ -30,6 +30,7 @@ > > #include <linux/kernel.h> > > #include <linux/math.h> > > #include <linux/mutex.h> > > +#include <linux/netlink.h> > > #include <linux/percpu-defs.h> > > #include <linux/printk.h> > > #include <linux/processor.h> > > @@ -480,7 +481,8 @@ void intel_hfi_online(unsigned int cpu) > > /* Enable this HFI instance if this is its first online CPU. */ > > if (cpumask_weight(hfi_instance->cpus) == 1) { > > hfi_set_hw_table(hfi_instance); > > - hfi_enable(); > > + if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) > > + hfi_enable(); > > You could avoid the extra level of indentation if you did: > if (cpumask_weight() == 1 && has_listeners()) Ok. > You would need to also update the comment. I'd rather remove the comment, code looks obvious enough for me. Regards Stanislaw
On Thu, Feb 01, 2024 at 02:20:04PM +0100, Stanislaw Gruszka wrote: > On Wed, Jan 31, 2024 at 05:48:08PM -0800, Ricardo Neri wrote: > > > drivers/thermal/intel/intel_hfi.c | 82 +++++++++++++++++++++++++++---- > > > 1 file changed, 73 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c > > > index 3b04c6ec4fca..50601f75f6dc 100644 > > > --- a/drivers/thermal/intel/intel_hfi.c > > > +++ b/drivers/thermal/intel/intel_hfi.c > > > @@ -30,6 +30,7 @@ > > > #include <linux/kernel.h> > > > #include <linux/math.h> > > > #include <linux/mutex.h> > > > +#include <linux/netlink.h> > > > #include <linux/percpu-defs.h> > > > #include <linux/printk.h> > > > #include <linux/processor.h> > > > @@ -480,7 +481,8 @@ void intel_hfi_online(unsigned int cpu) > > > /* Enable this HFI instance if this is its first online CPU. */ > > > if (cpumask_weight(hfi_instance->cpus) == 1) { > > > hfi_set_hw_table(hfi_instance); > > > - hfi_enable(); > > > + if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) > > > + hfi_enable(); > > > > You could avoid the extra level of indentation if you did: > > if (cpumask_weight() == 1 && has_listeners()) > > Ok. > > > You would need to also update the comment. > > I'd rather remove the comment, code looks obvious enough for me. Do you think that it is clar that needs to be done only once per package? I guess it is clear but only after reading the more code.
Wed, Jan 31, 2024 at 01:05:35PM CET, stanislaw.gruszka@linux.intel.com wrote: [...] >+static int hfi_netlink_notify(struct notifier_block *nb, unsigned long state, >+ void *_notify) >+{ >+ struct netlink_notify *notify = _notify; >+ struct hfi_instance *hfi_instance; >+ smp_call_func_t func; >+ unsigned int cpu; >+ int i; >+ >+ if (notify->protocol != NETLINK_GENERIC) >+ return NOTIFY_DONE; >+ >+ switch (state) { >+ case NETLINK_CHANGE: >+ case NETLINK_URELEASE: >+ mutex_lock(&hfi_instance_lock); >+ What's stopping other thread from mangling the listeners here? >+ if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) >+ func = hfi_do_enable; >+ else >+ func = hfi_do_disable; >+ >+ for (i = 0; i < max_hfi_instances; i++) { >+ hfi_instance = &hfi_instances[i]; >+ if (cpumask_empty(hfi_instance->cpus)) >+ continue; >+ >+ cpu = cpumask_any(hfi_instance->cpus); >+ smp_call_function_single(cpu, func, hfi_instance, true); >+ } >+ >+ mutex_unlock(&hfi_instance_lock); >+ return NOTIFY_OK; >+ } >+ >+ return NOTIFY_DONE; >+} [...]
On Fri, Feb 02, 2024 at 01:36:09PM +0100, Jiri Pirko wrote: > Wed, Jan 31, 2024 at 01:05:35PM CET, stanislaw.gruszka@linux.intel.com wrote: > > [...] > > > >+static int hfi_netlink_notify(struct notifier_block *nb, unsigned long state, > >+ void *_notify) > >+{ > >+ struct netlink_notify *notify = _notify; > >+ struct hfi_instance *hfi_instance; > >+ smp_call_func_t func; > >+ unsigned int cpu; > >+ int i; > >+ > >+ if (notify->protocol != NETLINK_GENERIC) > >+ return NOTIFY_DONE; > >+ > >+ switch (state) { > >+ case NETLINK_CHANGE: > >+ case NETLINK_URELEASE: > >+ mutex_lock(&hfi_instance_lock); > >+ > > What's stopping other thread from mangling the listeners here? Nothing. But if the listeners will be changed, we will get next notify. Serialization by the mutex is needed to assure that the last setting will win, so we do not end with HFI disabled when there are listeners or vice versa. > >+ if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) > >+ func = hfi_do_enable; > >+ else > >+ func = hfi_do_disable; > >+ > >+ for (i = 0; i < max_hfi_instances; i++) { > >+ hfi_instance = &hfi_instances[i]; > >+ if (cpumask_empty(hfi_instance->cpus)) > >+ continue; > >+ > >+ cpu = cpumask_any(hfi_instance->cpus); > >+ smp_call_function_single(cpu, func, hfi_instance, true); > >+ } > >+ > >+ mutex_unlock(&hfi_instance_lock); > >+ return NOTIFY_OK; > >+ } > >+ > >+ return NOTIFY_DONE; > >+} > > [...]
Fri, Feb 02, 2024 at 02:00:46PM CET, stanislaw.gruszka@linux.intel.com wrote: >On Fri, Feb 02, 2024 at 01:36:09PM +0100, Jiri Pirko wrote: >> Wed, Jan 31, 2024 at 01:05:35PM CET, stanislaw.gruszka@linux.intel.com wrote: >> >> [...] >> >> >> >+static int hfi_netlink_notify(struct notifier_block *nb, unsigned long state, >> >+ void *_notify) >> >+{ >> >+ struct netlink_notify *notify = _notify; >> >+ struct hfi_instance *hfi_instance; >> >+ smp_call_func_t func; >> >+ unsigned int cpu; >> >+ int i; >> >+ >> >+ if (notify->protocol != NETLINK_GENERIC) >> >+ return NOTIFY_DONE; >> >+ >> >+ switch (state) { >> >+ case NETLINK_CHANGE: >> >+ case NETLINK_URELEASE: >> >+ mutex_lock(&hfi_instance_lock); >> >+ >> >> What's stopping other thread from mangling the listeners here? > >Nothing. But if the listeners will be changed, we will get next notify. >Serialization by the mutex is needed to assure that the last setting will win, >so we do not end with HFI disabled when there are listeners or vice versa. Okay. Care to put a note somewhere? > >> >+ if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) >> >+ func = hfi_do_enable; >> >+ else >> >+ func = hfi_do_disable; >> >+ >> >+ for (i = 0; i < max_hfi_instances; i++) { >> >+ hfi_instance = &hfi_instances[i]; >> >+ if (cpumask_empty(hfi_instance->cpus)) >> >+ continue; >> >+ >> >+ cpu = cpumask_any(hfi_instance->cpus); >> >+ smp_call_function_single(cpu, func, hfi_instance, true); >> >+ } >> >+ >> >+ mutex_unlock(&hfi_instance_lock); >> >+ return NOTIFY_OK; >> >+ } >> >+ >> >+ return NOTIFY_DONE; >> >+} >> >> [...] >
On Sat, Feb 03, 2024 at 02:15:19PM +0100, Jiri Pirko wrote: > Fri, Feb 02, 2024 at 02:00:46PM CET, stanislaw.gruszka@linux.intel.com wrote: > >On Fri, Feb 02, 2024 at 01:36:09PM +0100, Jiri Pirko wrote: > >> Wed, Jan 31, 2024 at 01:05:35PM CET, stanislaw.gruszka@linux.intel.com wrote: > >> > >> [...] > >> > >> > >> >+static int hfi_netlink_notify(struct notifier_block *nb, unsigned long state, > >> >+ void *_notify) > >> >+{ > >> >+ struct netlink_notify *notify = _notify; > >> >+ struct hfi_instance *hfi_instance; > >> >+ smp_call_func_t func; > >> >+ unsigned int cpu; > >> >+ int i; > >> >+ > >> >+ if (notify->protocol != NETLINK_GENERIC) > >> >+ return NOTIFY_DONE; > >> >+ > >> >+ switch (state) { > >> >+ case NETLINK_CHANGE: > >> >+ case NETLINK_URELEASE: > >> >+ mutex_lock(&hfi_instance_lock); > >> >+ > >> > >> What's stopping other thread from mangling the listeners here? > > > >Nothing. But if the listeners will be changed, we will get next notify. > >Serialization by the mutex is needed to assure that the last setting will win, > >so we do not end with HFI disabled when there are listeners or vice versa. > > Okay. Care to put a note somewhere? I would if the flow would stay the same. But it was requested by Jakub to use netlink bind/unbind, and this will not work the way described above any longer, since bind() is before listeners change and unbind() after: if (optname == NETLINK_ADD_MEMBERSHIP && nlk->netlink_bind) { err = nlk->netlink_bind(sock_net(sk), val); if (err) return err; } netlink_table_grab(); netlink_update_socket_mc(nlk, val, optname == NETLINK_ADD_MEMBERSHIP); netlink_table_ungrab(); if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind) nlk->netlink_unbind(sock_net(sk), val) To avoid convoluted logic or new global lock, I'll use properly protected local counter increased on bind and decreased on unbind. Regards Stanislaw
diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c index 3b04c6ec4fca..50601f75f6dc 100644 --- a/drivers/thermal/intel/intel_hfi.c +++ b/drivers/thermal/intel/intel_hfi.c @@ -30,6 +30,7 @@ #include <linux/kernel.h> #include <linux/math.h> #include <linux/mutex.h> +#include <linux/netlink.h> #include <linux/percpu-defs.h> #include <linux/printk.h> #include <linux/processor.h> @@ -480,7 +481,8 @@ void intel_hfi_online(unsigned int cpu) /* Enable this HFI instance if this is its first online CPU. */ if (cpumask_weight(hfi_instance->cpus) == 1) { hfi_set_hw_table(hfi_instance); - hfi_enable(); + if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) + hfi_enable(); } unlock: @@ -573,28 +575,84 @@ static __init int hfi_parse_features(void) return 0; } -static void hfi_do_enable(void) +/* + * HFI enable/disable run in non-concurrent manner on boot CPU in syscore + * callbacks or under protection of hfi_instance_lock. + */ +static void hfi_do_enable(void *ptr) +{ + struct hfi_instance *hfi_instance = ptr; + + hfi_set_hw_table(hfi_instance); + hfi_enable(); +} + +static void hfi_do_disable(void *ptr) +{ + hfi_disable(); +} + +static void hfi_syscore_resume(void) { /* This code runs only on the boot CPU. */ struct hfi_cpu_info *info = &per_cpu(hfi_cpu_info, 0); struct hfi_instance *hfi_instance = info->hfi_instance; - /* No locking needed. There is no concurrency with CPU online. */ - hfi_set_hw_table(hfi_instance); - hfi_enable(); + hfi_do_enable(hfi_instance); } -static int hfi_do_disable(void) +static int hfi_syscore_suspend(void) { - /* No locking needed. There is no concurrency with CPU offline. */ hfi_disable(); return 0; } static struct syscore_ops hfi_pm_ops = { - .resume = hfi_do_enable, - .suspend = hfi_do_disable, + .resume = hfi_syscore_resume, + .suspend = hfi_syscore_suspend, +}; + +static int hfi_netlink_notify(struct notifier_block *nb, unsigned long state, + void *_notify) +{ + struct netlink_notify *notify = _notify; + struct hfi_instance *hfi_instance; + smp_call_func_t func; + unsigned int cpu; + int i; + + if (notify->protocol != NETLINK_GENERIC) + return NOTIFY_DONE; + + switch (state) { + case NETLINK_CHANGE: + case NETLINK_URELEASE: + mutex_lock(&hfi_instance_lock); + + if (thermal_group_has_listeners(THERMAL_GENL_EVENT_GROUP)) + func = hfi_do_enable; + else + func = hfi_do_disable; + + for (i = 0; i < max_hfi_instances; i++) { + hfi_instance = &hfi_instances[i]; + if (cpumask_empty(hfi_instance->cpus)) + continue; + + cpu = cpumask_any(hfi_instance->cpus); + smp_call_function_single(cpu, func, hfi_instance, true); + } + + mutex_unlock(&hfi_instance_lock); + return NOTIFY_OK; + } + + return NOTIFY_DONE; +} + +static struct notifier_block hfi_netlink_nb = { + .notifier_call = hfi_netlink_notify, }; void __init intel_hfi_init(void) @@ -628,10 +686,16 @@ void __init intel_hfi_init(void) if (!hfi_updates_wq) goto err_nomem; + if (netlink_register_notifier(&hfi_netlink_nb)) + goto err_nl_notif; + register_syscore_ops(&hfi_pm_ops); return; +err_nl_notif: + destroy_workqueue(hfi_updates_wq); + err_nomem: for (j = 0; j < i; ++j) { hfi_instance = &hfi_instances[j];
Enable and disable hardware feedback interface (HFI) when user space handler is present. For example, enable HFI, when intel-speed-select or Intel Low Power daemon is running and subscribing to thermal netlink events,. When user space handlers exit or remove subscription for thermal netlink events, disable HFI. Summary of changes: - When CPU is online, instead of blindly enabling HFI by calling hfi_enable(), check if the thermal netlink group has any listener. This will make sure that HFI is not enabled by default during boot time. - Register a netlink notifier. - In the notifier process reason code NETLINK_CHANGE and NETLINK_URELEASE. If thermal netlink group has any listener enable HFI on all packages. If there are no listener disable HFI on all packages. - Actual processing to enable/disable matches what is done in suspend/resume callbacks. So, create two functions hfi_do_enable() and hfi_do_disable(), which can be called from the netlink notifier callback and suspend/resume callbacks. Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com> --- drivers/thermal/intel/intel_hfi.c | 82 +++++++++++++++++++++++++++---- 1 file changed, 73 insertions(+), 9 deletions(-)