linux-kernel - Re: [PATCH] sched: Provide USF for the portable equipment.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200730182157.110a5cf0@oasis.local.home>
Date:   Thu, 30 Jul 2020 18:21:57 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Dongdong Yang <contribute.kernel@...il.com>
Cc:     gregkh@...uxfoundation.org, rjw@...ysocki.net,
        viresh.kumar@...aro.org, mingo@...hat.com, peterz@...radead.org,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, bsegall@...gle.com, mgorman@...e.de,
        linux-kernel@...r.kernel.org, devel@...verdev.osuosl.org,
        linux-pm@...r.kernel.org, yangdongdong@...omi.com,
        tanggeliang@...omi.com, taojun@...omi.com, huangqiwu@...omi.com,
        rocking@...ux.alibaba.com, fengwei@...omi.com,
        zhangguoquan@...omi.com, gulinghua@...omi.com, duhui@...omi.com
Subject: Re: [PATCH] sched: Provide USF for the portable equipment.

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

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

> 
> Signed-off-by: Dongdong Yang <yangdongdong@...omi.com>
> Signed-off-by: Jun Tao <taojun@...omi.com>
> Signed-off-by: Qiwu Huang <huangqiwu@...omi.com>
> Signed-off-by: Geliang Tang <tanggeliang@...omi.com>
> Signed-off-by: Peng Wang <rocking@...ux.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);
>