intel_idle: cleanup code for handling with cpuidle framework

PATCH In review Zhang Rui 2 participants 9 messages Oct 27, 2021 – Dec 7, 2021 Intel idle driver

lore

Share & tools

lore

Zhang Rui

author

2021-10-27 08:07 UTC #0 lore raw

PATCH 1/3 intel_idle: cleanup code for handling with cpuidle framework

Introduce two helper functions and move all the code that deals with the cpuidle framework into them.

No functional change in this patch.

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/idle/intel_idle.c | 67 ++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 29 deletions(-)

drivers/idle/intel_idle.c +38 ~~-29~~

index e6c543b5ee1d..ae9d8c43e6a5 100644
@@ -1677,14 +1677,46 @@ static int intel_idle_cpu_online(unsigned int cpu)
 }
 
 /**
- * intel_idle_cpuidle_devices_uninit - Unregister all cpuidle devices.
+ * intel_idle_cpuidle_unregister - unregister from cpuidle framework
  */
-static void __init intel_idle_cpuidle_devices_uninit(void)
+static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 {
 	int i;
 
 	for_each_online_cpu(i)
 		cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i));
+	cpuidle_unregister_driver(drv);
+	free_percpu(intel_idle_cpuidle_devices);
+}
+
+/**
+ * intel_idle_cpuidle_register - register to cpuidle framework
+ */
+static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv)
+{
+	int retval;
+
+	intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+	if (!intel_idle_cpuidle_devices)
+		return -ENOMEM;
+
+	retval = cpuidle_register_driver(drv);
+	if (retval) {
+		struct cpuidle_driver *drv = cpuidle_get_driver();
+
+		printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"),
+			drv ? drv->name : "none");
+		free_percpu(intel_idle_cpuidle_devices);
+		return retval;
+	}
+
+	retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online",
+				intel_idle_cpu_online, NULL);
+	if (retval < 0) {
+		intel_idle_cpuidle_unregister(drv);
+		return retval;
+	}
+	return 0;
 }
 
 static int __init intel_idle_init(void)

@@ -1740,37 +1772,14 @@ static int __init intel_idle_init(void)
 	pr_debug("v" INTEL_IDLE_VERSION " model 0x%X\n",
 		 boot_cpu_data.x86_model);
 
-	intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
-	if (!intel_idle_cpuidle_devices)
-		return -ENOMEM;
-
 	intel_idle_cpuidle_driver_init(&intel_idle_driver);
 
-	retval = cpuidle_register_driver(&intel_idle_driver);
-	if (retval) {
-		struct cpuidle_driver *drv = cpuidle_get_driver();
-		printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"),
-		       drv ? drv->name : "none");
-		goto init_driver_fail;
-	}
-
-	retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online",
-				   intel_idle_cpu_online, NULL);
-	if (retval < 0)
-		goto hp_setup_fail;
-
-	pr_debug("Local APIC timer is reliable in %s\n",
-		 boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1");
+	retval = intel_idle_cpuidle_register(&intel_idle_driver);
+	if (!retval)
+		pr_debug("Local APIC timer is reliable in %s\n",
+			boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1");
 
-	return 0;
-
-hp_setup_fail:
-	intel_idle_cpuidle_devices_uninit();
-	cpuidle_unregister_driver(&intel_idle_driver);
-init_driver_fail:
-	free_percpu(intel_idle_cpuidle_devices);
 	return retval;
-
 }
 device_initcall(intel_idle_init);

Signature · 1 line

2.17.1

Zhang Rui

author

2021-10-27 08:07 UTC #1 lore raw

PATCH 2/3 intel_idle: cleanup cpuhotplug setup

Only limited number of CPUHP_AP_ONLINE_DYN callbacks can be registered, thus cpuhp_remove_state() should be invoked to release the resource when it is not used.

Fixes fb1013a01673 ("intel_idle: Convert to hotplug state machine")

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/idle/intel_idle.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

drivers/idle/intel_idle.c +8 -4

index ae9d8c43e6a5..e7f2a5f85bf9 100644
@@ -1676,6 +1676,8 @@ static int intel_idle_cpu_online(unsigned int cpu)
 	return 0;
 }
 
+static enum cpuhp_state intel_idle_cpuhp_state;
+
 /**
  * intel_idle_cpuidle_unregister - unregister from cpuidle framework
  */

@@ -1683,6 +1685,8 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 {
 	int i;
 
+	if (intel_idle_cpuhp_state > 0)
+		cpuhp_remove_state(intel_idle_cpuhp_state);
 	for_each_online_cpu(i)
 		cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i));
 	cpuidle_unregister_driver(drv);

@@ -1710,11 +1714,11 @@ static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv)
 		return retval;
 	}
 
