[PATCH] sched: Provide USF for the portable equipment.

[Steven Rostedt]

On Thu, 30 Jul 2020 21:35:43 +0800
Dongdong Yang <contribute.kernel at gmail.com> wrote:

I'll let others decide the value of this, but I have some comments.

> 
> Signed-off-by: Dongdong Yang <yangdongdong at xiaomi.com>
> Signed-off-by: Jun Tao <taojun at xiaomi.com>
> Signed-off-by: Qiwu Huang <huangqiwu at xiaomi.com>
> Signed-off-by: Geliang Tang <tanggeliang at xiaomi.com>
> Signed-off-by: Peng Wang <rocking at linux.alibaba.com>

Why all the signed-off-bys? All of you worked on it?



> +	if (evdata->data && val == FB_EVENT_BLANK) {
> +		blank = *(int *)(evdata->data);
> +
> +		switch (blank) {
> +		case FB_BLANK_POWERDOWN:
> +			usf_vdev.is_screen_on = 0;
> +			if (usf_vdev.sysctl_sched_usf_non_ux != 0)
> +				adjust_task_pred_demand =
> +				    &adjust_task_pred_demand_impl;
> +			else
> +				adjust_task_pred_demand = NULL;

So sysctl can enable and disable this?

> +
> +			break;
> +

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
> index 7fbaee2..7bc3429 100644
> --- a/kernel/sched/cpufreq_schedutil.c
> +++ b/kernel/sched/cpufreq_schedutil.c
> @@ -289,12 +289,21 @@ unsigned long schedutil_cpu_util(int cpu, unsigned long util_cfs,
>  	return min(max, util);
>  }
>  
> +#ifdef CONFIG_SCHED_USF
> +void (*adjust_task_pred_demand)(int cpuid, unsigned long *util,
> +	struct rq *rq) = NULL;
> +EXPORT_SYMBOL(adjust_task_pred_demand);
> +#endif
> +
>  static unsigned long sugov_get_util(struct sugov_cpu *sg_cpu)
>  {
>  	struct rq *rq = cpu_rq(sg_cpu->cpu);
>  	unsigned long util = cpu_util_cfs(rq);
>  	unsigned long max = arch_scale_cpu_capacity(sg_cpu->cpu);
> -
> +#ifdef CONFIG_SCHED_USF
> +	if (adjust_task_pred_demand)
> +		adjust_task_pred_demand(sg_cpu->cpu, &util, rq);

The above is racy. Nothing stops adjust_task_pred_demand from being
non-null at the if condition, then becoming NULL before it is called.

Instead I would have the following:

DEFINE_STATIC_KEY_FALSE(adjust_task_pred_set);

#ifdef CONFIG_SCHED_USF
void adjust_task_pred_demand(int cpuid, unsigned long *util,
				struct rq *rq);
#else
static inline void adjust_task_pred_demand(int cpuid,
		unsigned long *util, struct rq *rq)
{ }
#endif


	if (static_key_unlikely(adjust_task_pred_set))
		adjust_task_pred_demand(sg_cpu->cpu, &util, rq);

And hopefully the compiler will just remove all of it if it's not
enabled.

Then you set the static branch when you want it to be called, and do
not use a racy function pointer.

-- Steve



> +#endif
>  	sg_cpu->max = max;
>  	sg_cpu->bw_dl = cpu_bw_dl(rq);
>