[PATCH 1/5] cpuidle: Remove pointless stub

[PATCH 1/5] cpuidle: Remove pointless stub

[Daniel Lezcano]

The cpuidle.h header is declaring functions with an empty stub when
cpuidle is not enabled. However these functions are only called from
the governors which depends on cpuidle. In other words, when the
function is called it is when cpuidle is enabled, there is no
situation when it is called with cpuidle disabled.

Remove the pointless stub.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/cpuidle.h | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 6175c77bf25e..74fdcc6106b1 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -270,13 +270,8 @@ struct cpuidle_governor {
 	void (*reflect)		(struct cpuidle_device *dev, int index);
 };
 
-#ifdef CONFIG_CPU_IDLE
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
 extern s64 cpuidle_governor_latency_req(unsigned int cpu);
-#else
-static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
-{return 0;}
-#endif
 
 #define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,			\
 				idx,					\
-- 
2.17.1

[PATCH 2/5] cpuidle: governor: Encapsulate the cpuidle on/off switch

[Daniel Lezcano]

The next patch will introduce the ability to unregister a governor
which will share part of the register function code. Instead of
duplicating, let's encapuslate the common parts into functions, so
they can be called from different places.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governor.c | 39 ++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index 29acaf48e575..d46ab8ec2dd7 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -39,6 +39,27 @@ struct cpuidle_governor *cpuidle_find_governor(const char *str)
 	return NULL;
 }
 
+static void cpuidle_switch_off(void)
+{
+	struct cpuidle_device *dev;
+
+	cpuidle_uninstall_idle_handler();
+
+	if (cpuidle_curr_governor) {
+		list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
+			cpuidle_disable_device(dev);
+	}
+}
+
+static void cpuidle_switch_on(void)
+{
+	struct cpuidle_device *dev;
+
+	list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
+		cpuidle_enable_device(dev);
+	cpuidle_install_idle_handler();
+}
+
 /**
  * cpuidle_switch_governor - changes the governor
  * @gov: the new target governor
@@ -46,29 +67,19 @@ struct cpuidle_governor *cpuidle_find_governor(const char *str)
  */
 int cpuidle_switch_governor(struct cpuidle_governor *gov)
 {
-	struct cpuidle_device *dev;
-
 	if (!gov)
 		return -EINVAL;
 
 	if (gov == cpuidle_curr_governor)
 		return 0;
 
-	cpuidle_uninstall_idle_handler();
-
-	if (cpuidle_curr_governor) {
-		list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
-			cpuidle_disable_device(dev);
-	}
+	cpuidle_switch_off();
 
 	cpuidle_curr_governor = gov;
 
-	if (gov) {
-		list_for_each_entry(dev, &cpuidle_detected_devices, device_list)
-			cpuidle_enable_device(dev);
-		cpuidle_install_idle_handler();
-		printk(KERN_INFO "cpuidle: using governor %s\n", gov->name);
-	}
+	cpuidle_switch_on();
+
+	printk(KERN_INFO "cpuidle: using governor %s\n", gov->name);
 
 	return 0;
 }
-- 
2.17.1

[PATCH 3/5] cpuidle: governor: Make possible to unregister a governor

[Daniel Lezcano]

This patch allows to unregister a governor. If the unregistered
governor is the current one, it will be replaced by the governor with
the highest rating. If it is the last governor, the cpuidle framework
will be switched off.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governor.c | 37 +++++++++++++++++++++++++++++++++++++
 include/linux/cpuidle.h    |  1 +
 2 files changed, 38 insertions(+)

diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index d46ab8ec2dd7..6ec27ef096f5 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -84,6 +84,43 @@ int cpuidle_switch_governor(struct cpuidle_governor *gov)
 	return 0;
 }
 
