[PATCH 0/3] thermal/netlink/intel_hfi: Enable HFI feature only when required

[PATCH 0/3] thermal/netlink/intel_hfi: Enable HFI feature only when required

[Stanislaw Gruszka]

The patchset introduces a netlink notification, which together with
netlink_has_listners() check, allow drivers to send netlink multicast
events based on the presence of actual user-space consumers.
This functionality optimizes resource usage by allowing disabling
of features when not needed.

Then implement the notification mechanism in the intel_hif driver,
it is utilized to disable the Hardware Feedback Interface (HFI)
dynamically. By implementing a netlink notify callback, the driver
can now enable or disable the HFI based on actual demand, particularly
when user-space applications like intel-speed-select or Intel Low Power
daemon utilize events related to performance and energy efficiency
capabilities.

On machines where Intel HFI is present, but there are no user-space
components installed, we can save tons of CPU cycles.

Stanislaw Gruszka (3):
  netlink: Add notifier when changing netlink socket membership
  thermal: netlink: Export thermal_group_has_listeners()
  thermal: intel: hfi: Enable interface only when required

 drivers/thermal/intel/intel_hfi.c | 82 +++++++++++++++++++++++++++----
 drivers/thermal/thermal_netlink.c |  7 +--
 drivers/thermal/thermal_netlink.h | 11 +++++
 include/linux/notifier.h          |  1 +
 net/netlink/af_netlink.c          |  6 +++
 5 files changed, 92 insertions(+), 15 deletions(-)

-- 
2.34.1

[PATCH 1/3] netlink: Add notifier when changing netlink socket membership

[Stanislaw Gruszka]

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.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 include/linux/notifier.h | 1 +
 net/netlink/af_netlink.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/include/linux/notifier.h b/include/linux/notifier.h
index 45702bdcbceb..8f5a5eed1e0e 100644
--- a/include/linux/notifier.h
+++ b/include/linux/notifier.h
@@ -227,6 +227,7 @@ static inline int notifier_to_errno(int ret)
 /* Virtual Terminal events are defined in include/linux/vt.h. */
 
 #define NETLINK_URELEASE	0x0001	/* Unicast netlink socket released */
+#define NETLINK_CHANGE		0x0002  /* Changed membership of netlink socket */
 
 /* Console keyboard events.
  * Note: KBD_KEYCODE is always sent before KBD_UNBOUND_KEYCODE, KBD_UNICODE and
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index eb086b06d60d..674af2cb0f12 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1680,6 +1680,11 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 	case NETLINK_ADD_MEMBERSHIP:
 	case NETLINK_DROP_MEMBERSHIP: {
 		int err;
+		struct netlink_notify n = {
+			.net = sock_net(sk),
+			.protocol = sk->sk_protocol,
+			.portid = nlk->portid,
+		};
 
 		if (!netlink_allowed(sock, NL_CFG_F_NONROOT_RECV))
 			return -EPERM;
@@ -1700,6 +1705,7 @@ static int netlink_setsockopt(struct socket *sock, int level, int optname,
 		if (optname == NETLINK_DROP_MEMBERSHIP && nlk->netlink_unbind)
 			nlk->netlink_unbind(sock_net(sk), val);
 
+		blocking_notifier_call_chain(&netlink_chain, NETLINK_CHANGE, &n);
 		break;
 	}
 	case NETLINK_BROADCAST_ERROR:
-- 
2.34.1

Re: [PATCH 1/3] netlink: Add notifier when changing netlink socket membership

[Jakub Kicinski]

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..

Re: [PATCH 1/3] netlink: Add notifier when changing netlink socket membership

[Stanislaw Gruszka]

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
> 

[PATCH 2/3] thermal: netlink: Export thermal_group_has_listeners()

[Stanislaw Gruszka]

Let drivers use thermal_group_has_listners(). The intel_hfi driver needs
it to enable HFI only when user-space consumers are present.

Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/thermal/thermal_netlink.c |  7 +------
 drivers/thermal/thermal_netlink.h | 11 +++++++++++
 2 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/thermal/thermal_netlink.c b/drivers/thermal/thermal_netlink.c
index 569e4fa62f73..44e8df2751ba 100644
--- a/drivers/thermal/thermal_netlink.c
+++ b/drivers/thermal/thermal_netlink.c
@@ -13,11 +13,6 @@
 
 #include "thermal_core.h"
 
-enum thermal_genl_multicast_groups {
-	THERMAL_GENL_SAMPLING_GROUP = 0,
-	THERMAL_GENL_EVENT_GROUP = 1,
-};
-
 static const struct genl_multicast_group thermal_genl_mcgrps[] = {
 	[THERMAL_GENL_SAMPLING_GROUP] = { .name = THERMAL_GENL_SAMPLING_GROUP_NAME, },
 	[THERMAL_GENL_EVENT_GROUP]  = { .name = THERMAL_GENL_EVENT_GROUP_NAME,  },
@@ -76,7 +71,7 @@ typedef int (*cb_t)(struct param *);
 
 static struct genl_family thermal_gnl_family;
 
-static int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
+int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
 {
 	return genl_has_listeners(&thermal_gnl_family, &init_net, group);
 }
diff --git a/drivers/thermal/thermal_netlink.h b/drivers/thermal/thermal_netlink.h
index 0a9987c3bc57..3272a966f404 100644
--- a/drivers/thermal/thermal_netlink.h
+++ b/drivers/thermal/thermal_netlink.h
@@ -10,10 +10,16 @@ struct thermal_genl_cpu_caps {
 	int efficiency;
 };
 
+enum thermal_genl_multicast_groups {
+	THERMAL_GENL_SAMPLING_GROUP = 0,
+	THERMAL_GENL_EVENT_GROUP = 1,
+};
+
 /* Netlink notification function */
 #ifdef CONFIG_THERMAL_NETLINK
 int __init thermal_netlink_init(void);
 void __init thermal_netlink_exit(void);
+int thermal_group_has_listeners(enum thermal_genl_multicast_groups group);
 int thermal_notify_tz_create(int tz_id, const char *name);
 int thermal_notify_tz_delete(int tz_id);
 int thermal_notify_tz_enable(int tz_id);
@@ -38,6 +44,11 @@ static inline int thermal_netlink_init(void)
 	return 0;
 }
 
+static inline int thermal_group_has_listeners(enum thermal_genl_multicast_groups group)
+{
+	return 0;
+}
+
 static inline int thermal_notify_tz_create(int tz_id, const char *name)
 {
 	return 0;
-- 
2.34.1

[PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Stanislaw Gruszka]

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(-)

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];
-- 
2.34.1

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Ricardo Neri]

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.
 

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Stanislaw Gruszka]

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

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Ricardo Neri]

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.

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Jiri Pirko]

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;
>+}

[...]

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Stanislaw Gruszka]

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;
> >+}
> 
> [...]

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Jiri Pirko]

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;
>> >+}
>> 
>> [...]
>

Re: [PATCH 3/3] thermal: intel: hfi: Enable interface only when required

[Stanislaw Gruszka]

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