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.

Care to describe how it is intended to work?

Also this patch uses data structures introduced in the previous one
(and the previous one didn't use them), so maybe merge the two?

> ---
>  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
> +		 */

This comment is hard to understand.

What does it mean, really?

> +		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;

What about

		dev->idle_stat.fast_idle = dev->idle_stat.predicted_us < overhead_threshold;

Also why don't you use dev->idle_stat.overhead directly here?

And what is the basic idea here?  Why do we compare the predicted
idle duration with the entry/exit overhead from the previous cycle
(if I understood this correctly, that is)?

> +	} 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;

We provide the callback which is going to be used by the core if present,
so why would the core not use it after all?

> +
>  	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();
>  	tick_nohz_idle_enter();
>  	cpuidle_entry_end();
>  
>