mbox series

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

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

Message

Stanislaw Gruszka Feb. 12, 2024, 4:16 p.m. UTC
The patchset introduces a new genetlink family bind/unbind callbacks
and thermal/netlink notifications, which 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 thermal genl 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.

Changes v3 -> v4:

- Add 'static inline' in patch2

Changes v2 -> v3:

- Fix unused variable compilation warning
- Add missed Suggested by tag to patch2
 
Changes v1 -> v2:

- Rewrite using netlink_bind/netlink_unbind callbacks.

- Minor changelog tweaks.

- Add missing check in intel hfi syscore resume (had it on my testing,
but somehow missed in post).

- Do not use netlink_has_listeners() any longer, use custom counter instead.
To keep using netlink_has_listners() would be required to rearrange 
netlink_setsockopt() and possibly netlink_bind() functions, to call 
nlk->netlink_bind() after listeners are updated. So I decided to custom
counter. This have potential issue as thermal netlink registers before
intel_hif, so theoretically intel_hif can miss events. But since both
are required to be kernel build-in (if CONFIG_INTEL_HFI_THERMAL is
configured), they start before any user-space.

v1: https://lore.kernel.org/linux-pm/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/

Stanislaw Gruszka (3):
  genetlink: Add per family bind/unbind callbacks
  thermal: netlink: Add genetlink bind/unbind notifications
  thermal: intel: hfi: Enable interface only when required

 drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
 drivers/thermal/thermal_netlink.c | 40 +++++++++++--
 drivers/thermal/thermal_netlink.h | 26 +++++++++
 include/net/genetlink.h           |  4 ++
 net/netlink/genetlink.c           | 30 ++++++++++
 5 files changed, 180 insertions(+), 15 deletions(-)

Comments

