linux-kernel - Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management framework

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: <20180607173143.GD3597@e108498-lin.cambridge.arm.com>
Date:   Thu, 7 Jun 2018 18:31:43 +0100
From:   Quentin Perret <quentin.perret@....com>
To:     Juri Lelli <juri.lelli@...hat.com>
Cc:     peterz@...radead.org, rjw@...ysocki.net,
        gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org, mingo@...hat.com,
        dietmar.eggemann@....com, morten.rasmussen@....com,
        chris.redpath@....com, patrick.bellasi@....com,
        valentin.schneider@....com, vincent.guittot@...aro.org,
        thara.gopinath@...aro.org, viresh.kumar@...aro.org,
        tkjos@...gle.com, joelaf@...gle.com, smuckle@...gle.com,
        adharmap@...cinc.com, skannan@...cinc.com, pkondeti@...eaurora.org,
        edubezval@...il.com, srinivas.pandruvada@...ux.intel.com,
        currojerez@...eup.net, javi.merino@...nel.org
Subject: Re: [RFC PATCH v3 03/10] PM: Introduce an Energy Model management
 framework

On Thursday 07 Jun 2018 at 18:04:19 (+0200), Juri Lelli wrote:
> On 07/06/18 16:19, Quentin Perret wrote:
> > On Thursday 07 Jun 2018 at 16:44:09 (+0200), Juri Lelli wrote:
> > > > +	if (!fd->cs_table)
> > > > +		goto free_fd;
> > > > +
> > > > +	/* Copy the span of the frequency domain */
> > > > +	cpumask_copy(&fd->cpus, span);
> > > > +
> > > > +	/* Build the list of capacity states for this freq domain */
> > > > +	for (i = 0, freq = 0; i < nr_states; i++, freq++) {
> > >                      ^                              ^
> > > The fact that this relies on active_power() to use ceil OPP for a given
> > > freq might deserve a comment. Also, is this behaviour of active_power()
> > > standardized?
> > 
> > Right, this can get confusing pretty quickly. There is a comment in
> > include/linux/energy_model.h where the expected behaviour of
> > active_power is explained, but a reminder above this function shouldn't
> > hurt.
> 
> Mmm, not sure if you could actually check that returned freq values are
> actually consistent with the assumption (just in case one didn't do
> homework).

Right, that's a good point. I'll add checks on the parameters modified by
active_power(). Monotonically increasing freq, monotonically increasing
power as well I guess, something along those lines.

> > > > +{
> > > > +	struct em_cs_table *old_table, *new_table;
> > > > +	struct em_freq_domain *fd;
> > > > +	unsigned long flags;
> > > > +	int nr_states, cpu;
> > > > +
> > > > +	read_lock_irqsave(&em_data_lock, flags);
> > > 
> > > Don't you need write_lock_ here, since you are going to exchange the
> > > em tables?
> > 
> > This lock protects the per_cpu() variable itself. Here we only read
> > pointers from that per_cpu variable, and we modify one attribute in
> > the pointed structure. We don't modify the per_cpu table itself. Does
> > that make sense ?
> 
> So, I don't seem to understand what protects the rcu_assign_pointer(s)
> below (as in
> https://elixir.bootlin.com/linux/latest/source/Documentation/RCU/whatisRCU.txt#L395).

Sigh, that's not right :(
I take back my previous message, the write lock _is_ needed. Thanks for
pointing that out ...

Thanks,
Quentin