Re: [RFC PATCH v2 1/8] cpuidle: menu: extract prediction functionality

[Rafael J. Wysocki]

On Saturday, September 30, 2017 9:20:27 AM CEST Aubrey Li wrote:
> There are several factors in the menu governor to predict the next
> idle interval:
> - the next timer
> - the recent idle interval history
> - the corrected idle interval pattern
> These factors are common enough to be extracted to be one function.
> 
> Signed-off-by: Aubrey Li <aubrey.li@xxxxxxxxxxxxxxx>

This patch alone would break things AFAICS, because it removes code from
menu_select() without a replacement (and menu_predict() is never called
just yet).

Please always do your best to ensure that things will work after *every*
patch in a series.

> ---
>  drivers/cpuidle/governors/menu.c | 64 +++++++++++++++++++++++++---------------
>  1 file changed, 40 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 61b64c2..6bed197 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -276,6 +276,45 @@ static unsigned int get_typical_interval(struct menu_device *data)
>  }
>  
>  /**
> + * menu_predict - predict the coming idle interval
> + *
> + * Return the predicted coming idle interval in micro-second
> + */
> +static unsigned int menu_predict(void)
> +{
> +	struct menu_device *data = this_cpu_ptr(&menu_devices);
> +	unsigned int expected_interval;
> +	int cpu = smp_processor_id();
> +
> +	if (!data)
> +		return UINT_MAX;
> +
> +	/* determine the expected residency time, round up */
> +	data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length());
> +
> +	data->bucket = which_bucket(data->next_timer_us, nr_iowait_cpu(cpu));
> +
> +	/*
> +	 * Force the result of multiplication to be 64 bits even if both
> +	 * operands are 32 bits.
> +	 * Make sure to round up for half microseconds.
> +	 */
> +	data->predicted_us = DIV_ROUND_CLOSEST_ULL((uint64_t)
> +		data->next_timer_us * data->correction_factor[data->bucket],
> +		RESOLUTION * DECAY);
> +
> +	expected_interval = get_typical_interval(data);
> +	expected_interval = min(expected_interval, data->next_timer_us);
> +
> +	/*
> +	 * Use the lowest expected idle interval to pick the idle state.
> +	 */
> +	data->predicted_us = min(data->predicted_us, expected_interval);
> +
> +	return data->predicted_us;
> +}
> +
> +/**
>   * menu_select - selects the next idle state to enter
>   * @drv: cpuidle driver containing state data
>   * @dev: the CPU
> @@ -289,7 +328,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	int first_idx;
>  	int idx;
>  	unsigned int interactivity_req;
> -	unsigned int expected_interval;
>  	unsigned long nr_iowaiters, cpu_load;
>  	int resume_latency = dev_pm_qos_raw_read_value(device);
>  
> @@ -306,24 +344,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	if (unlikely(latency_req == 0))
>  		return 0;
>  
> -	/* determine the expected residency time, round up */
> -	data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length());
> -
> -	get_iowait_load(&nr_iowaiters, &cpu_load);
> -	data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
> -
> -	/*
> -	 * Force the result of multiplication to be 64 bits even if both
> -	 * operands are 32 bits.
> -	 * Make sure to round up for half microseconds.
> -	 */
> -	data->predicted_us = DIV_ROUND_CLOSEST_ULL((uint64_t)data->next_timer_us *
> -					 data->correction_factor[data->bucket],
> -					 RESOLUTION * DECAY);
> -
> -	expected_interval = get_typical_interval(data);
> -	expected_interval = min(expected_interval, data->next_timer_us);
> -
>  	if (CPUIDLE_DRIVER_STATE_START > 0) {
>  		struct cpuidle_state *s = &drv->states[CPUIDLE_DRIVER_STATE_START];
>  		unsigned int polling_threshold;
> @@ -345,14 +365,10 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	}
>  
>  	/*
> -	 * Use the lowest expected idle interval to pick the idle state.
> -	 */
> -	data->predicted_us = min(data->predicted_us, expected_interval);
> -
> -	/*
>  	 * Use the performance multiplier and the user-configurable
>  	 * latency_req to determine the maximum exit latency.
>  	 */
> +	get_iowait_load(&nr_iowaiters, &cpu_load);
>  	interactivity_req = data->predicted_us / performance_multiplier(nr_iowaiters, cpu_load);
>  	if (latency_req > interactivity_req)
>  		latency_req = interactivity_req;
>