Simple transition to work_on_cpu(), rather than cpumask games.
Signed-off-by: Rusty Russell <[email protected]>
Cc: Ingo Molnar <[email protected]>
---
arch/x86/kernel/cpu/intel_cacheinfo.c | 38 ++++++++++++++++------------------
1 file changed, 18 insertions(+), 20 deletions(-)
diff -r 6f4737048b99 arch/x86/kernel/cpu/intel_cacheinfo.c
--- a/arch/x86/kernel/cpu/intel_cacheinfo.c Thu Oct 23 00:06:30 2008 +1100
+++ b/arch/x86/kernel/cpu/intel_cacheinfo.c Thu Oct 23 00:20:57 2008 +1100
@@ -534,28 +534,13 @@ static void __cpuinit free_cache_attribu
per_cpu(cpuid4_info, cpu) = NULL;
}
-static int __cpuinit detect_cache_attributes(unsigned int cpu)
+static long get_cpu_leaves(void *unused)
{
- struct _cpuid4_info *this_leaf;
- unsigned long j;
- int retval;
- cpumask_t oldmask;
-
- if (num_cache_leaves == 0)
- return -ENOENT;
-
- per_cpu(cpuid4_info, cpu) = kzalloc(
- sizeof(struct _cpuid4_info) * num_cache_leaves, GFP_KERNEL);
- if (per_cpu(cpuid4_info, cpu) == NULL)
- return -ENOMEM;
-
- oldmask = current->cpus_allowed;
- retval = set_cpus_allowed_ptr(current, &cpumask_of_cpu(cpu));
- if (retval)
- goto out;
+ int j, retval = 0, cpu = smp_processor_id();
/* Do cpuid and store the results */
for (j = 0; j < num_cache_leaves; j++) {
+ struct _cpuid4_info *this_leaf;
this_leaf = CPUID4_INFO_IDX(cpu, j);
retval = cpuid4_cache_lookup(j, this_leaf);
if (unlikely(retval < 0)) {
@@ -567,9 +552,22 @@ static int __cpuinit detect_cache_attrib
}
cache_shared_cpu_map_setup(cpu, j);
}
- set_cpus_allowed_ptr(current, &oldmask);
+ return retval;
+}
-out:
+static int __cpuinit detect_cache_attributes(unsigned int cpu)
+{
+ int retval;
+
+ if (num_cache_leaves == 0)
+ return -ENOENT;
+
+ per_cpu(cpuid4_info, cpu) = kzalloc(
+ sizeof(struct _cpuid4_info) * num_cache_leaves, GFP_KERNEL);
+ if (per_cpu(cpuid4_info, cpu) == NULL)
+ return -ENOMEM;
+
+ retval = work_on_cpu(cpu, get_cpu_leaves, NULL);
if (retval) {
kfree(per_cpu(cpuid4_info, cpu));
per_cpu(cpuid4_info, cpu) = NULL;
On 10/23, Rusty Russell wrote:
>
> +static int __cpuinit detect_cache_attributes(unsigned int cpu)
> +{
> + int retval;
> +
> + if (num_cache_leaves == 0)
> + return -ENOENT;
> +
> + per_cpu(cpuid4_info, cpu) = kzalloc(
> + sizeof(struct _cpuid4_info) * num_cache_leaves, GFP_KERNEL);
> + if (per_cpu(cpuid4_info, cpu) == NULL)
> + return -ENOMEM;
> +
> + retval = work_on_cpu(cpu, get_cpu_leaves, NULL);
This doesn't look right.
cache_add_dev()->cpuid4_cache_sysfs_init()->detect_cache_attributes()
is called by CPU_ONLINE under cpu_hotplug_begin(), this is deadlockable.
Yes, the caller (cpu_hotplug.active_writer) can safely take
get_online_cpus(), but if we have another pending work which needs
get_online_cpus(), we have the deadlock.
Oleg.
On Thursday 23 October 2008 20:46:21 Oleg Nesterov wrote:
> On 10/23, Rusty Russell wrote:
> > +static int __cpuinit detect_cache_attributes(unsigned int cpu)
> > +{
> > + int retval;
> > +
> > + if (num_cache_leaves == 0)
> > + return -ENOENT;
> > +
> > + per_cpu(cpuid4_info, cpu) = kzalloc(
> > + sizeof(struct _cpuid4_info) * num_cache_leaves, GFP_KERNEL);
> > + if (per_cpu(cpuid4_info, cpu) == NULL)
> > + return -ENOMEM;
> > +
> > + retval = work_on_cpu(cpu, get_cpu_leaves, NULL);
>
> This doesn't look right.
>
> cache_add_dev()->cpuid4_cache_sysfs_init()->detect_cache_attributes()
> is called by CPU_ONLINE under cpu_hotplug_begin(), this is deadlockable.
>
> Yes, the caller (cpu_hotplug.active_writer) can safely take
> get_online_cpus(), but if we have another pending work which needs
> get_online_cpus(), we have the deadlock.
Hi Oleg,
Nice catch. Can we use smp_call_function here instead?
Rusty.
>
> Oleg.
On 10/24, Rusty Russell wrote:
>
> Can we use smp_call_function here instead?
I think you are right, smp_call_function_single() should be fine.
But please note I don't really understand this code ;)
Oleg.