From patchwork Tue Sep 3 13:20:05 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Srivatsa S. Bhat" X-Patchwork-Id: 19711 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-vb0-f69.google.com (mail-vb0-f69.google.com [209.85.212.69]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id B6A7C25DFC for ; Tue, 3 Sep 2013 13:24:04 +0000 (UTC) Received: by mail-vb0-f69.google.com with SMTP id e13sf6402060vbg.4 for ; Tue, 03 Sep 2013 06:24:04 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:message-id:date:from:user-agent :mime-version:to:cc:subject:references:in-reply-to:x-original-sender :x-original-authentication-results:precedence:mailing-list:list-id :list-post:list-help:list-archive:list-unsubscribe:content-type :content-transfer-encoding; bh=gwQoG7gYyA2Mj2TuCLd8PqbiE6d+VopW4wclH9jLeL8=; b=kJVOT+xHKdJCG3YbMEqPQsgBEv7Q4oUgkVYPtXJfqxtZScBlQ6BjqLIOFPcCIW9+FK VtX8nU4/bGWQlWQf8KoXar8KG5zzmJyObCYY4vxwm92IbD3uip70NIKWo4XDmgOPi4oR yxfWTFwIccKdX5+Jx7ktdWFIkKEukzvNf6sdWtQGkLVlDR7hZURIimbKpbH0Eh1S7+Ae ciDsCmHpPDEf7A+qBrr7zsGbDdEWlXbfMmpLLNb9+82xvR4q1knkrP+QclKsR7Xe6iUj Ea1++qjCcBI5rFzz0zw/cAulF+MqN6AJR7NtZMZnjB4GwMeICi5IKE0xOXC8GqeX92BS TMbg== X-Received: by 10.236.31.228 with SMTP id m64mr10934441yha.5.1378214644187; Tue, 03 Sep 2013 06:24:04 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.49.104.198 with SMTP id gg6ls2683535qeb.89.gmail; Tue, 03 Sep 2013 06:24:04 -0700 (PDT) X-Received: by 10.220.74.69 with SMTP id t5mr28720666vcj.18.1378214644081; Tue, 03 Sep 2013 06:24:04 -0700 (PDT) Received: from mail-vc0-f182.google.com (mail-vc0-f182.google.com [209.85.220.182]) by mx.google.com with ESMTPS id wp10si4008538vdb.19.1969.12.31.16.00.00 (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 03 Sep 2013 06:24:04 -0700 (PDT) Received-SPF: neutral (google.com: 209.85.220.182 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.220.182; Received: by mail-vc0-f182.google.com with SMTP id hf12so3818689vcb.41 for ; Tue, 03 Sep 2013 06:24:04 -0700 (PDT) X-Gm-Message-State: ALoCoQnzKWXeX5eqICWWlkeONX52r9J2mf2Ya+gTjCZ11QvmoGEfEgoentYJanNPFfmjnEaC9Ke3 X-Received: by 10.58.100.234 with SMTP id fb10mr28762682veb.5.1378214643954; Tue, 03 Sep 2013 06:24:03 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.220.174.196 with SMTP id u4csp171376vcz; Tue, 3 Sep 2013 06:24:03 -0700 (PDT) X-Received: by 10.66.233.37 with SMTP id tt5mr31546509pac.95.1378214642432; Tue, 03 Sep 2013 06:24:02 -0700 (PDT) Received: from e28smtp02.in.ibm.com (e28smtp02.in.ibm.com. [122.248.162.2]) by mx.google.com with ESMTPS id yt8si15479520pab.148.1969.12.31.16.00.00 (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 03 Sep 2013 06:24:02 -0700 (PDT) Received-SPF: pass (google.com: domain of srivatsa.bhat@linux.vnet.ibm.com designates 122.248.162.2 as permitted sender) client-ip=122.248.162.2; Received: from /spool/local by e28smtp02.in.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Tue, 3 Sep 2013 18:43:21 +0530 Received: from d28dlp03.in.ibm.com (9.184.220.128) by e28smtp02.in.ibm.com (192.168.1.132) with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted; Tue, 3 Sep 2013 18:43:19 +0530 Received: from d28relay01.in.ibm.com (d28relay01.in.ibm.com [9.184.220.58]) by d28dlp03.in.ibm.com (Postfix) with ESMTP id BD2CA1258055; Tue, 3 Sep 2013 18:53:51 +0530 (IST) Received: from d28av04.in.ibm.com (d28av04.in.ibm.com [9.184.220.66]) by d28relay01.in.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id r83DPgiU19857494; Tue, 3 Sep 2013 18:55:42 +0530 Received: from d28av04.in.ibm.com (localhost [127.0.0.1]) by d28av04.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id r83DNsQU012659; Tue, 3 Sep 2013 18:53:54 +0530 Received: from srivatsabhat.in.ibm.com (srivatsabhat.in.ibm.com [9.124.35.208]) by d28av04.in.ibm.com (8.14.4/8.14.4/NCO v10.0 AVin) with ESMTP id r83DNsRD012647; Tue, 3 Sep 2013 18:53:54 +0530 Message-ID: <5225E205.7080008@linux.vnet.ibm.com> Date: Tue, 03 Sep 2013 18:50:05 +0530 From: "Srivatsa S. Bhat" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120828 Thunderbird/15.0 MIME-Version: 1.0 To: Viresh Kumar CC: rjw@sisk.pl, sboyd@codeaurora.org, linaro-kernel@lists.linaro.org, patches@linaro.org, cpufreq@vger.kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 2/2] cpufreq: serialize calls to __cpufreq_governor() References: <085013f4e584e3fef97187bcb349c3fa76942e19.1378012620.git.viresh.kumar@linaro.org> <80c4bd8c577e5da0aa63342671773be5cdc26a9a.1378012620.git.viresh.kumar@linaro.org> In-Reply-To: <80c4bd8c577e5da0aa63342671773be5cdc26a9a.1378012620.git.viresh.kumar@linaro.org> X-TM-AS-MML: No X-Content-Scanned: Fidelis XPS MAILER x-cbid: 13090313-5816-0000-0000-000009B722B0 X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: srivatsa.bhat@linux.vnet.ibm.com X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.220.182 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , On 09/01/2013 10:56 AM, Viresh Kumar wrote: > We can't take a big lock around __cpufreq_governor() as this causes recursive > locking for some cases. But calls to this routine must be serialized for every > policy. > > Lets introduce another variable which would guarantee serialization here. > > Signed-off-by: Viresh Kumar > --- > drivers/cpufreq/cpufreq.c | 7 ++++++- > include/linux/cpufreq.h | 1 + > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index f320a20..4d5723db 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -1692,13 +1692,15 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, > policy->cpu, event); > > mutex_lock(&cpufreq_governor_lock); > - if ((policy->governor_enabled && (event == CPUFREQ_GOV_START)) || > + if (policy->governor_busy || > + (policy->governor_enabled && (event == CPUFREQ_GOV_START)) || > (!policy->governor_enabled && ((event == CPUFREQ_GOV_LIMITS) || > (event == CPUFREQ_GOV_STOP)))) { > mutex_unlock(&cpufreq_governor_lock); > return -EBUSY; > } > > + policy->governor_busy = true; > if (event == CPUFREQ_GOV_STOP) > policy->governor_enabled = false; > else if (event == CPUFREQ_GOV_START) > @@ -1727,6 +1729,9 @@ static int __cpufreq_governor(struct cpufreq_policy *policy, > ((event == CPUFREQ_GOV_POLICY_EXIT) && !ret)) > module_put(policy->governor->owner); > > + mutex_lock(&cpufreq_governor_lock); > + policy->governor_busy = false; > + mutex_unlock(&cpufreq_governor_lock); > return ret; > } > This doesn't solve the problem completely: it prevents the store_*() task from continuing *only* when it concurrently executes the __cpufreq_governor() function along with the CPU offline task. But if the two calls don't overlap, we will still have the possibility where the store_*() task tries to acquire the timer mutex after the CPU offline task has just finished destroying it. So, IMHO, the right fix is to synchronize with CPU hotplug. That way, the store_*() thread will wait until the entire CPU offline operation is completed. After that, if it continues, it will get -EBUSY, due to patch [1], since policy->governor_enabled will be set to false. [1]. https://patchwork.kernel.org/patch/2852463/ So here is the (completely untested) fix that I propose, as a replacement to this patch [2/2]: Regards, Srivatsa S. Bhat diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 5c75e31..71c4fb2 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -440,17 +440,24 @@ static ssize_t store_##file_name \ unsigned int ret; \ struct cpufreq_policy new_policy; \ \ + get_online_cpus(); \ ret = cpufreq_get_policy(&new_policy, policy->cpu); \ - if (ret) \ - return -EINVAL; \ + if (ret) { \ + ret = -EINVAL; \ + goto out; \ + } \ \ ret = sscanf(buf, "%u", &new_policy.object); \ - if (ret != 1) \ - return -EINVAL; \ + if (ret != 1) { \ + ret = -EINVAL; \ + goto out; \ + } \ \ ret = __cpufreq_set_policy(policy, &new_policy); \ policy->user_policy.object = policy->object; \ \ +out: \ + put_online_cpus(); \ return ret ? ret : count; \ } @@ -494,17 +501,22 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, char str_governor[16]; struct cpufreq_policy new_policy; + get_online_cpus(); ret = cpufreq_get_policy(&new_policy, policy->cpu); if (ret) - return ret; + goto out; ret = sscanf(buf, "%15s", str_governor); - if (ret != 1) - return -EINVAL; + if (ret != 1) { + ret = -EINVAL; + goto out; + } if (cpufreq_parse_governor(str_governor, &new_policy.policy, - &new_policy.governor)) - return -EINVAL; + &new_policy.governor)) { + ret = -EINVAL; + goto out; + } /* * Do not use cpufreq_set_policy here or the user_policy.max @@ -515,6 +527,9 @@ static ssize_t store_scaling_governor(struct cpufreq_policy *policy, policy->user_policy.policy = policy->policy; policy->user_policy.governor = policy->governor; +out: + put_online_cpus(); + if (ret) return ret; else