Jiri Pirko Feb. 13, 2024, 9:11 a.m. UTC | #1
Mon, Feb 12, 2024 at 05:16:13PM CET, stanislaw.gruszka@linux.intel.com wrote:
>Add genetlink family bind()/unbind() callbacks when adding/removing
>multicast group to/from netlink client socket via setsockopt() or
>bind() syscall.
>
>They can be used to track if consumers of netlink multicast messages
>emerge or disappear. Thus, a client implementing callbacks, can now
>send events only when there are active consumers, preventing unnecessary
>work when none exist.
>
>Suggested-by: Jakub Kicinski <kuba@kernel.org>
>Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>
Rafael J. Wysocki Feb. 13, 2024, 1:59 p.m. UTC | #2
On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> 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.
>
> Summary of changes:
>
> - Register a thermal genetlink notifier
>
> - In the notifier, process THERMAL_NOTIFY_BIND and THERMAL_NOTIFY_UNBIND
> reason codes to count number of thermal event group netlink multicast
> clients. If thermal netlink group has any listener enable HFI on all
> packages. If there are no listener disable HFI on all packages.
>
> - When CPU is online, instead of blindly enabling HFI, check if
> the thermal netlink group has any listener. This will make sure that
> HFI is not enabled by default during boot time.
>
> - Actual processing to enable/disable matches what is done in
> suspend/resume callbacks. 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 | 95 +++++++++++++++++++++++++++----
>  1 file changed, 85 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/thermal/intel/intel_hfi.c b/drivers/thermal/intel/intel_hfi.c
> index 3b04c6ec4fca..5e1e2b5269b7 100644
> --- a/drivers/thermal/intel/intel_hfi.c
> +++ b/drivers/thermal/intel/intel_hfi.c
> @@ -159,6 +159,7 @@ struct hfi_cpu_info {
>  static DEFINE_PER_CPU(struct hfi_cpu_info, hfi_cpu_info) = { .index = -1 };
>
>  static int max_hfi_instances;
> +static int hfi_thermal_clients_num;
>  static struct hfi_instance *hfi_instances;
>
>  static struct hfi_features hfi_features;
> @@ -477,8 +478,11 @@ void intel_hfi_online(unsigned int cpu)
>  enable:
>         cpumask_set_cpu(cpu, hfi_instance->cpus);
>
> -       /* Enable this HFI instance if this is its first online CPU. */
> -       if (cpumask_weight(hfi_instance->cpus) == 1) {
> +       /*
> +        * Enable this HFI instance if this is its first online CPU and
> +        * there are user-space clients of thermal events.
> +        */
> +       if (cpumask_weight(hfi_instance->cpus) == 1 && hfi_thermal_clients_num > 0) {
>                 hfi_set_hw_table(hfi_instance);
>                 hfi_enable();
>         }
> @@ -573,28 +577,93 @@ 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.
> + */

In the comment above I would say "If concurrency is not prevented by
other means, the HFI enable/disable routines must be called under
hfi_instance_lock." and I would retain the comments below (they don't
hurt IMO).

> +static void hfi_do_enable(void *ptr)

I would call this hfi_enable_instance().

> +{
> +       struct hfi_instance *hfi_instance = ptr;

Why is this variable needed ro even useful?  prt can be passed
directly to hfi_set_hw_table().

> +
> +       hfi_set_hw_table(hfi_instance);
> +       hfi_enable();
> +}
> +
> +static void hfi_do_disable(void *ptr)

And I'd call this hfi_disable_instance().

> +{
> +       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();
> +       if (hfi_thermal_clients_num > 0)
> +               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_thermal_notify(struct notifier_block *nb, unsigned long state,
> +                             void *_notify)
> +{
> +       struct thermal_genl_notify *notify = _notify;
> +       struct hfi_instance *hfi_instance;
> +       smp_call_func_t func;
> +       unsigned int cpu;
> +       int i;
> +
> +       if (notify->mcgrp != THERMAL_GENL_EVENT_GROUP)
> +               return NOTIFY_DONE;
> +
> +       if (state != THERMAL_NOTIFY_BIND && state != THERMAL_NOTIFY_UNBIND)
> +               return NOTIFY_DONE;
> +
> +       mutex_lock(&hfi_instance_lock);
> +
> +       switch (state) {
> +       case THERMAL_NOTIFY_BIND:
> +               hfi_thermal_clients_num++;
> +               break;
> +
> +       case THERMAL_NOTIFY_UNBIND:
> +               hfi_thermal_clients_num--;
> +               break;
> +       }
> +
> +       if (hfi_thermal_clients_num > 0)
> +               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);

So AFAICS, one instance can be enabled multiple times because of this.
  I guess that's OK?  In any case, it would be kind of nice to leave a
note regarding it somewhere here.

> +
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block hfi_thermal_nb = {
> +       .notifier_call = hfi_thermal_notify,
>  };
>
>  void __init intel_hfi_init(void)
> @@ -628,10 +697,16 @@ void __init intel_hfi_init(void)
>         if (!hfi_updates_wq)
>                 goto err_nomem;
>
> +       if (thermal_genl_register_notifier(&hfi_thermal_nb))
> +               goto err_nl_notif;

Is it possible for any clients to be there before the notifier is
registered?  If not, it would be good to add a comment about it.

> +
>         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];
> --
Jakub Kicinski Feb. 16, 2024, 5:29 a.m. UTC | #3
On Mon, 12 Feb 2024 17:16:12 +0100 Stanislaw Gruszka wrote:
>   genetlink: Add per family bind/unbind callbacks

genetlink patch is now in net-next, and pushed to a 6.8-rc4-based
branch at:

 https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git
  for-thermal-genetlink-family-bind-unbind-callbacks

for anyone to pull.
patchwork-bot+netdevbpf@kernel.org Feb. 16, 2024, 5:30 a.m. UTC | #4
Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 12 Feb 2024 17:16:12 +0100 you wrote:
> The patchset introduces a new genetlink family bind/unbind callbacks
> and thermal/netlink notifications, which 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 thermal genl 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.
> 
> [...]

Here is the summary with links:
  - [v4,1/3] genetlink: Add per family bind/unbind callbacks
    https://git.kernel.org/netdev/net-next/c/3de21a8990d3
  - [v4,2/3] thermal: netlink: Add genetlink bind/unbind notifications
    (no matching commit)
  - [v4,3/3] thermal: intel: hfi: Enable interface only when required
    (no matching commit)

You are awesome, thank you!
Stanislaw Gruszka Feb. 22, 2024, 4:53 p.m. UTC | #5
On Tue, Feb 13, 2024 at 02:59:10PM +0100, Rafael J. Wysocki wrote:
> > -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.
> > + */
> 
> In the comment above I would say "If concurrency is not prevented by
> other means, the HFI enable/disable routines must be called under
> hfi_instance_lock." 

Ok. Will reword this way.

> and I would retain the comments below (they don't
> hurt IMO).

I found those comments somewhat confusing. FWICT at worst
what can happen when enable/resume race CPU online and
disable/suspend race with CPU offline is enable twice
or disable twice. What I think is fine, though plan to
check this (see below).

> > +static void hfi_do_enable(void *ptr)
> 
> I would call this hfi_enable_instance().
> 
> > +{
> > +       struct hfi_instance *hfi_instance = ptr;
> 
> Why is this variable needed ro even useful?  prt can be passed
> directly to hfi_set_hw_table().

Ok, will remove it.

> > +
> > +       hfi_set_hw_table(hfi_instance);
> > +       hfi_enable();
> > +}
> > +
> > +static void hfi_do_disable(void *ptr)
> 
> And I'd call this hfi_disable_instance().

Ok.

> > +static int hfi_thermal_notify(struct notifier_block *nb, unsigned long state,
> > +                             void *_notify)
> > +{
> > +       struct thermal_genl_notify *notify = _notify;
> > +       struct hfi_instance *hfi_instance;
> > +       smp_call_func_t func;
> > +       unsigned int cpu;
> > +       int i;
> > +
> > +       if (notify->mcgrp != THERMAL_GENL_EVENT_GROUP)
> > +               return NOTIFY_DONE;
> > +
> > +       if (state != THERMAL_NOTIFY_BIND && state != THERMAL_NOTIFY_UNBIND)
> > +               return NOTIFY_DONE;
> > +
> > +       mutex_lock(&hfi_instance_lock);
> > +
> > +       switch (state) {
> > +       case THERMAL_NOTIFY_BIND:
> > +               hfi_thermal_clients_num++;
> > +               break;
> > +
> > +       case THERMAL_NOTIFY_UNBIND:
> > +               hfi_thermal_clients_num--;
> > +               break;
> > +       }
> > +
> > +       if (hfi_thermal_clients_num > 0)
> > +               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);
> 
> So AFAICS, one instance can be enabled multiple times because of this.
>   I guess that's OK?  In any case, it would be kind of nice to leave a
> note regarding it somewhere here.

It's write the same values to the same registers. So I think this 
should be fine. However after your comment I start to think there
perhaps could be some side-effect of writing the registers.
I'll double check (previously I verified that double enable works,
but only on MTL) or eventually rearrange code to do not enable already
enabled interface.

