Skip to contentSkip to conversation

work_on_cpu: use on x86/kernel/cpu/intel_cacheinfo.c

PATCH In review Rusty Russell 3 participants 4 messages Oct 23, 2008 x86

lore
Share & tools
lore

Rusty Russell

author
#0 lore raw

PATCH 6/7 work_on_cpu: use on x86/kernel/cpu/intel_cacheinfo.c

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(-)
arch/x86/kernel/cpu/intel_cacheinfo.c +18 -20
diff -r 6f4737048b99 arch/x86/kernel/cpu/intel_cacheinfo.c
@@ -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;
#1 lore raw
Quoted 4 lines · Rusty Russell
arch/x86/kernel/cpu/intel_cacheinfo.c +3 -0
+static int __cpuinit detect_cache_attributes(unsigned int cpu)
+{
+	int			retval;

arch/x86/kernel/cpu/intel_cacheinfo.c static int __cpuinit detect_cache_attrib · line 570

Quoted 10 lines
arch/x86/kernel/cpu/intel_cacheinfo.c +10 -0
+
+	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.

author
#2 lore raw
Quoted 17 lines · On Thursday 23 October 2008 20:46:21 Oleg Nesterov wrote:

arch/x86/kernel/cpu/intel_cacheinfo.c static int __cpuinit detect_cache_attrib · line 570

Quoted 13 lines · Rusty Russell
arch/x86/kernel/cpu/intel_cacheinfo.c +13 -0
+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.

Quoted 6 lines

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.

Quoted 2 lines

Oleg.

#3 lore raw
Quoted 2 lines · Rusty Russell

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.