linux-kernel - Re: [PATCH V3 1/5] cpufreq: ondemand: Drop unnecessary locks from update_sampling

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] [day] [month] [year] [list]

Date:	Wed, 28 Oct 2015 14:26:22 +0530
From:	Viresh Kumar <viresh.kumar@...aro.org>
To:	"Rafael J. Wysocki" <rjw@...ysocki.net>
Cc:	linaro-kernel@...ts.linaro.org, linux-pm@...r.kernel.org,
	Preeti U Murthy <preeti@...ux.vnet.ibm.com>,
	open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH V3 1/5] cpufreq: ondemand: Drop unnecessary locks from
 update_sampling_rate()

On 28-10-15, 08:46, Rafael J. Wysocki wrote:
> On Wednesday, October 28, 2015 12:13:17 PM Viresh Kumar wrote:
> > Actually yeah, but then the fourth patch of this series uses the
> > timer_mutex to fix a long standing problem (which was fixed by hacking
> > the code earlier). And so we need to take the lock for the entire
> > dbs_timer() routine.

Well, there is another reason why the lock is taken for the complete
dbs_timer() routine. There are two parts of that routine:
- Checking if load evaluation is required or not + updating the
  last-update time.
- The second is the load evaluation + freq change thing.

Lock around the first check makes sure that timer handlers of other
CPUs don't do load evaluation in parallel and that they don't do it
before the sampling period.

Lock around the second part makes sure there is only one thread which
is doing load evaluation + freq update. The other thread being
cpufreq_governor_limits(). And so the same lock taken across that part
as well.

> I don't actually think that that patch is correct and even if it is,
> we'll only need to do that *after* that patch, so at least it would be
> fair to say a word about it in the changelog, wouldn't it?

Hmm, If you agree about the above reasoning, then we may not require
an update to the changelog, otherwise I will mention that in the
changelog of this patch.

> > Yeah, we are calling dbs_check_cpu(dbs_data, cpu) from that path,
> > which will reevaluate the load.
> 
> Which means that we should take the lock around dbs_check_cpu() everywhere
> in a consistent way.

We already do this from everywhere.

> Which in turn means that the lock actually does more
> than you said.

What I described towards the top is probably a better answer to the
earlier query.

> My point is basically that we seem to have a vague idea about what the lock
> is used for, while we need to know exactly why we need it.

I am totally with you on this, we have surely screwed up on locking
for a long time in cpufreq. And we should know exactly why we want to
change it now.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/