From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965209AbbJ1I0K (ORCPT ); Wed, 28 Oct 2015 04:26:10 -0400 Received: from mail-pa0-f48.google.com ([209.85.220.48]:34591 "EHLO mail-pa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932533AbbJ1I0E (ORCPT ); Wed, 28 Oct 2015 04:26:04 -0400 Date: Wed, 28 Oct 2015 13:55:59 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, open list Subject: Re: [PATCH V3 4/5] cpufreq: governor: Quit work-handlers early if governor is stopped Message-ID: <20151028082559.GD30039@ubuntu> References: <1e579d2bf8dbee09295725cda37bd92222fe61fb.1444723240.git.viresh.kumar@linaro.org> <11662041.x9fsKCWszL@vostro.rjw.lan> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <11662041.x9fsKCWszL@vostro.rjw.lan> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28-10-15, 08:10, Rafael J. Wysocki wrote: > I have a hard time figuring out what the patch is supposed to achieve from > the above. We had a problem earlier, where even after stopping the governor for a policy, the work was still queued for some of its CPUs. We failed to understand the real problem then, and so abused the wider cpufreq_governor_lock. I understood the problem much better now, and got a straight forward, and precise solution for that. > Do we eventually want to get rid of cpufreq_governor_lock and that's why we're > doing this? That's another benefit we get out of this change. > > + mutex_lock(&shared->timer_mutex); > > + shared->policy = NULL; > > + mutex_unlock(&shared->timer_mutex); Right. > So this assumes that dbs_timer() will acquire the mutex and see that > shared->policy is now NULL, so it will bail out immediately, but -> > > > + > > gov_cancel_work(dbs_data, policy); > > > > - shared->policy = NULL; > > mutex_destroy(&shared->timer_mutex); > > -> the mutex is destroyed here, so what the guarantee that the mutex will > still be around when dbs_timer() runs? You really got me worried for few minutes :) The earlier update of shared->policy = NULL, makes sure that no work-handler can start real work. After the unlock the work handlers will start executing but will return early. We also have gov_cancel_work(), which will until the time all the current handlers have finished executing and no work is queued. And so we are sure that there are no users of the mutex when it is destroyed. -- viresh