+/**
+ * cpuidle_unregister_governor - unregister a governor
+ * @gov: a pointer to a cpuidle governor structure
+ *
+ * Unregister the governor specified in parameter. If it is the
+ * current one, replace by another one in the list with the highest
+ * rating. If it is the last one, then switch off cpuidle.
+ */
+void cpuidle_unregister_governor(struct cpuidle_governor *gov)
+{
+	int rating = 0;
+	struct cpuidle_governor *new_gov = NULL;
+
+	mutex_lock(&cpuidle_lock);
+
+	list_del(&gov->governor_list);
+
+	/*
+	 * The governor is currently in use, switch to the one with
+	 * the best rating.
+	 */
+	if (cpuidle_curr_governor == gov) {
+
+		list_for_each_entry(gov, &cpuidle_governors, governor_list) {
+			if (gov->rating > rating)
+				new_gov = gov;
+		}
+
+		if (new_gov)
+			cpuidle_switch_governor(new_gov);
+		else
+			cpuidle_switch_off();
+	}
+
+	mutex_unlock(&cpuidle_lock);
+}
+
 /**
  * cpuidle_register_governor - registers a governor
  * @gov: the governor
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 74fdcc6106b1..457e0786b4f9 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -271,6 +271,7 @@ struct cpuidle_governor {
 };
 
 extern int cpuidle_register_governor(struct cpuidle_governor *gov);
+extern void cpuidle_unregister_governor(struct cpuidle_governor *gov);
 extern s64 cpuidle_governor_latency_req(unsigned int cpu);
 
 #define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,			\
-- 
2.17.1

[PATCH 4/5] cpuidle: governor: Export the needed symbols

[Daniel Lezcano]

In the next patch, the governors will be converted to modules. Export
the symbols of the different functions used by the governors.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governor.c | 3 +++
 include/linux/tick.h       | 2 ++
 kernel/sched/core.c        | 1 +
 kernel/time/tick-sched.c   | 9 +++++++++
 4 files changed, 15 insertions(+)

diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
index 6ec27ef096f5..2791fe352f51 100644
--- a/drivers/cpuidle/governor.c
+++ b/drivers/cpuidle/governor.c
@@ -120,6 +120,7 @@ void cpuidle_unregister_governor(struct cpuidle_governor *gov)
 
 	mutex_unlock(&cpuidle_lock);
 }
+EXPORT_SYMBOL(cpuidle_unregister_governor);
 
 /**
  * cpuidle_register_governor - registers a governor
@@ -150,6 +151,7 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
 
 	return ret;
 }
+EXPORT_SYMBOL(cpuidle_register_governor);
 
 /**
  * cpuidle_governor_latency_req - Compute a latency constraint for CPU
@@ -166,3 +168,4 @@ s64 cpuidle_governor_latency_req(unsigned int cpu)
 
 	return (s64)device_req * NSEC_PER_USEC;
 }
+EXPORT_SYMBOL(cpuidle_governor_latency_req);
diff --git a/include/linux/tick.h b/include/linux/tick.h
index 7340613c7eff..8349ba050b9c 100644
--- a/include/linux/tick.h
+++ b/include/linux/tick.h
@@ -124,6 +124,7 @@ enum tick_dep_bits {
 
 #ifdef CONFIG_NO_HZ_COMMON
 extern bool tick_nohz_enabled;
+extern bool tick_nohz_is_enabled(void);
 extern bool tick_nohz_tick_stopped(void);
 extern bool tick_nohz_tick_stopped_cpu(int cpu);
 extern void tick_nohz_idle_stop_tick(void);
@@ -149,6 +150,7 @@ static inline void tick_nohz_idle_stop_tick_protected(void)
 
 #else /* !CONFIG_NO_HZ_COMMON */
 #define tick_nohz_enabled (0)
+static inline int tick_nohz_is_enabled(void) { return 0 };
 static inline int tick_nohz_tick_stopped(void) { return 0; }
 static inline int tick_nohz_tick_stopped_cpu(int cpu) { return 0; }
 static inline void tick_nohz_idle_stop_tick(void) { }
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 2d95dc3f4644..ceba61bb364d 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3838,6 +3838,7 @@ unsigned long nr_iowait_cpu(int cpu)
 {
 	return atomic_read(&cpu_rq(cpu)->nr_iowait);
 }
