| Message ID | 1660815.CyKx9SEI9c@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 23-07-15, 23:14, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > After commit 87549141d516 (cpufreq: Stop migrating sysfs files on > hotplug) there is a problem with CPUs that share cpufreq policy > objects with other CPUs and are initially offline. > > Say CPU1 shares a policy with CPU0 which is online and is registered > first. As part of the registration process, cpufreq_add_dev() is > called for it. It creates the policy object and a symbolic link > to it from the CPU1's sysfs directory. If CPU1 is registered > subsequently and it is offline at that time, cpufreq_add_dev() will > attempt to create a symbolic link to the policy object for it, but > that link is present already, so a warning about that will be > triggered. > > To avoid that warning, make cpufreq use an additional CPU mask > containing related CPUs that are actually present for each policy > object. That mask is initialized when the policy object is populated > after its creation (for the first online CPU using it) and it includes > CPUs from the "policy CPUs" mask returned by the cpufreq driver's > ->init() callback that are physically present at that time. Symbolic > links to the policy are created only for the CPUs in that mask. > > If cpufreq_add_dev() is invoked for an offline CPU, it checks the > new mask and only creates the symlink if the CPU was not in it (the > CPU is added to the mask at the same time). > > In turn, cpufreq_remove_dev() drops the given CPU from the new mask, > removes its symlink to the policy object and returns, unless it is > the CPU owning the policy object. In that case, the policy object > is moved to a new CPU's sysfs directory or deleted if the CPU being > removed was the last user of the policy. > > While at it, notice that cpufreq_remove_dev() can't fail, because > its return value is ignored, so make it ignore return values from > __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish() > and prevent these functions from aborting on errors returned by > __cpufreq_governor(). > > Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug) > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > Reported-by: Russell King <linux@arm.linux.org.uk> > --- > > This is supposed to replace the other patches sent so far to address the issue > at hand. Lets take this one and leave my patches. They are generating more diff and actually doing part of the general improvements Russell suggested. > + if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL)) I was wondering if we should use cpumask_t type variables, so that we don't have to allocate these masks. They are always with policies. > @@ -1307,6 +1316,9 @@ static int cpufreq_add_dev(struct device > /* related cpus should atleast have policy->cpus */ > cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); > > + cpumask_and(policy->cpus, policy->cpus, cpu_present_mask); > + cpumask_or(policy->real_cpus, policy->real_cpus, policy->cpus); > + I will do this differently: cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask); policy->cpus is anyway going to be anded with online mask. > /* > * affected cpus must always be the one, which are online. We aren't > * managing offline cpus here. > static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif) > { > - ret = __cpufreq_remove_dev_prepare(dev, sif); > + if (cpu != policy->kobj_cpu) { > + remove_cpu_dev_symlink(policy, cpu); > + } else { > + /* > + * This is the CPU owning the policy object. Move it to another > + * suitable CPU. > + */ > + unsigned int new_cpu = cpumask_first(policy->real_cpus); > + struct device *new_dev = get_cpu_device(new_cpu); > > - if (!ret) > - ret = __cpufreq_remove_dev_finish(dev, sif); > + dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu); > > - return ret; > + policy->kobj_cpu = new_cpu; You need to remove the link for the target cpu, like what I did in my patch: sysfs_remove_link(&new_dev->kobj, "cpufreq"); > + WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj)); > + } > + > + return 0; > } > > static void handle_update(struct work_struct *work)
On Friday, July 24, 2015 07:39:43 PM Viresh Kumar wrote: > On 23-07-15, 23:14, Rafael J. Wysocki wrote: > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > After commit 87549141d516 (cpufreq: Stop migrating sysfs files on > > hotplug) there is a problem with CPUs that share cpufreq policy > > objects with other CPUs and are initially offline. > > > > Say CPU1 shares a policy with CPU0 which is online and is registered > > first. As part of the registration process, cpufreq_add_dev() is > > called for it. It creates the policy object and a symbolic link > > to it from the CPU1's sysfs directory. If CPU1 is registered > > subsequently and it is offline at that time, cpufreq_add_dev() will > > attempt to create a symbolic link to the policy object for it, but > > that link is present already, so a warning about that will be > > triggered. > > > > To avoid that warning, make cpufreq use an additional CPU mask > > containing related CPUs that are actually present for each policy > > object. That mask is initialized when the policy object is populated > > after its creation (for the first online CPU using it) and it includes > > CPUs from the "policy CPUs" mask returned by the cpufreq driver's > > ->init() callback that are physically present at that time. Symbolic > > links to the policy are created only for the CPUs in that mask. > > > > If cpufreq_add_dev() is invoked for an offline CPU, it checks the > > new mask and only creates the symlink if the CPU was not in it (the > > CPU is added to the mask at the same time). > > > > In turn, cpufreq_remove_dev() drops the given CPU from the new mask, > > removes its symlink to the policy object and returns, unless it is > > the CPU owning the policy object. In that case, the policy object > > is moved to a new CPU's sysfs directory or deleted if the CPU being > > removed was the last user of the policy. > > > > While at it, notice that cpufreq_remove_dev() can't fail, because > > its return value is ignored, so make it ignore return values from > > __cpufreq_remove_dev_prepare() and __cpufreq_remove_dev_finish() > > and prevent these functions from aborting on errors returned by > > __cpufreq_governor(). > > > > Fixes: 87549141d516 (cpufreq: Stop migrating sysfs files on hotplug) > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Reported-by: Russell King <linux@arm.linux.org.uk> > > --- > > > > This is supposed to replace the other patches sent so far to address the issue > > at hand. > > Lets take this one and leave my patches. They are generating more > diff and actually doing part of the general improvements Russell > suggested. OK, but we can do better than this. :-) (See below) > > + if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL)) > > I was wondering if we should use cpumask_t type variables, so that we > don't have to allocate these masks. They are always with policies. We can do that, but let's do it as a cleanup later. > > @@ -1307,6 +1316,9 @@ static int cpufreq_add_dev(struct device > > /* related cpus should atleast have policy->cpus */ > > cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); > > > > + cpumask_and(policy->cpus, policy->cpus, cpu_present_mask); > > + cpumask_or(policy->real_cpus, policy->real_cpus, policy->cpus); > > + > > I will do this differently: > cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask); > > policy->cpus is anyway going to be anded with online mask. Right. > > /* > > * affected cpus must always be the one, which are online. We aren't > > * managing offline cpus here. > > > > static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif) > > { > > > - ret = __cpufreq_remove_dev_prepare(dev, sif); > > + if (cpu != policy->kobj_cpu) { > > + remove_cpu_dev_symlink(policy, cpu); > > + } else { > > + /* > > + * This is the CPU owning the policy object. Move it to another > > + * suitable CPU. > > + */ > > + unsigned int new_cpu = cpumask_first(policy->real_cpus); > > + struct device *new_dev = get_cpu_device(new_cpu); > > > > - if (!ret) > > - ret = __cpufreq_remove_dev_finish(dev, sif); > > + dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu); > > > > - return ret; > > + policy->kobj_cpu = new_cpu; > > You need to remove the link for the target cpu, like what I did in my > patch: Right. Plus I forgot to drop the policy freeing from __cpufreq_remove_dev_finish(). > sysfs_remove_link(&new_dev->kobj, "cpufreq"); > > > + WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj)); > > + } > > + > > + return 0; > > } > > > > static void handle_update(struct work_struct *work) Still, as I said before, we can do better here. The key observation is that if a CPU doesn't go online at all, we don't need to care about it. Hence, we can ignore a CPU until it goes online for the first time and we only need to create policy symlinks for online CPUs (just like we only create policy objects for online CPUs). Of course, we need to avoid creating a duplicate policy symlink if the CPU has been online at least once before, but that's quite straightforward. I'll send an updated patch shortly. Thanks, Rafael -- 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
Index: linux-pm/include/linux/cpufreq.h =================================================================== --- linux-pm.orig/include/linux/cpufreq.h +++ linux-pm/include/linux/cpufreq.h @@ -62,6 +62,7 @@ struct cpufreq_policy { /* CPUs sharing clock, require sw coordination */ cpumask_var_t cpus; /* Online CPUs only */ cpumask_var_t related_cpus; /* Online + Offline CPUs */ + cpumask_var_t real_cpus; /* Related and present */ unsigned int shared_type; /* ACPI: ANY or ALL affected CPUs should set cpufreq */ Index: linux-pm/drivers/cpufreq/cpufreq.c =================================================================== --- linux-pm.orig/drivers/cpufreq/cpufreq.c +++ linux-pm/drivers/cpufreq/cpufreq.c @@ -1002,7 +1002,7 @@ static int cpufreq_add_dev_symlink(struc int ret = 0; /* Some related CPUs might not be present (physically hotplugged) */ - for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) { + for_each_cpu(j, policy->real_cpus) { if (j == policy->kobj_cpu) continue; @@ -1019,7 +1019,7 @@ static void cpufreq_remove_dev_symlink(s unsigned int j; /* Some related CPUs might not be present (physically hotplugged) */ - for_each_cpu_and(j, policy->related_cpus, cpu_present_mask) { + for_each_cpu(j, policy->real_cpus) { if (j == policy->kobj_cpu) continue; @@ -1163,11 +1163,14 @@ static struct cpufreq_policy *cpufreq_po if (!zalloc_cpumask_var(&policy->related_cpus, GFP_KERNEL)) goto err_free_cpumask; + if (!zalloc_cpumask_var(&policy->real_cpus, GFP_KERNEL)) + goto err_free_rcpumask; + ret = kobject_init_and_add(&policy->kobj, &ktype_cpufreq, &dev->kobj, "cpufreq"); if (ret) { pr_err("%s: failed to init policy->kobj: %d\n", __func__, ret); - goto err_free_rcpumask; + goto err_free_real_cpus; } INIT_LIST_HEAD(&policy->policy_list); @@ -1184,6 +1187,8 @@ static struct cpufreq_policy *cpufreq_po return policy; +err_free_real_cpus: + free_cpumask_var(policy->real_cpus); err_free_rcpumask: free_cpumask_var(policy->related_cpus); err_free_cpumask: @@ -1234,6 +1239,7 @@ static void cpufreq_policy_free(struct c write_unlock_irqrestore(&cpufreq_driver_lock, flags); cpufreq_policy_put_kobj(policy, notify); + free_cpumask_var(policy->real_cpus); free_cpumask_var(policy->related_cpus); free_cpumask_var(policy->cpus); kfree(policy); @@ -1258,14 +1264,17 @@ static int cpufreq_add_dev(struct device pr_debug("adding CPU %u\n", cpu); - /* - * Only possible if 'cpu' wasn't physically present earlier and we are - * here from subsys_interface add callback. A hotplug notifier will - * follow and we will handle it like logical CPU hotplug then. For now, - * just create the sysfs link. - */ - if (cpu_is_offline(cpu)) - return add_cpu_dev_symlink(per_cpu(cpufreq_cpu_data, cpu), cpu); + if (cpu_is_offline(cpu)) { + /* + * Only possible if we are here from the subsys_interface add + * callback. A hotplug notifier will follow and we will handle + * it as logical CPU hotplug then. For now, just create the + * sysfs link, unless there is no policy. + */ + policy = per_cpu(cpufreq_cpu_data, cpu); + return policy && !cpumask_test_and_set_cpu(cpu, policy->real_cpus) + ? add_cpu_dev_symlink(policy, cpu) : 0; + } if (!down_read_trylock(&cpufreq_rwsem)) return 0; @@ -1307,6 +1316,9 @@ static int cpufreq_add_dev(struct device /* related cpus should atleast have policy->cpus */ cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); + cpumask_and(policy->cpus, policy->cpus, cpu_present_mask); + cpumask_or(policy->real_cpus, policy->real_cpus, policy->cpus); + /* * affected cpus must always be the one, which are online. We aren't * managing offline cpus here. @@ -1437,10 +1449,8 @@ static int __cpufreq_remove_dev_prepare( if (has_target()) { ret = __cpufreq_governor(policy, CPUFREQ_GOV_STOP); - if (ret) { + if (ret) pr_err("%s: Failed to stop governor\n", __func__); - return ret; - } } down_write(&policy->rwsem); @@ -1492,10 +1502,8 @@ static int __cpufreq_remove_dev_finish(s /* If cpu is last user of policy, free policy */ if (has_target()) { ret = __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT); - if (ret) { + if (ret) pr_err("%s: Failed to exit governor\n", __func__); - return ret; - } } /* @@ -1521,42 +1529,40 @@ static int __cpufreq_remove_dev_finish(s static int cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif) { unsigned int cpu = dev->id; - int ret; - - /* - * Only possible if 'cpu' is getting physically removed now. A hotplug - * notifier should have already been called and we just need to remove - * link or free policy here. - */ - if (cpu_is_offline(cpu)) { - struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); - struct cpumask mask; + struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); - if (!policy) - return 0; + if (!policy) + return 0; - cpumask_copy(&mask, policy->related_cpus); - cpumask_clear_cpu(cpu, &mask); + if (cpu_online(cpu)) { + __cpufreq_remove_dev_prepare(dev, sif); + __cpufreq_remove_dev_finish(dev, sif); + } - /* - * Free policy only if all policy->related_cpus are removed - * physically. - */ - if (cpumask_intersects(&mask, cpu_present_mask)) { - remove_cpu_dev_symlink(policy, cpu); - return 0; - } + cpumask_clear_cpu(cpu, policy->real_cpus); + if (cpumask_empty(policy->real_cpus)) { cpufreq_policy_free(policy, true); return 0; } - ret = __cpufreq_remove_dev_prepare(dev, sif); + if (cpu != policy->kobj_cpu) { + remove_cpu_dev_symlink(policy, cpu); + } else { + /* + * This is the CPU owning the policy object. Move it to another + * suitable CPU. + */ + unsigned int new_cpu = cpumask_first(policy->real_cpus); + struct device *new_dev = get_cpu_device(new_cpu); - if (!ret) - ret = __cpufreq_remove_dev_finish(dev, sif); + dev_dbg(dev, "%s: Moving policy object to CPU%u\n", __func__, new_cpu); - return ret; + policy->kobj_cpu = new_cpu; + WARN_ON(kobject_move(&policy->kobj, &new_dev->kobj)); + } + + return 0; } static void handle_update(struct work_struct *work)