mbox series

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

Message ID 20240131120535.933424-1-stanislaw.gruszka@linux.intel.com
Headers show
Series thermal/netlink/intel_hfi: Enable HFI feature only when required | expand

Message

Stanislaw Gruszka Jan. 31, 2024, 12:05 p.m. UTC 
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(-)

Comments

Jakub Kicinski Feb. 1, 2024, 1:40 a.m. UTC | #1 
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..
Stanislaw Gruszka Feb. 1, 2024, 1:10 p.m. UTC | #2 
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
>
Stanislaw Gruszka Feb. 1, 2024, 1:20 p.m. UTC | #3 
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
Ricardo Neri Feb. 1, 2024, 4:21 p.m. UTC | #4 
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.
Jiri Pirko Feb. 2, 2024, 12:36 p.m. UTC | #5 
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;
>+}

[...]
Stanislaw Gruszka Feb. 2, 2024, 1 p.m. UTC | #6 
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;
> >+}
> 
> [...]
Jiri Pirko Feb. 3, 2024, 1:15 p.m. UTC | #7 
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;
>> >+}
>> 
>> [...]
>
Stanislaw Gruszka Feb. 5, 2024, noon UTC | #8 
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