From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755483AbcBHCS4 (ORCPT ); Sun, 7 Feb 2016 21:18:56 -0500 Received: from v094114.home.net.pl ([79.96.170.134]:51706 "HELO v094114.home.net.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1754221AbcBHCSy (ORCPT ); Sun, 7 Feb 2016 21:18:54 -0500 From: "Rafael J. Wysocki" To: Viresh Kumar Cc: Saravana Kannan , "Rafael J. Wysocki" , Shilpa Bhat , Juri Lelli , Lists linaro-kernel , "linux-pm@vger.kernel.org" , Peter Zijlstra , Michael Turquette , Steve Muckle , Vincent Guittot , Morten Rasmussen , dietmar.eggemann@arm.com, Linux Kernel Mailing List Subject: Re: [PATCH V2 0/7] cpufreq: governors: Fix ABBA lockups Date: Mon, 08 Feb 2016 03:20:03 +0100 Message-ID: <6941844.eWerKNtl0q@vostro.rjw.lan> User-Agent: KMail/4.11.5 (Linux/4.5.0-rc1+; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20160205094925.GN21792@vireshk> References: <3866963.E19P3EGEHd@vostro.rjw.lan> <20160205094925.GN21792@vireshk> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="utf-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Friday, February 05, 2016 03:19:25 PM Viresh Kumar wrote: > On 05-02-16, 04:54, Rafael J. Wysocki wrote: > > Having actually posted that series again after cleaning it up I can say > > what I'm thinking about hopefully without confusing anyone too much. So > > please bear in mind that I'm going to refer to this series below: > > > > http://marc.info/?l=linux-pm&m=145463901630950&w=4 > > > > Also this is more of a brain dump rather than actual design description, > > so there may be holes etc in it. Please let me know if you can see any. > > > > The problem at hand is that policy->rwsem needs to be held around *all* > > operations in cpufreq_set_policy(). In particular, it cannot be dropped > > around invocations of __cpufreq_governor() with the event arg equal to > > _EXIT as that leads to interesting races. > > > > Unfortunately, we know that holding policy->rwsem in those places leads > > to a deadlock with governor sysfs attributes removal in cpufreq_governor_exit(). > > > > Viresh attempted to fix this by avoiding to acquire policy->rwsem for governor > > attributes access (as holding it is not necessary for them in principle). That > > was a nice try, but it turned out to be insufficient because of another deadlock > > scenario uncovered by it. > > Not really. > > The other deadlock wasn't uncovered by it, its just that Shilpa tested > directly after my patches and reported the issue. Later yesterday, she > was hitting the exactly same issue on pm/linux-next as well (i.e. > without my patches). And ofcourse Juri has also reported the same > issue on linux-next few days back. OK, fair enough. > > Namely, since the ondemand governor's update_sampling_rate() > > acquires the governor mutex (called dbs_data_mutex after my patches mentioned > > above), it may deadlock with exactly the same piece of code in cpufreq_governor_exit() > > in almost exactly the same way. > > Right. > > > To avoid that other deadlock, we'd either need to drop dbs_data_mutex from > > update_sampling_rate(), > > And my so called 'ugly' 8th patch tried to do just that :) > > But as I also mentioned in reply to the update-util patchset of yours, > its possible somewhat. Yes, it should be possible and not even too difficult. > > or drop it for the removal of the governor sysfs > > attributes in cpufreq_governor_exit(). I don't think the former is an option > > at least at this point, so it looks like we pretty much have to do the latter. > > > > With that in mind, I'd start with the changes made by Viresh (maybe without the > > first patch which really isn't essential here). > > That was just to cleanup the macro mess a bit, nothing more. Over > that, I think the first 7 patches can be picked as it is without any > changes. Ofcourse they are required to be rebased over your 13 > patches, if those are going in first :) Yes, please rebase. Also please skip the first one that was moving min_sampling_rate around, at least for now. As I said, we may be moving other attributes in the opposite direction, so two sets of macros may be necessary anyway. > > That is, introduce a separate > > kobject type for the governor attributes kobject and register that in > > cpufreq_governor_init(). The show/store callbacks for that kobject type won't > > acquire policy->rwsem so the first deadlock will be avoided. > > > > But in addition to that, I'd drop dbs_data_mutex before the removal of governor > > sysfs attributes. That actually happens in two places, in cpufreq_governor_exit() > > and in the error path of cpufreq_governor_init(). > > > > To that end, I'd move the locking from cpufreq_governor_dbs() to the functions > > called by it. That should be readily doable and they can do all of the > > necessary checks themselves. cpufreq_governor_dbs() would become a pure mux then, > > but that's not such a big deal. > > > > With that, cpufreq_governor_exit() may just drop the lock before it does the > > final kobject_put(). The danger here is that the sysfs show/store callbacks of > > the governor attributes kobject may see invalid dbs_data for a while, after the > > lock has been dropped and before the kobject is deleted. That may be addressed > > by checking, for example, the presence of the dbs_data's "tuners" pointer in those > > callbacks. If it is NULL, they can simply return -EAGAIN or similar. > > So you mean something like this (consider only !governor_per_policy > case with ondemand governor for now): > > exit() > { > lock-dbs_data_mutex; > ... > dbs_data->tuners = NULL; //so that sysfs files can return early > dbs_governor->gdbs_data = NULL; //For !governor_per_policy case > unlock-dbs_data_mutex; > > /* > * Problem: Who is stopping us to set ondemand as governor for > * another policy, which can try create a kobject which will > * try to create sysfs directory at the same path ? > * > * Though another field in dbs_governor can be used to fix this > * I think, which needs to block the other INIT operation. > */ > > kobject_put(dbs_data->kobj); //This should wait for all sysfs operations to end. > > kfree(dbs_data); > } > > And the sysfs operations show/store need to take dbs_data_mutex() for > their entire operations. > > ?? Yes, roughly. But it shouldn't be necessary after all, because dropping the mutex from update_sampling_rate() looks easier than I thought previously. Thanks, Rafael