-	retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online",
-				intel_idle_cpu_online, NULL);
-	if (retval < 0) {
+	intel_idle_cpuhp_state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+		"idle/intel:online", intel_idle_cpu_online, NULL);
+	if (intel_idle_cpuhp_state < 0) {
 		intel_idle_cpuidle_unregister(drv);
-		return retval;
+		return intel_idle_cpuhp_state;
 	}
 	return 0;
 }

Signature · 1 line

2.17.1

Zhang Rui

author

2021-12-07 06:22 UTC #3 lore raw

Re: [PATCH 2/3] intel_idle: cleanup cpuhotplug setup

Quoted 36 lines · Rafael J. Wysocki

Quoted 34 lines · Zhang Rui
Only limited number of CPUHP_AP_ONLINE_DYN callbacks can be registered,
thus cpuhp_remove_state() should be invoked to release the resource when
it is not used.
Fixes fb1013a01673 ("intel_idle: Convert to hotplug state
machine")
Signed-off-by Zhang Rui <[email protected]>
---
 drivers/idle/intel_idle.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)
drivers/idle/intel_idle.c +2 -0
index ae9d8c43e6a5..e7f2a5f85bf9 100644
@@ -1676,6 +1676,8 @@ static int intel_idle_cpu_online(unsigned int
 cpu)
        return 0;
 }
 
+static enum cpuhp_state intel_idle_cpuhp_state;
+
 /**
  * intel_idle_cpuidle_unregister - unregister from cpuidle
framework
*/
drivers/idle/intel_idle.c +0 -0
@@ -1683,6 +1685,8 @@ static void __init
 intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 {
        int i;
 

Quoted 6 lines

drivers/idle/intel_idle.c static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) · line 1689
Quoted 2 lines
drivers/idle/intel_idle.c +2 -0
+       if (intel_idle_cpuhp_state > 0)
+               cpuhp_remove_state(intel_idle_cpuhp_state);
It would be more straightforward to do that directly in intel_idle_init(), because intel_idle_cpuhp_state could be a local variable in that function then.

I rechecked the code.
in intel_idle_init(), cpuhp_setup_state() is the last step, so either it succeeds and we never unload the driver, or it fails and the intel_idle_init() fails directly.
So nothing needs to be fixed if we don't support intel_idle driver module unload.

thanks,
rui

Quoted 27 lines

drivers/idle/intel_idle.c +0 -0

        for_each_online_cpu(i)
                cpuidle_unregister_device(per_cpu_ptr(intel_idle_cp

uidle_devices, i));
        cpuidle_unregister_driver(drv);

drivers/idle/intel_idle.c +4 -4

@@ -1710,11 +1714,11 @@ static int __init
 intel_idle_cpuidle_register(struct cpuidle_driver *drv)
                return retval;
        }
 
-       retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
 "idle/intel:online",
-                               intel_idle_cpu_online, NULL);
-       if (retval < 0) {
+       intel_idle_cpuhp_state =
 cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
+               "idle/intel:online", intel_idle_cpu_online, NULL);
+       if (intel_idle_cpuhp_state < 0) {
                intel_idle_cpuidle_unregister(drv);
-               return retval;
+               return intel_idle_cpuhp_state;

        }
        return 0;

}
--
2.17.1

Zhang Rui

author

2021-10-27 08:07 UTC #4 lore raw

PATCH 3/3 intel_idle: fix cpuidle_device unregistration

In reply to Zhang Rui

cpuidle_device is allocated as percpu data, and it is registered for every CPU that has ever been onlined.
When unregistering, checking current online CPUs is not sufficient, because some cpu may be offlined later with its cpuidle_device registered.

Fix this by using for_each_present_cpu() instead, and unregistering all the cpuidle_devices that have been registered.

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/idle/intel_idle.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

drivers/idle/intel_idle.c +7 -2

index e7f2a5f85bf9..9e916e2adc89 100644
@@ -1687,8 +1687,13 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 
 	if (intel_idle_cpuhp_state > 0)
 		cpuhp_remove_state(intel_idle_cpuhp_state);
-	for_each_online_cpu(i)
-		cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i));
+	for_each_present_cpu(i) {
+		struct cpuidle_device *dev;
+
+		dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
+		if (dev->registered)
+			cpuidle_unregister_device(dev);
+	}
 	cpuidle_unregister_driver(drv);
 	free_percpu(intel_idle_cpuidle_devices);
 }

Signature · 1 line

2.17.1

Rafael J. Wysocki

maintainer

2021-11-24 13:22 UTC #5 lore raw

Re: [PATCH 3/3] intel_idle: fix cpuidle_device unregistration

Quoted 5 lines · Zhang Rui