+EXPORT_SYMBOL_GPL(nr_iowait_cpu);
 
 /*
  * IO-wait accounting, and how its mostly bollocks (on SMP).
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index f0199a4ba1ad..537716124d46 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -500,12 +500,19 @@ static int __init setup_tick_nohz(char *str)
 
 __setup("nohz=", setup_tick_nohz);
 
+bool tick_nohz_is_enabled(void)
+{
+	return tick_nohz_enabled;
+}
+EXPORT_SYMBOL_GPL(tick_nohz_is_enabled);
+
 bool tick_nohz_tick_stopped(void)
 {
 	struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
 
 	return ts->tick_stopped;
 }
+EXPORT_SYMBOL_GPL(tick_nohz_tick_stopped);
 
 bool tick_nohz_tick_stopped_cpu(int cpu)
 {
@@ -1066,6 +1073,7 @@ bool tick_nohz_idle_got_tick(void)
 	}
 	return false;
 }
+EXPORT_SYMBOL_GPL(tick_nohz_idle_got_tick);
 
 /**
  * tick_nohz_get_next_hrtimer - return the next expiration time for the hrtimer
@@ -1117,6 +1125,7 @@ ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
 
 	return ktime_sub(next_event, now);
 }
+EXPORT_SYMBOL_GPL(tick_nohz_get_sleep_length);
 
 /**
  * tick_nohz_get_idle_calls_cpu - return the current idle calls counter value
-- 
2.17.1

[PATCH 5/5] cpuidle: governor: Convert governors to modules

[Daniel Lezcano]

This patch converts the cpuidle governors into modules. Even if it is
not the utmost importance, that will be consistent with the devfreq,
the watchdog and the cpufreq governors.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/Kconfig              |  8 ++++----
 drivers/cpuidle/governors/haltpoll.c |  9 ++++++++-
 drivers/cpuidle/governors/ladder.c   | 12 ++++++++++--
 drivers/cpuidle/governors/menu.c     | 10 +++++++++-
 drivers/cpuidle/governors/teo.c      | 10 +++++++++-
 5 files changed, 40 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd66f02..ff83042edd50 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -19,13 +19,13 @@ config CPU_IDLE_MULTIPLE_DRIVERS
 	bool
 
 config CPU_IDLE_GOV_LADDER
-	bool "Ladder governor (for periodic timer tick)"
+	tristate "Ladder governor (for periodic timer tick)"
 
 config CPU_IDLE_GOV_MENU
-	bool "Menu governor (for tickless system)"
+	tristate "Menu governor (for tickless system)"
 
 config CPU_IDLE_GOV_TEO
-	bool "Timer events oriented (TEO) governor (for tickless systems)"
+	tristate "Timer events oriented (TEO) governor (for tickless systems)"
 	help
 	  This governor implements a simplified idle state selection method
 	  focused on timer events and does not do any interactivity boosting.
@@ -34,7 +34,7 @@ config CPU_IDLE_GOV_TEO
 	  to use.  Say Y here if you are not happy with the alternatives.
 
 config CPU_IDLE_GOV_HALTPOLL
-	bool "Haltpoll governor (for virtualized systems)"
+	tristate "Haltpoll governor (for virtualized systems)"
 	depends on KVM_GUEST
 	help
 	  This governor implements haltpoll idle state selection, to be
diff --git a/drivers/cpuidle/governors/haltpoll.c b/drivers/cpuidle/governors/haltpoll.c
index cb2a96eafc02..4756b758c324 100644
--- a/drivers/cpuidle/governors/haltpoll.c
+++ b/drivers/cpuidle/governors/haltpoll.c
@@ -146,4 +146,11 @@ static int __init init_haltpoll(void)
 	return 0;
 }
 
-postcore_initcall(init_haltpoll);
+static void __exit exit_haltpoll(void)
+{
+	cpuidle_unregister_governor(&haltpoll_governor);
+}
+
+module_init(init_haltpoll);
+module_exit(exit_haltpoll);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
index 8e9058c4ea63..78b622b75ce7 100644
--- a/drivers/cpuidle/governors/ladder.c
+++ b/drivers/cpuidle/governors/ladder.c
@@ -14,6 +14,7 @@
 
 #include <linux/kernel.h>
 #include <linux/cpuidle.h>
+#include <linux/module.h>
 #include <linux/jiffies.h>
 #include <linux/tick.h>
 
@@ -188,10 +189,17 @@ static int __init init_ladder(void)
 	 * governor is better so give it a higher rating than the menu
 	 * governor.
 	 */
-	if (!tick_nohz_enabled)
+	if (!tick_nohz_is_enabled())
 		ladder_governor.rating = 25;
 
 	return cpuidle_register_governor(&ladder_governor);
 }
 
-postcore_initcall(init_ladder);
+static void __exit exit_ladder(void)
+{
+	cpuidle_unregister_governor(&ladder_governor);
+}
+
+module_init(init_ladder);
+module_exit(exit_ladder);
+MODULE_LICENSE("GPL");
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index b0a7ad566081..fc92a5b18a7b 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -13,6 +13,7 @@
 #include <linux/time.h>
 #include <linux/ktime.h>
 #include <linux/hrtimer.h>