> > +
> > +       return NOTIFY_OK;
> > +}
> > +
> > +static struct notifier_block hfi_thermal_nb = {
> > +       .notifier_call = hfi_thermal_notify,
> >  };
> >
> >  void __init intel_hfi_init(void)
> > @@ -628,10 +697,16 @@ void __init intel_hfi_init(void)
> >         if (!hfi_updates_wq)
> >                 goto err_nomem;
> >
> > +       if (thermal_genl_register_notifier(&hfi_thermal_nb))
> > +               goto err_nl_notif;
> 
> Is it possible for any clients to be there before the notifier is
> registered?  If not, it would be good to add a comment about it.

HFI is build-in so it's started before any user space. I added note about that
in the cover letter but indeed it should be comment in the code. Will fix.  

Regards
Stanislaw
Stanislaw Gruszka Feb. 23, 2024, 3:44 p.m. UTC | #6
On Thu, Feb 15, 2024 at 09:29:46PM -0800, Jakub Kicinski wrote:
> On Mon, 12 Feb 2024 17:16:12 +0100 Stanislaw Gruszka wrote:
> >   genetlink: Add per family bind/unbind callbacks
> 
> genetlink patch is now in net-next, and pushed to a 6.8-rc4-based
> branch at:
> 
>  https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git
>   for-thermal-genetlink-family-bind-unbind-callbacks
> 
> for anyone to pull.