cpuidle_device is allocated as percpu data, and it is registered for every CPU that has ever been onlined.
When unregistering, checking current online CPUs is not sufficient, because some cpu may be offlined later with its cpuidle_device registered.

But the unregistration happens only in the error code path of intel_idle_init(), doesn't it?

While I agree that doing a for_each_present_cpu() walk for that is more prudent', I'm not sure if that makes any difference in practice.

Quoted 13 lines

Fix this by using for_each_present_cpu() instead, and unregistering all the cpuidle_devices that have been registered.
Signed-off-by Zhang Rui <[email protected]>
---
 drivers/idle/intel_idle.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/idle/intel_idle.c +0 -0
index e7f2a5f85bf9..9e916e2adc89 100644

drivers/idle/intel_idle.c static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) · line 1694

Quoted 10 lines

drivers/idle/intel_idle.c +5 -2

@@ -1687,8 +1687,13 @@ static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 
        if (intel_idle_cpuhp_state > 0)
                cpuhp_remove_state(intel_idle_cpuhp_state);
-       for_each_online_cpu(i)
-               cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i));
+       for_each_present_cpu(i) {
+               struct cpuidle_device *dev;
+
+               dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
+               if (dev->registered)

dev->registered is checked by cpuidle_unregister_device().

Quoted 6 lines

drivers/idle/intel_idle.c +2 -1

+                       cpuidle_unregister_device(dev);
+       }
        cpuidle_unregister_driver(drv);
        free_percpu(intel_idle_cpuidle_devices);
 }
--

Zhang Rui

author

2021-11-24 13:46 UTC #6 lore raw

Re: [PATCH 3/3] intel_idle: fix cpuidle_device unregistration

Quoted 10 lines · Rafael J. Wysocki

Quoted 7 lines · Zhang Rui
cpuidle_device is allocated as percpu data, and it is registered for every
CPU that has ever been onlined.
When unregistering, checking current online CPUs is not sufficient, because some cpu may be offlined later with its cpuidle_device registered.

Quoted 2 lines

But the unregistration happens only in the error code path of intel_idle_init(), doesn't it?

yes.

Quoted 3 lines

While I agree that doing a for_each_present_cpu() walk for that is more prudent', I'm not sure if that makes any difference in practice.

And yes, exactly.
This is not a problem as long as intel_idle driver can not be unloaded.

There is no technical gap either to unregister the intel_idle cpuidle driver, or to unload the intel_idle module. And this potential issue will be exposed only when we decided to do so.

If you prefer to describe this *potential* issue more precisely, I totally agree.
If you want to fix it only when really needed, that's also okay to me.

Quoted 17 lines

Quoted 17 lines
Fix this by using for_each_present_cpu() instead, and unregistering all
the cpuidle_devices that have been registered.
Signed-off-by Zhang Rui <[email protected]>
---
 drivers/idle/intel_idle.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)
drivers/idle/intel_idle.c +0 -0
index e7f2a5f85bf9..9e916e2adc89 100644
@@ -1687,8 +1687,13 @@ static void __init
 intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 

Quoted 12 lines

drivers/idle/intel_idle.c static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv) · line 1694

Quoted 10 lines

drivers/idle/intel_idle.c +5 -2

        if (intel_idle_cpuhp_state > 0)
                cpuhp_remove_state(intel_idle_cpuhp_state);
-       for_each_online_cpu(i)
-               cpuidle_unregister_device(per_cpu_ptr(intel_idle_cp
 uidle_devices, i));
+       for_each_present_cpu(i) {
+               struct cpuidle_device *dev;
+
+               dev = per_cpu_ptr(intel_idle_cpuidle_devices, i);
+               if (dev->registered)

dev->registered is checked by cpuidle_unregister_device().

right, this check is not needed.

thanks,
rui

Quoted 7 lines

drivers/idle/intel_idle.c +2 -0

+                       cpuidle_unregister_device(dev);
+       }
        cpuidle_unregister_driver(drv);

        free_percpu(intel_idle_cpuidle_devices);

}
--

Rafael J. Wysocki

maintainer

2021-11-24 13:59 UTC #7 lore raw

Re: [PATCH 3/3] intel_idle: fix cpuidle_device unregistration

Quoted 23 lines · Zhang Rui

Quoted 10 lines · Rafael J. Wysocki
Quoted 7 lines · Zhang Rui
cpuidle_device is allocated as percpu data, and it is registered for every
CPU that has ever been onlined.
When unregistering, checking current online CPUs is not sufficient, because some cpu may be offlined later with its cpuidle_device registered.
Quoted 2 lines
But the unregistration happens only in the error code path of intel_idle_init(), doesn't it?
yes.
Quoted 3 lines
While I agree that doing a for_each_present_cpu() walk for that is more prudent', I'm not sure if that makes any difference in practice.
And yes, exactly.
This is not a problem as long as intel_idle driver can not be unloaded.

