From patchwork Tue May 24 04:56:30 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 68434 Delivered-To: patch@linaro.org Received: by 10.140.92.199 with SMTP id b65csp432288qge; Mon, 23 May 2016 21:56:37 -0700 (PDT) X-Received: by 10.98.82.19 with SMTP id g19mr3645642pfb.157.1464065797346; Mon, 23 May 2016 21:56:37 -0700 (PDT) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 21si57216582pfi.15.2016.05.23.21.56.37; Mon, 23 May 2016 21:56:37 -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; Authentication-Results: mx.google.com; dkim=neutral (body hash did not verify) header.i=@linaro.org; spf=pass (google.com: best guess record for domain of linux-pm-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-pm-owner@vger.kernel.org; dmarc=fail (p=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752000AbcEXE4f (ORCPT + 14 others); Tue, 24 May 2016 00:56:35 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:34516 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751870AbcEXE4e (ORCPT ); Tue, 24 May 2016 00:56:34 -0400 Received: by mail-pa0-f43.google.com with SMTP id qo8so2753725pab.1 for ; Mon, 23 May 2016 21:56:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=lLr3TR9UkoDPhCl27p35up4kguwmnw/HG5SDhPjybFM=; b=j5AIn3OGJF6mrEZ2XyDd2tDQpEQttzUyKzmPhHAkwxIX7Ft1KeAAjDWx2EM7QS9701 w45PL51g8/XXxjIyyOs65xgZ24sQDNo+s7jgAL0p192uSM0L2E0xAIWTPN41XNVaHCTn yov1yR9MOhCB+x5/3UlLJbcca7LVHy6NprrZI= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=lLr3TR9UkoDPhCl27p35up4kguwmnw/HG5SDhPjybFM=; b=LQZhFZi7utpsG+xDERYQiumpsEo+9JbcdUfVjzrKA+1fGCRn4yHyPIV6KQFuWFSP2+ fA3JeSorvA9D6qAE5njyxL9u/XXlvZ0t2maIJCZv2oOC2Y8BYrE5dCQhLews5JKeSj8L 7gUcP4Nx5uOn9j9yazYNt14dZeyEGiTOrvjhGdKNk+zJGBN/Ao+qofetheiOxMa/cKf+ 4aY7d1e6m8zd26QfBZVOKBNhD54OkMhDDF18IFoc9tus0PLb00FkSrdglYWwSGcXQyE7 8HueWnBVxTqu85g+W6KKRKFFrlycWLevFcldqIew0XhS6yxhi4gwdP+/wztX/ClSrV9T gHtw== X-Gm-Message-State: ALyK8tLov4H/be5sJzAjVk/WJmXadonflpQIgjB7T2zSWtXpZy2TQTksa6zQ1ruG6czn2t8m X-Received: by 10.66.183.69 with SMTP id ek5mr3686928pac.153.1464065793578; Mon, 23 May 2016 21:56:33 -0700 (PDT) Received: from localhost ([122.172.42.124]) by smtp.gmail.com with ESMTPSA id e7sm50370969pfa.28.2016.05.23.21.56.32 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 23 May 2016 21:56:32 -0700 (PDT) Date: Tue, 24 May 2016 10:26:30 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: "Rafael J. Wysocki" , Linux PM list , Linux Kernel Mailing List , Srinivas Pandruvada Subject: Re: [PATCH v2] cpufreq: stats: Walk online CPUs with CPU offline/online locked Message-ID: <20160524045630.GP17585@vireshk-i7> References: <19792191.esL3UuyORb@vostro.rjw.lan> <5580598.s7imtWPVt5@vostro.rjw.lan> <20160523035703.GF2850@vireshk-i7> <3351136.myUMJKBnm0@vostro.rjw.lan> <20160523151910.GC17585@vireshk-i7> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On 23-05-16, 22:47, Rafael J. Wysocki wrote: > Assuming that the loops are over online CPUs and not over possible > CPUs I suppose? I wasn't focussing on that loop lately but the policy->rwsem :) > Anyway, if you are talking about the code without the patch (which I > guess is the case), the reason why it is racy is because, if > cpufreq_stats_init() runs in parallel with CPU online, the CPU going > online may be missed by it. To my eyes that happens if > cpufreq_online() has already advanced beyond the point where the > notifier would have been invoked, but hasn't returned yet when the > for_each_online_cpu() loop in cpufreq_stats_init() is executed. Yes. That's a race we need to fix. I agree. > Worse yet, if a CPU goes offline when cpufreq_stats_exit() is running > and that happens exactly between the notifier unregistration and the > for_each_online_cpu() loop, the stats table will never be freed for > that CPU (say the policy isn't shared). Same here. > Switching over to loops over possible CPUs doesn't address those races > (at least not the second one), and I'm not really sure why I thought > it would address them, but adding CPU online/offline locking to > cpufreq_stats_init/exit() can address them, so it looks like the very > first version of my patch (ie. > https://patchwork.kernel.org/patch/9128509/) was actually correct, > because it didn't put too much code under the CPU offline/online > locking. :-) Well, I think there is one more way of getting all this fixed, which may eventually look much more cleaner. What if we update cpufreq core instead of stats with something like this: -------------------------8<------------------------- From: Viresh Kumar Date: Tue, 24 May 2016 10:16:25 +0530 Subject: [PATCH] cpufreq: Initiate notifiers for existing policy Races are possible in the init/exit paths of the cpufreq-stats layer, which may lead to 'stats' sysfs directory not getting created or removed for some of the policies. This can happen while the policy is getting created while cpufreq_stats_init/exit() are getting called. To avoid adding unnecessary locks in the init/exit paths of the cpufreq-stats layer, update the policy notifier register/unregister routines to send notifications for any existing cpufreq policies. Also make sure (with help of cpufreq_driver_lock) that CPUFREQ_CREATE/REMOVE notifiers aren't getting issued in parallel from the policy creation/removal paths. Signed-off-by: Viresh Kumar --- drivers/cpufreq/cpufreq.c | 35 +++++++++++++++++++++++++- drivers/cpufreq/cpufreq_stats.c | 55 ++++++----------------------------------- 2 files changed, 41 insertions(+), 49 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index c3f950f0e5f0..90f4bf03701d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1269,10 +1269,10 @@ static int cpufreq_online(unsigned int cpu) ret = cpufreq_add_dev_interface(policy); if (ret) goto out_exit_policy; + write_lock_irqsave(&cpufreq_driver_lock, flags); blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_CREATE_POLICY, policy); - write_lock_irqsave(&cpufreq_driver_lock, flags); list_add(&policy->policy_list, &cpufreq_policy_list); write_unlock_irqrestore(&cpufreq_driver_lock, flags); } @@ -1728,6 +1728,8 @@ EXPORT_SYMBOL_GPL(cpufreq_get_driver_data); */ int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list) { + struct cpufreq_policy *policy; + unsigned long flags; int ret; if (cpufreq_disabled()) @@ -1751,8 +1753,28 @@ int cpufreq_register_notifier(struct notifier_block *nb, unsigned int list) mutex_unlock(&cpufreq_fast_switch_lock); break; case CPUFREQ_POLICY_NOTIFIER: + write_lock_irqsave(&cpufreq_driver_lock, flags); + + /* Notify about all existing policies */ + for_each_policy(policy) { + nb->notifier_call(nb, CPUFREQ_CREATE_POLICY, + policy); + if (policy_is_inactive(policy)) + continue; + + nb->notifier_call(nb, CPUFREQ_START, policy); + } + ret = blocking_notifier_chain_register( &cpufreq_policy_notifier_list, nb); + if (ret) { + /* Notify about all existing policies */ + for_each_policy(policy) { + nb->notifier_call(nb, CPUFREQ_REMOVE_POLICY, + policy); + } + } + write_unlock_irqrestore(&cpufreq_driver_lock, flags); break; default: ret = -EINVAL; @@ -1774,6 +1796,8 @@ EXPORT_SYMBOL(cpufreq_register_notifier); */ int cpufreq_unregister_notifier(struct notifier_block *nb, unsigned int list) { + struct cpufreq_policy *policy; + unsigned long flags; int ret; if (cpufreq_disabled()) @@ -1793,6 +1817,15 @@ int cpufreq_unregister_notifier(struct notifier_block *nb, unsigned int list) case CPUFREQ_POLICY_NOTIFIER: ret = blocking_notifier_chain_unregister( &cpufreq_policy_notifier_list, nb); + if (!ret) { + write_lock_irqsave(&cpufreq_driver_lock, flags); + /* Notify about all existing policies */ + for_each_policy(policy) { + nb->notifier_call(nb, CPUFREQ_REMOVE_POLICY, + policy); + } + write_unlock_irqrestore(&cpufreq_driver_lock, flags); + } break; default: ret = -EINVAL; diff --git a/drivers/cpufreq/cpufreq_stats.c b/drivers/cpufreq/cpufreq_stats.c index 5e370a30a964..d4618144b4c0 100644 --- a/drivers/cpufreq/cpufreq_stats.c +++ b/drivers/cpufreq/cpufreq_stats.c @@ -130,7 +130,7 @@ static int freq_table_get_index(struct cpufreq_stats *stats, unsigned int freq) return -1; } -static void __cpufreq_stats_free_table(struct cpufreq_policy *policy) +static void cpufreq_stats_free_table(struct cpufreq_policy *policy) { struct cpufreq_stats *stats = policy->stats; @@ -146,20 +146,7 @@ static void __cpufreq_stats_free_table(struct cpufreq_policy *policy) policy->stats = NULL; } -static void cpufreq_stats_free_table(unsigned int cpu) -{ - struct cpufreq_policy *policy; - - policy = cpufreq_cpu_get(cpu); - if (!policy) - return; - - __cpufreq_stats_free_table(policy); - - cpufreq_cpu_put(policy); -} - -static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) +static int cpufreq_stats_create_table(struct cpufreq_policy *policy) { unsigned int i = 0, count = 0, ret = -ENOMEM; struct cpufreq_stats *stats; @@ -226,23 +213,6 @@ static int __cpufreq_stats_create_table(struct cpufreq_policy *policy) return ret; } -static void cpufreq_stats_create_table(unsigned int cpu) -{ - struct cpufreq_policy *policy; - - /* - * "likely(!policy)" because normally cpufreq_stats will be registered - * before cpufreq driver - */ - policy = cpufreq_cpu_get(cpu); - if (likely(!policy)) - return; - - __cpufreq_stats_create_table(policy); - - cpufreq_cpu_put(policy); -} - static int cpufreq_stat_notifier_policy(struct notifier_block *nb, unsigned long val, void *data) { @@ -250,9 +220,9 @@ static int cpufreq_stat_notifier_policy(struct notifier_block *nb, struct cpufreq_policy *policy = data; if (val == CPUFREQ_CREATE_POLICY) - ret = __cpufreq_stats_create_table(policy); + ret = cpufreq_stats_create_table(policy); else if (val == CPUFREQ_REMOVE_POLICY) - __cpufreq_stats_free_table(policy); + cpufreq_stats_free_table(policy); return ret; } @@ -314,7 +284,6 @@ static struct notifier_block notifier_trans_block = { static int __init cpufreq_stats_init(void) { int ret; - unsigned int cpu; spin_lock_init(&cpufreq_stats_lock); ret = cpufreq_register_notifier(¬ifier_policy_block, @@ -322,31 +291,21 @@ static int __init cpufreq_stats_init(void) if (ret) return ret; - for_each_online_cpu(cpu) - cpufreq_stats_create_table(cpu); - ret = cpufreq_register_notifier(¬ifier_trans_block, CPUFREQ_TRANSITION_NOTIFIER); if (ret) { cpufreq_unregister_notifier(¬ifier_policy_block, CPUFREQ_POLICY_NOTIFIER); - for_each_online_cpu(cpu) - cpufreq_stats_free_table(cpu); - return ret; } - return 0; + return ret; } static void __exit cpufreq_stats_exit(void) { - unsigned int cpu; - - cpufreq_unregister_notifier(¬ifier_policy_block, - CPUFREQ_POLICY_NOTIFIER); cpufreq_unregister_notifier(¬ifier_trans_block, CPUFREQ_TRANSITION_NOTIFIER); - for_each_online_cpu(cpu) - cpufreq_stats_free_table(cpu); + cpufreq_unregister_notifier(¬ifier_policy_block, + CPUFREQ_POLICY_NOTIFIER); } MODULE_AUTHOR("Zou Nan hai ");