Thanks!

I'll post next version of this set just to linux-pm since remaining
patches are thermal specific. If they will be ready to apply
the above dependency can be pulled by Rafael - I assume this will
not create any marge conflict.

Regards
Stanislaw
Rafael J. Wysocki Feb. 29, 2024, 3:18 p.m. UTC | #7
On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> The patchset introduces a new genetlink family bind/unbind callbacks
> and thermal/netlink notifications, which 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 thermal genl 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.
>
> Changes v3 -> v4:
>
> - Add 'static inline' in patch2
>
> Changes v2 -> v3:
>
> - Fix unused variable compilation warning
> - Add missed Suggested by tag to patch2
>
> Changes v1 -> v2:
>
> - Rewrite using netlink_bind/netlink_unbind callbacks.
>
> - Minor changelog tweaks.
>
> - Add missing check in intel hfi syscore resume (had it on my testing,
> but somehow missed in post).
>
> - Do not use netlink_has_listeners() any longer, use custom counter instead.
> To keep using netlink_has_listners() would be required to rearrange
> netlink_setsockopt() and possibly netlink_bind() functions, to call
> nlk->netlink_bind() after listeners are updated. So I decided to custom
> counter. This have potential issue as thermal netlink registers before
> intel_hif, so theoretically intel_hif can miss events. But since both
> are required to be kernel build-in (if CONFIG_INTEL_HFI_THERMAL is
> configured), they start before any user-space.
>
> v1: https://lore.kernel.org/linux-pm/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
>
> Stanislaw Gruszka (3):
>   genetlink: Add per family bind/unbind callbacks
>   thermal: netlink: Add genetlink bind/unbind notifications
>   thermal: intel: hfi: Enable interface only when required
>
>  drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
>  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
>  drivers/thermal/thermal_netlink.h | 26 +++++++++
>  include/net/genetlink.h           |  4 ++
>  net/netlink/genetlink.c           | 30 ++++++++++
>  5 files changed, 180 insertions(+), 15 deletions(-)
>
> --

What tree is this based on?
Stanislaw Gruszka Feb. 29, 2024, 4:13 p.m. UTC | #8
On Thu, Feb 29, 2024 at 04:18:50PM +0100, Rafael J. Wysocki wrote:
> On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> <stanislaw.gruszka@linux.intel.com> wrote:
> >
> > The patchset introduces a new genetlink family bind/unbind callbacks
> > and thermal/netlink notifications, which 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 thermal genl 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.
> >
> > Changes v3 -> v4:
> >
> > - Add 'static inline' in patch2
> >
> > Changes v2 -> v3:
> >
> > - Fix unused variable compilation warning
> > - Add missed Suggested by tag to patch2
> >
> > Changes v1 -> v2:
> >
> > - Rewrite using netlink_bind/netlink_unbind callbacks.
> >
> > - Minor changelog tweaks.
> >
> > - Add missing check in intel hfi syscore resume (had it on my testing,
> > but somehow missed in post).
> >
> > - Do not use netlink_has_listeners() any longer, use custom counter instead.
> > To keep using netlink_has_listners() would be required to rearrange
> > netlink_setsockopt() and possibly netlink_bind() functions, to call
> > nlk->netlink_bind() after listeners are updated. So I decided to custom
> > counter. This have potential issue as thermal netlink registers before
> > intel_hif, so theoretically intel_hif can miss events. But since both
> > are required to be kernel build-in (if CONFIG_INTEL_HFI_THERMAL is
> > configured), they start before any user-space.
> >
> > v1: https://lore.kernel.org/linux-pm/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> > v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> > v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
> >
> > Stanislaw Gruszka (3):
> >   genetlink: Add per family bind/unbind callbacks
> >   thermal: netlink: Add genetlink bind/unbind notifications
> >   thermal: intel: hfi: Enable interface only when required
> >
> >  drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
> >  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
> >  drivers/thermal/thermal_netlink.h | 26 +++++++++
> >  include/net/genetlink.h           |  4 ++
> >  net/netlink/genetlink.c           | 30 ++++++++++
> >  5 files changed, 180 insertions(+), 15 deletions(-)
> >
> > --
> 
> What tree is this based on?

