From patchwork Thu Jul 30 09:00:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Viresh Kumar X-Patchwork-Id: 51683 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f71.google.com (mail-la0-f71.google.com [209.85.215.71]) by patches.linaro.org (Postfix) with ESMTPS id 8717722DB5 for ; Thu, 30 Jul 2015 09:00:30 +0000 (UTC) Received: by lagw2 with SMTP id w2sf11789148lag.3 for ; Thu, 30 Jul 2015 02:00:29 -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:date:from:to:cc:subject:message-id :references:mime-version:content-type:content-disposition :in-reply-to:user-agent:sender:precedence:list-id:x-original-sender :x-original-authentication-results:mailing-list:list-post:list-help :list-archive:list-unsubscribe; bh=06Cl7dkfBnqTNGErPM9G78/sfdEpZTqdn1WP0JBE49o=; b=icap6jdNWEgrY4jH6J2hq99FakaueTevFV6uUzse5jmdQMP+FPHgtF3Em+oVxjtPsW ib3piXdqj5RjqPUXyXqTwncwzAbDTIR1fDqZKaos0zAGKo0EPA4aTb33gjeQzeuFtNar Q6ra0/dvnLoY6RDSEwBtjUKudltylx6NCvKLn24tbGwqSIigN4xUA8fZJrIFd4ZyMKkC xDlrIPVyokQtrBFr+Izd9nfMxiCg+EfnlrI69uWrnJcOq/uD5ZFiBAdDvuE92kM76mM7 f4GxpUj7sSr0Sf5/p860qjUBBLrp2DUGLR3zOalFBYiLqCfhaTXMDJfjxS5OL90ZzlK/ ZILw== X-Gm-Message-State: ALoCoQmmqs8b3suCGEwO+BYj8LiVyCMx+bzQGfBEdVG5s2cxANk1SFkH+W1/H9S1qxL7lQE1IUGI X-Received: by 10.152.1.105 with SMTP id 9mr17354892lal.3.1438246829418; Thu, 30 Jul 2015 02:00:29 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.163.65 with SMTP id yg1ls141494lab.26.gmail; Thu, 30 Jul 2015 02:00:29 -0700 (PDT) X-Received: by 10.152.8.38 with SMTP id o6mr43652446laa.116.1438246829238; Thu, 30 Jul 2015 02:00:29 -0700 (PDT) Received: from mail-la0-f44.google.com (mail-la0-f44.google.com. [209.85.215.44]) by mx.google.com with ESMTPS id kh10si326028lbc.24.2015.07.30.02.00.29 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 30 Jul 2015 02:00:29 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.44 as permitted sender) client-ip=209.85.215.44; Received: by lafd3 with SMTP id d3so21115852laf.1 for ; Thu, 30 Jul 2015 02:00:29 -0700 (PDT) X-Received: by 10.112.125.34 with SMTP id mn2mr39114232lbb.76.1438246829082; Thu, 30 Jul 2015 02:00:29 -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.7.198 with SMTP id l6csp426791lba; Thu, 30 Jul 2015 02:00:27 -0700 (PDT) X-Received: by 10.66.119.174 with SMTP id kv14mr92818622pab.115.1438246826728; Thu, 30 Jul 2015 02:00:26 -0700 (PDT) Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id ds4si847215pac.237.2015.07.30.02.00.25; Thu, 30 Jul 2015 02:00:26 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-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 S1755117AbbG3JAX (ORCPT + 26 others); Thu, 30 Jul 2015 05:00:23 -0400 Received: from mail-pd0-f173.google.com ([209.85.192.173]:36327 "EHLO mail-pd0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754127AbbG3JAT (ORCPT ); Thu, 30 Jul 2015 05:00:19 -0400 Received: by pdjr16 with SMTP id r16so21598470pdj.3 for ; Thu, 30 Jul 2015 02:00:19 -0700 (PDT) X-Received: by 10.70.20.5 with SMTP id j5mr103540129pde.40.1438246819196; Thu, 30 Jul 2015 02:00:19 -0700 (PDT) Received: from localhost ([122.171.186.190]) by smtp.gmail.com with ESMTPSA id ym6sm885857pac.32.2015.07.30.02.00.17 (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Thu, 30 Jul 2015 02:00:18 -0700 (PDT) Date: Thu, 30 Jul 2015 14:30:13 +0530 From: Viresh Kumar To: "Rafael J. Wysocki" Cc: Russell King - ARM Linux , "Rafael J. Wysocki" , Linux PM list , Linux Kernel Mailing List Subject: Re: [PATCH v2] cpufreq: Avoid attempts to create duplicate symbolic links Message-ID: <20150730090013.GE31351@linux> References: <1660815.CyKx9SEI9c@vostro.rjw.lan> <4080510.IQ60sVQvbL@vostro.rjw.lan> <20150727143935.GB18535@linux> <2112385.YuDJ7h1x56@vostro.rjw.lan> <20150729091136.GN7557@n2100.arm.linux.org.uk> <20150729142148.GF5100@linux> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: list List-ID: X-Mailing-List: linux-kernel@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.215.44 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: , I am not going to fight hard to prove a point as the code is in good working conditions, but wanted to finish the discussion .. On 29-07-15, 22:32, Rafael J. Wysocki wrote: > On Wed, Jul 29, 2015 at 4:21 PM, Viresh Kumar wrote: > > On 29-07-15, 15:57, Rafael J. Wysocki wrote: > >> In practice, this means a cpufreq driver registration done in parallel > >> with CPU hotplug. > > > > Not necessarily. Also consider the case where cpufreq driver is already working > > for a set of CPUs. And a new set of CPUs (that will share the policy) are > > getting physically added to the system. > > That's what I mean by "hotplug" (as opposed to online/offline). You were talking about driver registration done in parallel with hotplug. But I was pointing at something else. Suppose a system that has: - 8 CPUs, 0-3 and 4-7 share policy - 4-7 physically hotplugged out - cpufreq driver registered and so policy0 active for cpu 0-3. - We hotplug 4-7. So, this is a bit different compared to the case where Russell mentioned the Racy thing. Not sure if this case also has similary racy situations though. > Problem is, we can't do that for all of them. Right. > If the CPU is ofline > while being registered (the common case for hot-add) and it doesn't > have a policy already, we can't link it to an existing policy anyway, > so that operation has to be carried out later. That is, we need to > create links for those CPUs after the policy has been created in any > case. Right, but at least the cpufreq-core is already requested on behalf of such CPUs. We aren't creating links based on the assumption that a add_dev() will be called for these devices. > Moreover, the only case when we need to create links for online CPUs offline CPUs as well.. > is the registration of a cpufreq driver, because only then we can see > online CPUs that should be linked to a policy, but aren't. It takes > less code to treat them in the same way as the offline CPUs at that > point (and create the links for them right after the policy has been > created) than to defer it to until sif->add() is called for each of > them, because we know that sif->add() is practically guaraneed to > succeeed for them (this is the code path in which we call > cpufreq_add_policy_cpu() and the governor "stop" only fails if it > hasn't been started for that policy). Choose whatever you feel is right. I have already written below code, so just pasting it here. Its not to say, that this code looks better :P --- drivers/cpufreq/cpufreq.c | 108 +++++++++++++++++++++------------------------- 1 file changed, 48 insertions(+), 60 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 24e4ba568e77..87faabce777d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -31,6 +31,12 @@ #include #include +/* + * CPUs that were offline when a request to allocate policy was issued, symlinks + * for them should be created once the policy is available for them. + */ +cpumask_t real_cpus_pending; + static LIST_HEAD(cpufreq_policy_list); static inline bool policy_is_inactive(struct cpufreq_policy *policy) @@ -941,62 +947,46 @@ EXPORT_SYMBOL(cpufreq_sysfs_remove_file); static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu) { struct device *cpu_dev; - - pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu); - - if (!policy) - return 0; + int ret; cpu_dev = get_cpu_device(cpu); if (WARN_ON(!cpu_dev)) return 0; - return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq"); -} - -static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu) -{ - struct device *cpu_dev; - - pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu); + dev_dbg(cpu_dev, "%s: Adding symlink for CPU: %u\n", __func__, cpu); - cpu_dev = get_cpu_device(cpu); - if (WARN_ON(!cpu_dev)) - return; + ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq"); + if (ret) + dev_err(cpu_dev, "%s: Failed to create link (%d)\n", __func__, + ret); + else + cpumask_set_cpu(cpu, policy->real_cpus); - sysfs_remove_link(&cpu_dev->kobj, "cpufreq"); + return ret; } -/* Add/remove symlinks for all related CPUs */ -static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy) +/* + * Create symlinks for CPUs which are already added via subsys callbacks (and + * were offline then), before the policy was created. + */ +static int cpufreq_add_pending_symlinks(struct cpufreq_policy *policy) { - unsigned int j; - int ret = 0; + struct cpumask mask; + int cpu, ret; + + cpumask_and(&mask, policy->related_cpus, &real_cpus_pending); - /* Some related CPUs might not be present (physically hotplugged) */ - for_each_cpu(j, policy->real_cpus) { - if (j == policy->kobj_cpu) - continue; + if (cpumask_empty(&mask)) + return 0; - ret = add_cpu_dev_symlink(policy, j); + for_each_cpu(cpu, &mask) { + ret = add_cpu_dev_symlink(policy, cpu); if (ret) - break; + return ret; + cpumask_clear_cpu(cpu, &real_cpus_pending); } - return ret; -} - -static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy) -{ - unsigned int j; - - /* Some related CPUs might not be present (physically hotplugged) */ - for_each_cpu(j, policy->real_cpus) { - if (j == policy->kobj_cpu) - continue; - - remove_cpu_dev_symlink(policy, j); - } + return 0; } static int cpufreq_add_dev_interface(struct cpufreq_policy *policy) @@ -1028,7 +1018,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy) return ret; } - return cpufreq_add_dev_symlink(policy); + return cpufreq_add_pending_symlinks(policy); } static int cpufreq_init_policy(struct cpufreq_policy *policy) @@ -1155,7 +1145,6 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify) CPUFREQ_REMOVE_POLICY, policy); down_write(&policy->rwsem); - cpufreq_remove_dev_symlink(policy); kobj = &policy->kobj; cmp = &policy->kobj_unregister; up_write(&policy->rwsem); @@ -1235,10 +1224,9 @@ static int cpufreq_online(unsigned int cpu) down_write(&policy->rwsem); if (new_policy) { + cpumask_copy(policy->real_cpus, cpumask_of(cpu)); /* related_cpus should at least include policy->cpus. */ cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); - /* Remember CPUs present at the policy creation time. */ - cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask); } /* @@ -1363,23 +1351,17 @@ static int cpufreq_online(unsigned int cpu) static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif) { unsigned cpu = dev->id; - int ret; + struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); + int ret = 0; dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu); - if (cpu_online(cpu)) { + if (policy) + ret = add_cpu_dev_symlink(policy, cpu); + else if (cpu_online(cpu)) ret = cpufreq_online(cpu); - } else { - /* - * A hotplug notifier will follow and we will handle it as CPU - * online then. For now, just create the sysfs link, unless - * there is no policy or the link is already present. - */ - struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); - - ret = policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus) - ? add_cpu_dev_symlink(policy, cpu) : 0; - } + else + cpumask_set_cpu(cpu, &real_cpus_pending); return ret; } @@ -1469,8 +1451,10 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif) unsigned int cpu = dev->id; struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); - if (!policy) + if (!policy) { + cpumask_clear_cpu(cpu, &real_cpus_pending); return 0; + } if (cpu_online(cpu)) { cpufreq_offline_prepare(cpu); @@ -1485,7 +1469,8 @@ static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif) } if (cpu != policy->kobj_cpu) { - remove_cpu_dev_symlink(policy, cpu); + dev_dbg(dev, "%s: Removing symlink\n", __func__); + sysfs_remove_link(&dev->kobj, "cpufreq"); } else { /* * The CPU owning the policy object is going away. Move it to @@ -2550,6 +2535,9 @@ int cpufreq_unregister_driver(struct cpufreq_driver *driver) if (cpufreq_boost_supported()) cpufreq_sysfs_remove_file(&boost.attr); + if (WARN_ON(!cpumask_empty(&real_cpus_pending))) + cpumask_clear(&real_cpus_pending); + unregister_hotcpu_notifier(&cpufreq_cpu_notifier); write_lock_irqsave(&cpufreq_driver_lock, flags);