Re: [RFC PATCH v2 3/8] cpuidle: add a new predict interface

[Rafael J. Wysocki]

On Saturday, September 30, 2017 9:20:29 AM CEST Aubrey Li wrote:
> For the governor has predict functionality, add a new predict
> interface in cpuidle framework to call and use it.
> ---
>  drivers/cpuidle/cpuidle.c        | 34 ++++++++++++++++++++++++++++++++++
>  drivers/cpuidle/governors/menu.c |  7 +++++++
>  include/linux/cpuidle.h          |  3 +++
>  kernel/sched/idle.c              |  1 +
>  4 files changed, 45 insertions(+)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index 4066308..ef6f7dd 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -336,6 +336,40 @@ void cpuidle_entry_end(void)
>  }
>  
>  /**
> + * cpuidle_predict - predict whether the coming idle is a fast idle or not
> + */
> +void cpuidle_predict(void)
> +{
> +	struct cpuidle_device *dev = cpuidle_get_device();
> +	unsigned int overhead_threshold;
> +
> +	if (!dev)
> +		return;
> +
> +	overhead_threshold = dev->idle_stat.overhead;
> +
> +	if (cpuidle_curr_governor->predict) {
> +		dev->idle_stat.predicted_us = cpuidle_curr_governor->predict();
> +		/*
> +		 * notify idle governor to avoid reduplicative
> +		 * prediction computation
> +		 */
> +		dev->idle_stat.predicted = true;
> +		if (dev->idle_stat.predicted_us < overhead_threshold) {
> +			/*
> +			 * notify tick subsystem to keep ticking
> +			 * for the coming idle
> +			 */
> +			dev->idle_stat.fast_idle = true;
> +		} else
> +			dev->idle_stat.fast_idle = false;
> +	} else {
> +		dev->idle_stat.predicted = false;
> +		dev->idle_stat.fast_idle = false;
> +	}
> +}
> +
> +/**
>   * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
>   */
>  void cpuidle_install_idle_handler(void)
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 6bed197..90b2a10 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -344,6 +344,12 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	if (unlikely(latency_req == 0))
>  		return 0;
>  
> +	/*don't predict again if idle framework already did it */
> +	if (!dev->idle_stat.predicted)
> +		menu_predict();
> +	else
> +		dev->idle_stat.predicted = false;
> +
>  	if (CPUIDLE_DRIVER_STATE_START > 0) {
>  		struct cpuidle_state *s = &drv->states[CPUIDLE_DRIVER_STATE_START];
>  		unsigned int polling_threshold;
> @@ -518,6 +524,7 @@ static struct cpuidle_governor menu_governor = {
>  	.enable =	menu_enable_device,
>  	.select =	menu_select,
>  	.reflect =	menu_reflect,
> +	.predict =	menu_predict,
>  };
>  
>  /**
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index cad9b71..9ca0288 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -143,6 +143,7 @@ extern int cpuidle_select(struct cpuidle_driver *drv,
>  			  struct cpuidle_device *dev);
>  extern void cpuidle_entry_start(void);
>  extern void cpuidle_entry_end(void);
> +extern void cpuidle_predict(void);
>  extern int cpuidle_enter(struct cpuidle_driver *drv,
>  			 struct cpuidle_device *dev, int index);
>  extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
> @@ -178,6 +179,7 @@ static inline int cpuidle_select(struct cpuidle_driver *drv,
>  {return -ENODEV; }
>  static inline void cpuidle_entry_start(void) { }
>  static inline void cpuidle_entry_end(void) { }
> +static inline void cpuidle_predict(void) { }
>  static inline int cpuidle_enter(struct cpuidle_driver *drv,
>  				struct cpuidle_device *dev, int index)
>  {return -ENODEV; }
> @@ -255,6 +257,7 @@ struct cpuidle_governor {
>  	int  (*select)		(struct cpuidle_driver *drv,
>  					struct cpuidle_device *dev);
>  	void (*reflect)		(struct cpuidle_device *dev, int index);
> +	unsigned int (*predict)(void);
>  };
>  
>  #ifdef CONFIG_CPU_IDLE
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 0951dac..8704f3c 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -225,6 +225,7 @@ static void do_idle(void)
>  	 */
>  	__current_set_polling();
>  	quiet_vmstat();
> +	cpuidle_predict();

One more question here.

This changes the cpuidle code ordering such that if the ->predict callback
is present, the governor prediction will run before tick_nohz_idle_enter(),
whereas without that callback it runs in cpuidle_idle_call().

Is that actually going to work correctly for the menu governor, in particular?

>  	tick_nohz_idle_enter();
>  	cpuidle_entry_end();
>  
>