v5: https://patchwork.kernel.org/project/linux-pm/list/?series=829159
it's on top of linux-pm master, but require additional net dependency,
which can be added by:

git fetch https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git for-thermal-genetlink-family-bind-unbind-callbacks
git merge FETCH_HEAD

and will be merged mainline in the next merge window.
So at this point would be probably better just wait for 6.9-rc1 
when the dependency will be in the mainline, before applying this set.

Regards
Stanislaw
Rafael J. Wysocki Feb. 29, 2024, 4:24 p.m. UTC | #9
On Thu, Feb 29, 2024 at 5:13 PM Stanislaw Gruszka
<stanislaw.gruszka@linux.intel.com> wrote:
>
> On Thu, Feb 29, 2024 at 04:18:50PM +0100, Rafael J. Wysocki wrote:
> > On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> > <stanislaw.gruszka@linux.intel.com> wrote:
> > >
> > > The patchset introduces a new genetlink family bind/unbind callbacks
> > > and thermal/netlink notifications, which 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 thermal genl 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.
> > >
> > > Changes v3 -> v4:
> > >
> > > - Add 'static inline' in patch2
> > >
> > > Changes v2 -> v3:
> > >
> > > - Fix unused variable compilation warning
> > > - Add missed Suggested by tag to patch2
> > >
> > > Changes v1 -> v2:
> > >
> > > - Rewrite using netlink_bind/netlink_unbind callbacks.
> > >
> > > - Minor changelog tweaks.
> > >
> > > - Add missing check in intel hfi syscore resume (had it on my testing,
> > > but somehow missed in post).
> > >
> > > - Do not use netlink_has_listeners() any longer, use custom counter instead.
> > > To keep using netlink_has_listners() would be required to rearrange
> > > netlink_setsockopt() and possibly netlink_bind() functions, to call
> > > nlk->netlink_bind() after listeners are updated. So I decided to custom
> > > counter. This have potential issue as thermal netlink registers before
> > > intel_hif, so theoretically intel_hif can miss events. But since both
> > > are required to be kernel build-in (if CONFIG_INTEL_HFI_THERMAL is
> > > configured), they start before any user-space.
> > >
> > > v1: https://lore.kernel.org/linux-pm/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> > > v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> > > v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
> > >
> > > Stanislaw Gruszka (3):
> > >   genetlink: Add per family bind/unbind callbacks
> > >   thermal: netlink: Add genetlink bind/unbind notifications
> > >   thermal: intel: hfi: Enable interface only when required
> > >
> > >  drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
> > >  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
> > >  drivers/thermal/thermal_netlink.h | 26 +++++++++
> > >  include/net/genetlink.h           |  4 ++
> > >  net/netlink/genetlink.c           | 30 ++++++++++
> > >  5 files changed, 180 insertions(+), 15 deletions(-)
> > >
> > > --
> >
> > What tree is this based on?
>
> v5: https://patchwork.kernel.org/project/linux-pm/list/?series=829159
> it's on top of linux-pm master, but require additional net dependency,
> which can be added by:
>
> git fetch https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git for-thermal-genetlink-family-bind-unbind-callbacks
> git merge FETCH_HEAD
>
> and will be merged mainline in the next merge window.
> So at this point would be probably better just wait for 6.9-rc1
> when the dependency will be in the mainline, before applying this set.

