From patchwork Mon Jun 22 08:02:56 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 50141 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-lb0-f197.google.com (mail-lb0-f197.google.com [209.85.217.197]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id B815B21594 for ; Mon, 22 Jun 2015 08:03:46 +0000 (UTC) Received: by lbcui10 with SMTP id ui10sf1932888lbc.0 for ; Mon, 22 Jun 2015 01:03:45 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:delivered-to:from:to:cc:subject :date:message-id:in-reply-to:references:in-reply-to:references :sender:precedence:list-id:x-original-sender :x-original-authentication-results:mailing-list:list-post:list-help :list-archive:list-unsubscribe; bh=HGs4O5TWuDWLX3cF/xisr9r6qN/vn85rGze90Bxi0yA=; b=gPq86mHLe3tjwKSBjhV/z7gWHoVDnaJZnS4Kw/xecIbtmUalkk8aSs4emb7RZQbTVu ggtv0Kpf/11PN0CLro6h41yGih3YX+L4xkyFZYTtS1FqJJ7qFPYsjm+61tvXgZI4uMsJ TafyLoRab4FVadhWHY/3SZLGtP5rnCP/CA229IfYyA7tG1fJR083vMMo2axF+WBWcRcj uJug4rHSFj/kGfyUS6V8YV2kplWFblGpbqvT9XUqZEPWONG31CUpyTXJPMlV19sT9ypH 7OhdFZJD0G8+jKP5Pa+UqOqpvqaGp1jgDv2PCtaAZvWtQd2HE72h4fChgCUxXSOukZLG buQw== X-Gm-Message-State: ALoCoQnYQrkww2tFpvFUe85wKS8weJCPAp6znhSPOO+Cb7qpUNCMq2SO6s0w4AUyDDIxAgTTgHR4 X-Received: by 10.152.19.161 with SMTP id g1mr27009042lae.8.1434960225804; Mon, 22 Jun 2015 01:03:45 -0700 (PDT) MIME-Version: 1.0 X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.19.137 with SMTP id f9ls356194lae.5.gmail; Mon, 22 Jun 2015 01:03:45 -0700 (PDT) X-Received: by 10.112.190.10 with SMTP id gm10mr28051543lbc.2.1434960225659; Mon, 22 Jun 2015 01:03:45 -0700 (PDT) Received: from mail-lb0-f181.google.com (mail-lb0-f181.google.com. [209.85.217.181]) by mx.google.com with ESMTPS id e7si15909650lbc.62.2015.06.22.01.03.45 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 22 Jun 2015 01:03:45 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.181 as permitted sender) client-ip=209.85.217.181; Received: by lbbpo10 with SMTP id po10so4058996lbb.3 for ; Mon, 22 Jun 2015 01:03:45 -0700 (PDT) X-Received: by 10.152.7.7 with SMTP id f7mr29241297laa.106.1434960225364; Mon, 22 Jun 2015 01:03:45 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.112.108.230 with SMTP id hn6csp2208908lbb; Mon, 22 Jun 2015 01:03:44 -0700 (PDT) X-Received: by 10.68.87.5 with SMTP id t5mr55677054pbz.137.1434960221938; Mon, 22 Jun 2015 01:03:41 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id n11si28457864pdl.134.2015.06.22.01.03.41; Mon, 22 Jun 2015 01:03:41 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756129AbbFVIDk (ORCPT + 11 others); Mon, 22 Jun 2015 04:03:40 -0400 Received: from mail-pd0-f178.google.com ([209.85.192.178]:32889 "EHLO mail-pd0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756173AbbFVIDj (ORCPT ); Mon, 22 Jun 2015 04:03:39 -0400 Received: by pdjn11 with SMTP id n11so133813161pdj.0 for ; Mon, 22 Jun 2015 01:03:39 -0700 (PDT) X-Received: by 10.68.68.135 with SMTP id w7mr56866693pbt.63.1434960219099; Mon, 22 Jun 2015 01:03:39 -0700 (PDT) Received: from localhost ([122.167.71.211]) by mx.google.com with ESMTPSA id d5sm18900840pdl.85.2015.06.22.01.03.37 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Mon, 22 Jun 2015 01:03:38 -0700 (PDT) From: Viresh Kumar To: Rafael Wysocki , Preeti U Murthy Cc: linaro-kernel@lists.linaro.org, linux-pm@vger.kernel.org, Viresh Kumar Subject: [PATCH 09/10] cpufreq: governor: Quit work-handlers early if governor is stopped Date: Mon, 22 Jun 2015 13:32:56 +0530 Message-Id: X-Mailer: git-send-email 2.4.0 In-Reply-To: References: In-Reply-To: References: Sender: linux-pm-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-pm@vger.kernel.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: viresh.kumar@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.217.181 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , cpufreq_governor_lock is abused by using it outside of cpufreq core, i.e. in cpufreq-governors. But we didn't had a solution at that point of time, and so doing that was the only acceptable solution: 6f1e4efd882e ("cpufreq: Fix timer/workqueue corruption by protecting reading governor_enabled") The cpufreq governor core is fixed now against possible races and things are in much better shape. cpufreq core is checking for invalid state-transitions of governors in __cpufreq_governor() with help of governor_enabled flag. The governor core is already taking care of that now and so we can get rid of those extra checks in __cpufreq_governor(). To do that, we first need to get rid of the dependency on governor_enabled flag in governor core, in gov_queue_work. This patch is about getting rid of this dependency. When a CPU is hot removed we'll cancel all the delayed work items via gov_cancel_work(). Normally this will just cancels a delayed timer on each CPU that the policy is managing and the work won't run. But if the work is already running, the workqueue code will wait for the work to finish before continuing to prevent the work items from re-queuing themselves like they normally do. This scheme will work most of the time, except for the case where the work function determines that it should adjust the delay for all other CPUs that the policy is managing. If this scenario occurs, the canceling CPU will cancel its own work but queue up the other CPUs works to run. And we will enter a situation where gov_cancel_work() has returned with work being queued on few CPUs. To fix that in a different (non-hacky) way, set set ccdbs->policy to false before trying to cancel the work. It should be updated within timer_mutex, which will prevent the work-handlers to start. Once the work-handlers finds that we are already trying to stop the governor, it will exit early. And that will prevent queuing of works again as well. Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq_governor.c | 33 +++++++++++++++++++++++---------- 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index ac288f0efb06..38748e219cae 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -164,17 +164,10 @@ void gov_queue_work(struct dbs_data *dbs_data, struct cpufreq_policy *policy, struct cpu_dbs_info *cdbs; int cpu; - mutex_lock(&cpufreq_governor_lock); - if (!policy->governor_enabled) - goto out_unlock; - for_each_cpu(cpu, cpus) { cdbs = dbs_data->cdata->get_cpu_cdbs(cpu); mod_delayed_work_on(cpu, system_wq, &cdbs->dwork, delay); } - -out_unlock: - mutex_unlock(&cpufreq_governor_lock); } EXPORT_SYMBOL_GPL(gov_queue_work); @@ -213,14 +206,25 @@ static void dbs_timer(struct work_struct *work) struct cpu_dbs_info *cdbs = container_of(work, struct cpu_dbs_info, dwork.work); struct cpu_common_dbs_info *ccdbs = cdbs->ccdbs; - struct cpufreq_policy *policy = ccdbs->policy; - struct dbs_data *dbs_data = policy->governor_data; + struct cpufreq_policy *policy; + struct dbs_data *dbs_data; unsigned int sampling_rate, delay; const struct cpumask *cpus; bool load_eval; mutex_lock(&ccdbs->timer_mutex); + policy = ccdbs->policy; + + /* + * Governor might already be disabled and there is no point continuing + * with the work-handler. + */ + if (!policy) + goto unlock; + + dbs_data = policy->governor_data; + if (dbs_data->cdata->governor == GOV_CONSERVATIVE) { struct cs_dbs_tuners *cs_tuners = dbs_data->tuners; @@ -237,6 +241,7 @@ static void dbs_timer(struct work_struct *work) delay = dbs_data->cdata->gov_dbs_timer(cdbs, dbs_data, load_eval); gov_queue_work(dbs_data, policy, delay, cpus); +unlock: mutex_unlock(&ccdbs->timer_mutex); } @@ -473,9 +478,17 @@ static int cpufreq_governor_stop(struct cpufreq_policy *policy, if (!ccdbs || !ccdbs->policy) return -EBUSY; + /* + * Work-handler must see this updated, as it should not proceed any + * further after governor is disabled. And so timer_mutex is taken while + * updating this value. + */ + mutex_lock(&ccdbs->timer_mutex); + ccdbs->policy = NULL; + mutex_unlock(&ccdbs->timer_mutex); + gov_cancel_work(dbs_data, policy); - ccdbs->policy = NULL; mutex_destroy(&ccdbs->timer_mutex); return 0; }