Quoted 7 lines

There is no technical gap either to unregister the intel_idle cpuidle driver, or to unload the intel_idle module. And this potential issue will be exposed only when we decided to do so.
If you prefer to describe this *potential* issue more precisely, I totally agree.
If you want to fix it only when really needed, that's also okay to me.

I would prefer to fix it first and then extend as needed.

Rafael J. Wysocki

maintainer

2021-11-24 13:27 UTC #8 lore raw

In reply to Zhang Rui

Quoted 3 lines · Zhang Rui

Introduce two helper functions and move all the code that deals with the cpuidle framework into them.

So what exactly is the benefit?

The role of intel_idle_cpuidle_devices_uninit() was to contain the loop over CPUs and now it is combined with the driver unregistration which looks confusing to me, because this function is only called in one place in the error patch of intel_idle_init().

The existing code flow is easier to follow for me TBH and it is fewer lines of code.

Quoted 105 lines

No functional change in this patch.

Signed-off-by Zhang Rui <[email protected]>

---
 drivers/idle/intel_idle.c | 67 ++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 29 deletions(-)

drivers/idle/intel_idle.c +38 ~~-30~~

index e6c543b5ee1d..ae9d8c43e6a5 100644
@@ -1677,14 +1677,46 @@ static int intel_idle_cpu_online(unsigned int cpu)
 }
 
 /**
- * intel_idle_cpuidle_devices_uninit - Unregister all cpuidle devices.
+ * intel_idle_cpuidle_unregister - unregister from cpuidle framework
  */
-static void __init intel_idle_cpuidle_devices_uninit(void)
+static void __init intel_idle_cpuidle_unregister(struct cpuidle_driver *drv)
 {
        int i;
 
        for_each_online_cpu(i)
                cpuidle_unregister_device(per_cpu_ptr(intel_idle_cpuidle_devices, i));
+       cpuidle_unregister_driver(drv);
+       free_percpu(intel_idle_cpuidle_devices);
+}
+
+/**
+ * intel_idle_cpuidle_register - register to cpuidle framework
+ */
+static int __init intel_idle_cpuidle_register(struct cpuidle_driver *drv)
+{
+       int retval;
+
+       intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
+       if (!intel_idle_cpuidle_devices)
+               return -ENOMEM;
+
+       retval = cpuidle_register_driver(drv);
+       if (retval) {
+               struct cpuidle_driver *drv = cpuidle_get_driver();
+
+               printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"),
+                       drv ? drv->name : "none");
+               free_percpu(intel_idle_cpuidle_devices);
+               return retval;
+       }
+
+       retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online",
+                               intel_idle_cpu_online, NULL);
+       if (retval < 0) {
+               intel_idle_cpuidle_unregister(drv);
+               return retval;
+       }
+       return 0;
 }
 
 static int __init intel_idle_init(void)

@@ -1740,37 +1772,14 @@ static int __init intel_idle_init(void)
        pr_debug("v" INTEL_IDLE_VERSION " model 0x%X\n",
                 boot_cpu_data.x86_model);
 
-       intel_idle_cpuidle_devices = alloc_percpu(struct cpuidle_device);
-       if (!intel_idle_cpuidle_devices)
-               return -ENOMEM;
-
        intel_idle_cpuidle_driver_init(&intel_idle_driver);
 
-       retval = cpuidle_register_driver(&intel_idle_driver);
-       if (retval) {
-               struct cpuidle_driver *drv = cpuidle_get_driver();
-               printk(KERN_DEBUG pr_fmt("intel_idle yielding to %s\n"),
-                      drv ? drv->name : "none");
-               goto init_driver_fail;
-       }
-
-       retval = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "idle/intel:online",
-                                  intel_idle_cpu_online, NULL);
-       if (retval < 0)
-               goto hp_setup_fail;
-
-       pr_debug("Local APIC timer is reliable in %s\n",
-                boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1");
+       retval = intel_idle_cpuidle_register(&intel_idle_driver);
+       if (!retval)
+               pr_debug("Local APIC timer is reliable in %s\n",
+                       boot_cpu_has(X86_FEATURE_ARAT) ? "all C-states" : "C1");
 
-       return 0;
-
-hp_setup_fail:
-       intel_idle_cpuidle_devices_uninit();
-       cpuidle_unregister_driver(&intel_idle_driver);
-init_driver_fail:
-       free_percpu(intel_idle_cpuidle_devices);
        return retval;
-
 }
 device_initcall(intel_idle_init);
 
--

2.17.1