And that's what's going to happen, unless I have any additional
comments on the patches.

Thanks!
Rafael J. Wysocki March 27, 2024, 1:53 p.m. UTC | #10
On Thu, Feb 29, 2024 at 5:24 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Thu, Feb 29, 2024 at 5:13 PM Stanislaw Gruszka
> <stanislaw.gruszka@linux.intel.com> wrote:
> >
> > On Thu, Feb 29, 2024 at 04:18:50PM +0100, Rafael J. Wysocki wrote:
> > > On Mon, Feb 12, 2024 at 5:16 PM Stanislaw Gruszka
> > > <stanislaw.gruszka@linux.intel.com> wrote:
> > > >
> > > > The patchset introduces a new genetlink family bind/unbind callbacks
> > > > and thermal/netlink notifications, which 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 thermal genl 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.
> > > >
> > > > Changes v3 -> v4:
> > > >
> > > > - Add 'static inline' in patch2
> > > >
> > > > Changes v2 -> v3:
> > > >
> > > > - Fix unused variable compilation warning
> > > > - Add missed Suggested by tag to patch2
> > > >
> > > > Changes v1 -> v2:
> > > >
> > > > - Rewrite using netlink_bind/netlink_unbind callbacks.
> > > >
> > > > - Minor changelog tweaks.
> > > >
> > > > - Add missing check in intel hfi syscore resume (had it on my testing,
> > > > but somehow missed in post).
> > > >
> > > > - Do not use netlink_has_listeners() any longer, use custom counter instead.
> > > > To keep using netlink_has_listners() would be required to rearrange
> > > > netlink_setsockopt() and possibly netlink_bind() functions, to call
> > > > nlk->netlink_bind() after listeners are updated. So I decided to custom
> > > > counter. This have potential issue as thermal netlink registers before
> > > > intel_hif, so theoretically intel_hif can miss events. But since both
> > > > are required to be kernel build-in (if CONFIG_INTEL_HFI_THERMAL is
> > > > configured), they start before any user-space.
> > > >
> > > > v1: https://lore.kernel.org/linux-pm/20240131120535.933424-1-stanislaw.gruszka@linux.intel.com//
> > > > v2: https://lore.kernel.org/linux-pm/20240206133605.1518373-1-stanislaw.gruszka@linux.intel.com/
> > > > v3: https://lore.kernel.org/linux-pm/20240209120625.1775017-1-stanislaw.gruszka@linux.intel.com/
> > > >
> > > > Stanislaw Gruszka (3):
> > > >   genetlink: Add per family bind/unbind callbacks
> > > >   thermal: netlink: Add genetlink bind/unbind notifications
> > > >   thermal: intel: hfi: Enable interface only when required
> > > >
> > > >  drivers/thermal/intel/intel_hfi.c | 95 +++++++++++++++++++++++++++----
> > > >  drivers/thermal/thermal_netlink.c | 40 +++++++++++--
> > > >  drivers/thermal/thermal_netlink.h | 26 +++++++++
> > > >  include/net/genetlink.h           |  4 ++
> > > >  net/netlink/genetlink.c           | 30 ++++++++++
> > > >  5 files changed, 180 insertions(+), 15 deletions(-)
> > > >
> > > > --
> > >
> > > What tree is this based on?
> >
> > v5: https://patchwork.kernel.org/project/linux-pm/list/?series=829159
> > it's on top of linux-pm master, but require additional net dependency,
> > which can be added by:
> >
> > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/kuba/linux.git for-thermal-genetlink-family-bind-unbind-callbacks
> > git merge FETCH_HEAD
> >
> > and will be merged mainline in the next merge window.
> > So at this point would be probably better just wait for 6.9-rc1
> > when the dependency will be in the mainline, before applying this set.
>
> And that's what's going to happen, unless I have any additional
> comments on the patches.

Now applied as 6.10 material (with a minor change in the subject of
the last patch) and I'm assuming that Srinivas likes this.