+#include <linux/module.h>
 #include <linux/tick.h>
 #include <linux/sched.h>
 #include <linux/sched/loadavg.h>
@@ -571,4 +572,11 @@ static int __init init_menu(void)
 	return cpuidle_register_governor(&menu_governor);
 }
 
-postcore_initcall(init_menu);
+static void __exit exit_menu(void)
+{
+	cpuidle_unregister_governor(&menu_governor);
+}
+
+module_init(init_menu);
+module_exit(exit_menu);
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index 6deaaf5f05b5..d11dab61113c 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -48,6 +48,7 @@
 #include <linux/cpuidle.h>
 #include <linux/jiffies.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
 #include <linux/sched/clock.h>
 #include <linux/tick.h>
 
@@ -491,4 +492,11 @@ static int __init teo_governor_init(void)
 	return cpuidle_register_governor(&teo_governor);
 }
 
-postcore_initcall(teo_governor_init);
+static void __exit teo_governor_exit(void)
+{
+	cpuidle_unregister_governor(&teo_governor);
+}
+
+module_init(teo_governor_init);
+module_exit(teo_governor_exit);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

Re: [PATCH 1/5] cpuidle: Remove pointless stub

[Rafael J. Wysocki]

On Thu, Oct 15, 2020 at 4:44 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> The cpuidle.h header is declaring functions with an empty stub when
> cpuidle is not enabled. However these functions are only called from
> the governors which depends on cpuidle. In other words, when the
> function is called it is when cpuidle is enabled, there is no
> situation when it is called with cpuidle disabled.
>
> Remove the pointless stub.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  include/linux/cpuidle.h | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 6175c77bf25e..74fdcc6106b1 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -270,13 +270,8 @@ struct cpuidle_governor {
>         void (*reflect)         (struct cpuidle_device *dev, int index);
>  };
>
> -#ifdef CONFIG_CPU_IDLE
>  extern int cpuidle_register_governor(struct cpuidle_governor *gov);
>  extern s64 cpuidle_governor_latency_req(unsigned int cpu);
> -#else
> -static inline int cpuidle_register_governor(struct cpuidle_governor *gov)
> -{return 0;}
> -#endif
>
>  #define __CPU_PM_CPU_IDLE_ENTER(low_level_idle_enter,                  \
>                                 idx,                                    \
> --

Applied (this patch alone) as 5.10-rc material with some minor edits
in the changelog, thanks!

Re: [PATCH 1/5] cpuidle: Remove pointless stub

[Daniel Lezcano]

Hi Rafael,

On 16/10/2020 17:24, Rafael J. Wysocki wrote:
> On Thu, Oct 15, 2020 at 4:44 PM Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>>
>> The cpuidle.h header is declaring functions with an empty stub when
>> cpuidle is not enabled. However these functions are only called from
>> the governors which depends on cpuidle. In other words, when the
>> function is called it is when cpuidle is enabled, there is no
>> situation when it is called with cpuidle disabled.
>>
>> Remove the pointless stub.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---

[ ... ]

> Applied (this patch alone) as 5.10-rc material with some minor edits
> in the changelog, thanks!

Does it mean you disagree the other patches? Or are you waiting for more
comments?


-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/5] cpuidle: governor: Export the needed symbols

[Rafael J. Wysocki]

