| Message ID | 20240131120535.933424-1-stanislaw.gruszka@linux.intel.com |
|---|---|
| Headers | show |
| Series | thermal/netlink/intel_hfi: Enable HFI feature only when required | expand |
On Wed, 31 Jan 2024 13:05:33 +0100 Stanislaw Gruszka wrote: > Add notification when adding/removing multicast group to/from > client socket via setsockopt() syscall. > > It can be used with conjunction with netlink_has_listeners() to check > if consumers of netlink multicast messages emerge or disappear. > > A client can call netlink_register_notifier() to register a callback. > In the callback check for state NETLINK_CHANGE and NETLINK_URELEASE to > get notification for change in the netlink socket membership. > > Thus, a client can now send events only when there are active consumers, > preventing unnecessary work when none exist. Can we plumb thru the existing netlink_bind / netlink_unbind callbacks? Add similar callbacks to the genl family struct to plumb it thru to thermal. Then thermal can do what it wants with it (also add driver callbacks or notifiers). Having a driver listen to a core AF_NETLINK notifier to learn about changes to a genl family it registers with skips too many layers to easily reason about. At least for my taste. When you repost please CC Florian W, Johannes B and Jiri P, off the top of my head. Folks who most often work on netlink internals..
On Wed, Jan 31, 2024 at 05:40:56PM -0800, Jakub Kicinski wrote: > On Wed, 31 Jan 2024 13:05:33 +0100 Stanislaw Gruszka wrote: > > Add notification when adding/removing multicast group to/from > > client socket via setsockopt() syscall. > > > > It can be used with conjunction with netlink_has_listeners() to check > > if consumers of netlink multicast messages emerge or disappear. > > > > A client can call netlink_register_notifier() to register a callback. > > In the callback check for state NETLINK_CHANGE and NETLINK_URELEASE to > > get notification for change in the netlink socket membership. > > > > Thus, a client can now send events only when there are active consumers, > > preventing unnecessary work when none exist. > > Can we plumb thru the existing netlink_bind / netlink_unbind callbacks? > > Add similar callbacks to the genl family struct to plumb it thru to > thermal. Then thermal can do what it wants with it (also add driver > callbacks or notifiers). Yes, sure, can be done this way and make sense. Going to do this. > Having a driver listen to a core AF_NETLINK notifier to learn about > changes to a genl family it registers with skips too many layers to > easily reason about. At least for my taste. > > When you repost please CC Florian W, Johannes B and Jiri P, off the top > of my head. Folks who most often work on netlink internals.. Ok. Regards Stanislaw >
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