linux-kernel - Re: cpufreq cleanups

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: <20090706134531.GB7082@Krystal>
Date:	Mon, 6 Jul 2009 09:45:31 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
To:	Thomas Renninger <trenn@...e.de>
Cc:	Dave Jones <davej@...hat.com>, linux-kernel@...r.kernel.org,
	cpufreq@...r.kernel.org, kernel-testers@...r.kernel.org,
	Ingo Molnar <mingo@...e.hu>, "Rafael J. Wysocki" <rjw@...k.pl>,
	Dave Young <hidave.darkstar@...il.com>,
	Pekka Enberg <penberg@...helsinki.fi>,
	Venkatesh Pallipadi <venkatesh.pallipadi@...el.com>
Subject: Re: cpufreq cleanups - .30 vs .31

* Thomas Renninger (trenn@...e.de) wrote:
> Hi Dave,
> 
> this is about Venki's and Mathieu's recently sent cleanups.
> I'd like to summarize this to help finding a solution:
> 
> IMO Venki's approach (making .governor() always be called with
> rwsem held) is the cleaner one and this should be the way to
> go for .31 and future. This better separates locking responsibilities
> between cpufreq core and governors and brings back "design" into this.
> 
> One could argue that for .30 Mathieu's is better, because less
> intrusive. It's up to Dave in the end, but:
> [patch 2.6.30 1/4] remove rwsem lock from CPUFREQ_GOV_STOP call (second call 
> site)
> should not be the way to go for .31 and I'd vote for Venki's
> approach concerning locking .governor() against multiple calls (done by
> rwsem) and governor() vs do_dbs_timer calls (governor's job with a governor
> specific sem).
> 
> So if not find too intrusive, I'd say:
> Venkatesh's whole series of:
> [patch 0/4] Take care of cpufreq lockdep issues (take 2)
> should be seen in .31.
> 
> Depending on how intrusive this is seen, Venki's first patch:
> [patch 1/4] cpufreq: Eliminate the recent lockdep warnings in cpufreq
> should then go to .30 (after still waiting a bit?)
> or Mathieu's approach (I'd vote for Venki's to be consistent for .30 and .31).
> 
> The one patch from Mathieu:
> [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess
> is a separate, general cleanup which should show up in .31.
> 
> 
> 
> I still have two patch specific questions:
> about Mathieu's (it's a minor issue in the error path):
> [patch 2.6.30 2/4] CPUFREQ: fix (utter) cpufreq_add_dev mess
> 
> +                       if (lock_policy_rwsem_write(cpu) < 0) {
> +                               /* Should not go through policy unlock path */
> +                               if (cpufreq_driver->exit)
> +                                       cpufreq_driver->exit(policy);
> +                               ret = -EBUSY;
> +                               cpufreq_cpu_put(managed_policy);
> Shouldn't:
>  cpufreq_cpu_put(managed_policy);
> be called before:
>  cpufreq_driver->exit(policy);
> Just in case the driver itself wants to grab the policy of the
> managed cpu?
> 

If we want to make it perfectly similar to the error path:

(note : this is a success path if ret = 0, and error path if ret != 0)

ret != 0 :
                        if (!ret)
                              cpufreq_cpu_put(managed_policy);
                        /*
                         * Success. We only needed to be added to the mask.
                         * Call driver->exit() because only the cpu parent of
                         * the kobj needed to call init().
                         */
                        goto out_driver_exit; /* call driver->exit() */

Then yes, we might be tempted to flip the cpu_put and exit.

But given cpu_put just decrements a reference counter (and performs
cleanup if needed), then even if ->exit() takes a reference count
somehow, this will just make the code hold 2 refcounts temporarily.

And I prefer to keep this refcount thorough ->exit() call, because we
don't hold any policy_rwsem at this point : we are in a lock acquisition
error path. It's therefore safer to keep the refcount to ensure that
data won't vanish.

Mathieu

> 
> about Venki's:
> [patch 3/4] cpufreq: Cleanup locking in ondemand governor
> Isn't it possible to use only one mutex(timer_mutex) to protect do_dbs_timer 
> against governor start, stop, limit?
> Then dbs_mutex would only be used to protect against concurrent sysfs access
> and can be thrown away as soon as ondemand only provides global sysfs files,
> not per cpu ones.
> 
> Hmm, maybe this should just go in? It eases up things, but it's still hard
> to follow up each detail. Fixes/enhancements can still be put on top
> for .31.
> 
>   Thomas
> 

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
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/