On Thu, Oct 15, 2020 at 4:45 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> In the next patch, the governors will be converted to modules. Export
> the symbols of the different functions used by the governors.
>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/cpuidle/governor.c | 3 +++
>  include/linux/tick.h       | 2 ++
>  kernel/sched/core.c        | 1 +
>  kernel/time/tick-sched.c   | 9 +++++++++
>  4 files changed, 15 insertions(+)
>
> diff --git a/drivers/cpuidle/governor.c b/drivers/cpuidle/governor.c
> index 6ec27ef096f5..2791fe352f51 100644
> --- a/drivers/cpuidle/governor.c
> +++ b/drivers/cpuidle/governor.c
> @@ -120,6 +120,7 @@ void cpuidle_unregister_governor(struct cpuidle_governor *gov)
>
>         mutex_unlock(&cpuidle_lock);
>  }
> +EXPORT_SYMBOL(cpuidle_unregister_governor);
>
>  /**
>   * cpuidle_register_governor - registers a governor
> @@ -150,6 +151,7 @@ int cpuidle_register_governor(struct cpuidle_governor *gov)
>
>         return ret;
>  }
> +EXPORT_SYMBOL(cpuidle_register_governor);
>
>  /**
>   * cpuidle_governor_latency_req - Compute a latency constraint for CPU
> @@ -166,3 +168,4 @@ s64 cpuidle_governor_latency_req(unsigned int cpu)
>
>         return (s64)device_req * NSEC_PER_USEC;
>  }
> +EXPORT_SYMBOL(cpuidle_governor_latency_req);
> diff --git a/include/linux/tick.h b/include/linux/tick.h
> index 7340613c7eff..8349ba050b9c 100644
> --- a/include/linux/tick.h
> +++ b/include/linux/tick.h
> @@ -124,6 +124,7 @@ enum tick_dep_bits {
>
>  #ifdef CONFIG_NO_HZ_COMMON
>  extern bool tick_nohz_enabled;
> +extern bool tick_nohz_is_enabled(void);
>  extern bool tick_nohz_tick_stopped(void);
>  extern bool tick_nohz_tick_stopped_cpu(int cpu);
>  extern void tick_nohz_idle_stop_tick(void);
> @@ -149,6 +150,7 @@ static inline void tick_nohz_idle_stop_tick_protected(void)
>
>  #else /* !CONFIG_NO_HZ_COMMON */
>  #define tick_nohz_enabled (0)
> +static inline int tick_nohz_is_enabled(void) { return 0 };
>  static inline int tick_nohz_tick_stopped(void) { return 0; }
>  static inline int tick_nohz_tick_stopped_cpu(int cpu) { return 0; }
>  static inline void tick_nohz_idle_stop_tick(void) { }
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 2d95dc3f4644..ceba61bb364d 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3838,6 +3838,7 @@ unsigned long nr_iowait_cpu(int cpu)
>  {
>         return atomic_read(&cpu_rq(cpu)->nr_iowait);
>  }
> +EXPORT_SYMBOL_GPL(nr_iowait_cpu);

Hmm.  See below.

