Re: [PATCH] ACPI/Processor: Rework processor throttling with work_on_cpu()

[Rafael J. Wysocki]

On Friday, February 21, 2014 01:35:45 PM Lan Tianyu wrote:
> acpi_processor_set_throttling() uses set_cpus_allowed_ptr() to make
> sure struct acpi_processor->acpi_processor_set_throttling() callback
> run on associated cpu. But the function maybe called in a worker which
> has been bound to a cpu. The patch is to replace set_cpus_allowed_ptr()
> with work_on_cpu().
> 
> Signed-off-by: Lan Tianyu <tianyu.lan@xxxxxxxxx>
> ---
>  drivers/acpi/processor_throttling.c | 70 +++++++++++++++++--------------------
>  1 file changed, 33 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/acpi/processor_throttling.c b/drivers/acpi/processor_throttling.c
> index 28baa05..2db105a 100644
> --- a/drivers/acpi/processor_throttling.c
> +++ b/drivers/acpi/processor_throttling.c
> @@ -56,6 +56,12 @@ struct throttling_tstate {
>  	int target_state;		/* target T-state */
>  };
>  
> +struct acpi_processor_throttling_arg {
> +	struct acpi_processor *pr;
> +	int target_state;
> +	bool force;
> +};
> +
>  #define THROTTLING_PRECHANGE       (1)
>  #define THROTTLING_POSTCHANGE      (2)
>  
> @@ -1060,16 +1066,25 @@ static int acpi_processor_set_throttling_ptc(struct acpi_processor *pr,
>  	return 0;
>  }
>  
> +static long acpi_processor_throttling_fn(void *data)
> +{
> +	struct acpi_processor_throttling_arg *arg = data;
> +	struct acpi_processor *pr = arg->pr;
> +	struct acpi_processor_throttling *p_throttling = &pr->throttling;
> +
> +	return p_throttling->acpi_processor_set_throttling(pr,
> +			arg->target_state, arg->force);

What about doing

	return pr->throttling.acpi_processor_set_throttling(...);

directly without using the extra p_throttling pointer?

> +}
> +
>  int acpi_processor_set_throttling(struct acpi_processor *pr,
>  						int state, bool force)
>  {
> -	cpumask_var_t saved_mask;
>  	int ret = 0;
>  	unsigned int i;
>  	struct acpi_processor *match_pr;
>  	struct acpi_processor_throttling *p_throttling;
> +	struct acpi_processor_throttling_arg arg;
>  	struct throttling_tstate t_state;
> -	cpumask_var_t online_throttling_cpus;
>  
>  	if (!pr)
>  		return -EINVAL;
> @@ -1080,14 +1095,6 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
>  	if ((state < 0) || (state > (pr->throttling.state_count - 1)))
>  		return -EINVAL;
>  
> -	if (!alloc_cpumask_var(&saved_mask, GFP_KERNEL))
> -		return -ENOMEM;
> -
> -	if (!alloc_cpumask_var(&online_throttling_cpus, GFP_KERNEL)) {
> -		free_cpumask_var(saved_mask);
> -		return -ENOMEM;
> -	}
> -
>  	if (cpu_is_offline(pr->id)) {
>  		/*
>  		 * the cpu pointed by pr->id is offline. Unnecessary to change
> @@ -1096,17 +1103,15 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
>  		return -ENODEV;
>  	}
>  
> -	cpumask_copy(saved_mask, &current->cpus_allowed);
>  	t_state.target_state = state;
>  	p_throttling = &(pr->throttling);
> -	cpumask_and(online_throttling_cpus, cpu_online_mask,
> -		    p_throttling->shared_cpu_map);
> +
>  	/*
>  	 * The throttling notifier will be called for every
>  	 * affected cpu in order to get one proper T-state.
>  	 * The notifier event is THROTTLING_PRECHANGE.
>  	 */
> -	for_each_cpu(i, online_throttling_cpus) {
> +	for_each_cpu_and(i, cpu_online_mask, p_throttling->shared_cpu_map) {
>  		t_state.cpu = i;
>  		acpi_processor_throttling_notifier(THROTTLING_PRECHANGE,
>  							&t_state);
> @@ -1118,21 +1123,18 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
>  	 * it can be called only for the cpu pointed by pr.
>  	 */
>  	if (p_throttling->shared_type == DOMAIN_COORD_TYPE_SW_ANY) {
> -		/* FIXME: use work_on_cpu() */
> -		if (set_cpus_allowed_ptr(current, cpumask_of(pr->id))) {
> -			/* Can't migrate to the pr->id CPU. Exit */
> -			ret = -ENODEV;
> -			goto exit;
> -		}
> -		ret = p_throttling->acpi_processor_set_throttling(pr,
> -						t_state.target_state, force);
> +		arg.pr = pr;
> +		arg.target_state = state;
> +		arg.force = force;
> +		ret = work_on_cpu(pr->id, acpi_processor_throttling_fn, &arg);
>  	} else {
>  		/*
>  		 * When the T-state coordination is SW_ALL or HW_ALL,
>  		 * it is necessary to set T-state for every affected
>  		 * cpus.
>  		 */
> -		for_each_cpu(i, online_throttling_cpus) {
> +		for_each_cpu_and(i, cpu_online_mask,
> +		    p_throttling->shared_cpu_map) {
>  			match_pr = per_cpu(processors, i);
>  			/*
>  			 * If the pointer is invalid, we will report the
> @@ -1153,13 +1155,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
>  					"on CPU %d\n", i));
>  				continue;
>  			}
> -			t_state.cpu = i;
> -			/* FIXME: use work_on_cpu() */
> -			if (set_cpus_allowed_ptr(current, cpumask_of(i)))
> -				continue;
> -			ret = match_pr->throttling.
> -				acpi_processor_set_throttling(
> -				match_pr, t_state.target_state, force);
> +
> +			arg.pr = match_pr;
> +			arg.target_state = state;
> +			arg.force = force;
> +			ret = work_on_cpu(pr->id, acpi_processor_throttling_fn,
> +				&arg);
>  		}
>  	}
>  	/*
> @@ -1168,17 +1169,12 @@ int acpi_processor_set_throttling(struct acpi_processor *pr,
>  	 * affected cpu to update the T-states.
>  	 * The notifier event is THROTTLING_POSTCHANGE
>  	 */
> -	for_each_cpu(i, online_throttling_cpus) {
> +	for_each_cpu_and(i, cpu_online_mask, p_throttling->shared_cpu_map) {
>  		t_state.cpu = i;
>  		acpi_processor_throttling_notifier(THROTTLING_POSTCHANGE,
>  							&t_state);
>  	}
> -	/* restore the previous state */
> -	/* FIXME: use work_on_cpu() */
> -	set_cpus_allowed_ptr(current, saved_mask);
> -exit:
> -	free_cpumask_var(online_throttling_cpus);
> -	free_cpumask_var(saved_mask);
> +
>  	return ret;
>  }
>  
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/