>
>  /*
>   * IO-wait accounting, and how its mostly bollocks (on SMP).
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index f0199a4ba1ad..537716124d46 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -500,12 +500,19 @@ static int __init setup_tick_nohz(char *str)
>
>  __setup("nohz=", setup_tick_nohz);
>
> +bool tick_nohz_is_enabled(void)
> +{
> +       return tick_nohz_enabled;
> +}
> +EXPORT_SYMBOL_GPL(tick_nohz_is_enabled);
> +
>  bool tick_nohz_tick_stopped(void)
>  {
>         struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
>
>         return ts->tick_stopped;
>  }
> +EXPORT_SYMBOL_GPL(tick_nohz_tick_stopped);
>
>  bool tick_nohz_tick_stopped_cpu(int cpu)
>  {
> @@ -1066,6 +1073,7 @@ bool tick_nohz_idle_got_tick(void)
>         }
>         return false;
>  }
> +EXPORT_SYMBOL_GPL(tick_nohz_idle_got_tick);
>
>  /**
>   * tick_nohz_get_next_hrtimer - return the next expiration time for the hrtimer
> @@ -1117,6 +1125,7 @@ ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
>
>         return ktime_sub(next_event, now);
>  }
> +EXPORT_SYMBOL_GPL(tick_nohz_get_sleep_length);

Peter please correct me if I'm mistaken, but IMV the above are core
kernel internals and they should not be accessible to random modular
stuff.

>
>  /**
>   * tick_nohz_get_idle_calls_cpu - return the current idle calls counter value
> --

Re: [PATCH 1/5] cpuidle: Remove pointless stub

[Rafael J. Wysocki]

On Fri, Oct 16, 2020 at 10:31 PM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
>
> Hi Rafael,
>
> On 16/10/2020 17:24, Rafael J. Wysocki wrote:
> > On Thu, Oct 15, 2020 at 4:44 PM Daniel Lezcano
> > <daniel.lezcano@linaro.org> wrote:
> >>
> >> The cpuidle.h header is declaring functions with an empty stub when
> >> cpuidle is not enabled. However these functions are only called from
> >> the governors which depends on cpuidle. In other words, when the
> >> function is called it is when cpuidle is enabled, there is no
> >> situation when it is called with cpuidle disabled.
> >>
> >> Remove the pointless stub.
> >>
> >> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> >> ---
>
> [ ... ]
>
> > Applied (this patch alone) as 5.10-rc material with some minor edits
> > in the changelog, thanks!
>
> Does it mean you disagree the other patches? Or are you waiting for more
> comments?

Actually, both.

My primary concern regarding the modularization of cpuidle governors
is that they really belong to the core kernel.  They access the
internals thereof and the behavior of the idle loop in general depends
on what they do.

OTOH IMV a potential benefit resulting from allowing them to be
modular is very small, at least from the mainline perspective.

IOW this case is very similar to the modularization of the schedutil
cpufreq governor which we decided not to do for similar reasons.

Re: [PATCH 1/5] cpuidle: Remove pointless stub

[Daniel Lezcano]

Hi Rafael,

On 05/11/2020 15:14, Rafael J. Wysocki wrote:

[ ... ]

>> [ ... ]
>>
>>> Applied (this patch alone) as 5.10-rc material with some minor edits
>>> in the changelog, thanks!
>>
>> Does it mean you disagree the other patches? Or are you waiting for more
>> comments?
> 
> Actually, both.
> 
> My primary concern regarding the modularization of cpuidle governors
> is that they really belong to the core kernel.  They access the
> internals thereof and the behavior of the idle loop in general depends
> on what they do.
> 
> OTOH IMV a potential benefit resulting from allowing them to be
> modular is very small, at least from the mainline perspective.
> 
> IOW this case is very similar to the modularization of the schedutil
> cpufreq governor which we decided not to do for similar reasons.

Yes, I recall this discussion.

The purpose is to be more consistent with the governors in the other
frameworks which can be module. But as you mentioned it, the benefit is
small, so I'm fine to drop the series.

Thanks

  -- Daniel

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH 4/5] cpuidle: governor: Export the needed symbols

[Peter Zijlstra]

On Thu, Nov 05, 2020 at 03:04:10PM +0100, Rafael J. Wysocki wrote:
> > diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> > index 2d95dc3f4644..ceba61bb364d 100644
> > --- a/kernel/sched/core.c
> > +++ b/kernel/sched/core.c
> > @@ -3838,6 +3838,7 @@ unsigned long nr_iowait_cpu(int cpu)
> >  {
> >         return atomic_read(&cpu_rq(cpu)->nr_iowait);
> >  }
> > +EXPORT_SYMBOL_GPL(nr_iowait_cpu);
> 
> Hmm.  See below.

Did anyone read the comment above this function? It's garbage, it should
be deleted, not made available to a wider audience.

> >  /*
> >   * IO-wait accounting, and how its mostly bollocks (on SMP).
> > diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> > index f0199a4ba1ad..537716124d46 100644
> > --- a/kernel/time/tick-sched.c
> > +++ b/kernel/time/tick-sched.c
> > @@ -500,12 +500,19 @@ static int __init setup_tick_nohz(char *str)
> >
> >  __setup("nohz=", setup_tick_nohz);
> >
> > +bool tick_nohz_is_enabled(void)
> > +{
> > +       return tick_nohz_enabled;
> > +}
> > +EXPORT_SYMBOL_GPL(tick_nohz_is_enabled);
> > +
> >  bool tick_nohz_tick_stopped(void)
> >  {
> >         struct tick_sched *ts = this_cpu_ptr(&tick_cpu_sched);
> >
> >         return ts->tick_stopped;
> >  }
> > +EXPORT_SYMBOL_GPL(tick_nohz_tick_stopped);
> >
> >  bool tick_nohz_tick_stopped_cpu(int cpu)
> >  {
> > @@ -1066,6 +1073,7 @@ bool tick_nohz_idle_got_tick(void)
> >         }
> >         return false;
> >  }
> > +EXPORT_SYMBOL_GPL(tick_nohz_idle_got_tick);
> >
> >  /**
> >   * tick_nohz_get_next_hrtimer - return the next expiration time for the hrtimer
> > @@ -1117,6 +1125,7 @@ ktime_t tick_nohz_get_sleep_length(ktime_t *delta_next)
> >
> >         return ktime_sub(next_event, now);
> >  }
> > +EXPORT_SYMBOL_GPL(tick_nohz_get_sleep_length);
> 
> Peter please correct me if I'm mistaken, but IMV the above are core
> kernel internals and they should not be accessible to random modular
> stuff.

Yeah,... making this available seems unfortunate. Also, I don't really
see the point, why do we want the idle governors as modules? On the
cpufreq side we're trying to move away from modules and multiple
governors.