Skip to contentSkip to conversation

powerpc: perfmon2 prereqs

PATCH In review Kevin Corry 2 participants 5 messages May 2, 2007 – May 3, 2007 PowerPC

lore
Share & tools
lore
#0 lore raw

cover powerpc: perfmon2 prereqs

Hi,

This is a repost of a couple patches I posted about a month ago related to porting perfmon2 to powerpc. I wanted to see if there were any further comments on these patches, and also wanted to ask if these should be submitted separately to the ppc kernel maintainers, or if they should be kept with the perfmon2 patches until those are submitted.

The first patch adds an smp_call_function_single() routine for powerpc. In 2.6.21, there's a prototype for this routine in include/linux/smp.h, and it is implemented on i386, ia64, and x86-64. Since this routine is very similar to the existing smp_call_function() routine, the common portions have been pulled out into __smp_call_function(). Perfmon2 uses this unload a context that's loaded on a CPU other than the one that's performing the close-context routine.

The second patch changes the powerpc version of topology_init() from an __initcall to a subsys_initcall, which matches the definition of topology_init() on all other architectures. Perfmon2's initialization is done as a subsys_initcall, but it fails to set up its sysfs information if topology_init() has not yet run. Changing perfmon2 to an __initcall() was discussed, but we came to the conclusion that this would not work because the perfmon2 core must be initialized before any of its sub-modules. These sub-modules are located in arch/*/perfmon/, and thus if they are built statically they would run their module_init routines before the perfmon2 core ran its __initcall routine.

We have been running tests with these patches for the last few weeks on Cell and Power5 systems without any problems.

Thanks,

Signature · 3 lines
Kevin Corry
[email protected]
http://www.ibm.com/linux/
#1 lore raw

PATCH 1/2 powerpc: add smp_call_function_single()

Add an smp_call_function_single() to the powerpc architecture. Since this is very similar to the existing smp_call_function() routine, the common portions have been split out into __smp_call_function(). Since the spin_lock(&call_lock) was moved to __smp_call_function(), smp_call_function() now explicitly calls preempt_disable() before getting the count of online CPUs.

arch/powerpc/kernel/smp.c +79 -35
Index: linux-2.6.21/arch/powerpc/kernel/smp.c
===================================================================
@@ -175,26 +175,11 @@ static struct call_data_struct {
 /* delay of at least 8 seconds */
 #define SMP_CALL_TIMEOUT	8
 
-/*
- * This function sends a 'generic call function' IPI to all other CPUs
- * in the system.
- *
- * [SUMMARY] Run a function on all other CPUs.
- * <func> The function to run. This must be fast and non-blocking.
- * <info> An arbitrary pointer to pass to the function.
- * <nonatomic> currently unused.
- * <wait> If true, wait (atomically) until function has completed on other CPUs.
- * [RETURNS] 0 on success, else a negative status code. Does not return until
- * remote CPUs are nearly ready to execute <<func>> or are or have executed.
- *
- * You must not call this function with disabled interrupts or from a
- * hardware interrupt handler or from a bottom half handler.
- */
-int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
-		       int wait)
-{ 
+static int __smp_call_function(void (*func)(void *info), void *info,
+			       int wait, int target_cpu, int num_cpus)
+{
 	struct call_data_struct data;
-	int ret = -1, cpus;
+	int ret = -1;
 	u64 timeout;
 
 	/* Can deadlock when called with interrupts disabled */
@@ -211,40 +196,33 @@ int smp_call_function (void (*func) (voi
 		atomic_set(&data.finished, 0);
 
 	spin_lock(&call_lock);
-	/* Must grab online cpu count with preempt disabled, otherwise
-	 * it can change. */
-	cpus = num_online_cpus() - 1;
-	if (!cpus) {
-		ret = 0;
-		goto out;
-	}
 
 	call_data = &data;
 	smp_wmb();
 	/* Send a message to all other CPUs and wait for them to respond */
-	smp_ops->message_pass(MSG_ALL_BUT_SELF, PPC_MSG_CALL_FUNCTION);
+	smp_ops->message_pass(target_cpu, PPC_MSG_CALL_FUNCTION);
 
 	timeout = get_tb() + (u64) SMP_CALL_TIMEOUT * tb_ticks_per_sec;
 
 	/* Wait for response */
-	while (atomic_read(&data.started) != cpus) {
+	while (atomic_read(&data.started) != num_cpus) {
 		HMT_low();
 		if (get_tb() >= timeout) {
-			printk("smp_call_function on cpu %d: other cpus not "
-			       "responding (%d)\n", smp_processor_id(),
-			       atomic_read(&data.started));
+			printk("%s on cpu %d: other cpus not "
+			       "responding (%d)\n", __FUNCTION__,
+			       smp_processor_id(), atomic_read(&data.started));
 			debugger(NULL);
 			goto out;
 		}
 	}
 
 	if (wait) {
-		while (atomic_read(&data.finished) != cpus) {
+		while (atomic_read(&data.finished) != num_cpus) {
 			HMT_low();
 			if (get_tb() >= timeout) {
-				printk("smp_call_function on cpu %d: other "
-				       "cpus not finishing (%d/%d)\n",
-				       smp_processor_id(),
+				printk("%s on cpu %d: other cpus "
+				       "not finishing (%d/%d)\n",
+				       __FUNCTION__, smp_processor_id(),
 				       atomic_read(&data.finished),
 				       atomic_read(&data.started));
 				debugger(NULL);
@@ -262,8 +240,74 @@ int smp_call_function (void (*func) (voi
 	return ret;
 }
 
+/*
+ * This function sends a 'generic call function' IPI to all other CPUs
+ * in the system.
+ *
+ * [SUMMARY] Run a function on all other CPUs.
+ * <func> The function to run. This must be fast and non-blocking.
+ * <info> An arbitrary pointer to pass to the function.
+ * <nonatomic> currently unused.
+ * <wait> If true, wait (atomically) until function has completed on other CPUs.
+ * [RETURNS] 0 on success, else a negative status code. Does not return until
+ * remote CPUs are nearly ready to execute <<func>> or are or have executed.
+ *
+ * You must not call this function with disabled interrupts or from a
+ * hardware interrupt handler or from a bottom half handler.
+ */
+int smp_call_function (void (*func) (void *info), void *info, int nonatomic,
+		       int wait)
+{
+	int num_cpus, ret = 0;
+
+	/* Must grab online cpu count with preempt disabled, otherwise
+	 * it can change. */
+	preempt_disable();
+	num_cpus = num_online_cpus() - 1;
+	if (num_cpus) {
+		ret = __smp_call_function(func, info, wait,
+					  MSG_ALL_BUT_SELF, num_cpus);
+	}
+	preempt_enable();
+	return ret;
+}
+
 EXPORT_SYMBOL(smp_call_function);
 
+/*
+ * This function sends a 'generic call function' IPI to the specified CPU.
+ *
+ * [SUMMARY] Run a function on the specified CPUs.
+ * <cpuid> The CPU to run the function on.
+ * <func> The function to run. This must be fast and non-blocking.
+ * <info> An arbitrary pointer to pass to the function.
+ * <nonatomic> currently unused.
+ * <wait> If true, wait (atomically) until function has completed on the
+ * other CPU.
+ * [RETURNS] 0 on success, else a negative status code. Does not return until
+ * remote CPU is nearly ready to execute <<func>> or are or has executed.
+ *
+ * You must not call this function with disabled interrupts or from a
+ * hardware interrupt handler or from a bottom half handler.
+ */
+int smp_call_function_single(int cpuid, void (*func)(void *info), void *info,
+			     int nonatomic, int wait)
+{
+	int ret;
+
+	/* Prevent preemption and reschedule on another processor */
+	if (get_cpu() == cpuid) {
+		printk(KERN_INFO "%s: trying to call self\n", __FUNCTION__);
+		ret = -EBUSY;
+	} else
+		ret = __smp_call_function(func, info, wait, cpuid, 1);
+
+	put_cpu();
+	return ret;
+}
+
+EXPORT_SYMBOL(smp_call_function_single);
+
 void smp_call_function_interrupt(void)
 {
 	void (*func) (void *info);
#2 lore raw

PATCH 2/2 powerpc: change topology_init() to a subsys_initcall

In reply to Kevin Corry

Change the powerpc version of topology_init() from an __initcall to a subsys_initcall to match all other architectures.

arch/powerpc/kernel/sysfs.c +1 -1
Index: linux-2.6.21/arch/powerpc/kernel/sysfs.c
===================================================================
@@ -498,4 +498,4 @@ static int __init topology_init(void)
 
 	return 0;
 }
-__initcall(topology_init);
+subsys_initcall(topology_init);
#3 lore raw

Re: [PATCH 2/2] powerpc: change topology_init() to a subsys_initcall

Quoted 10 lines · Kevin Corry

Change the powerpc version of topology_init() from an __initcall to a subsys_initcall to match all other architectures.

Index: linux-2.6.21/arch/powerpc/kernel/sysfs.c
===================================================================

arch/powerpc/kernel/sysfs.c static int __init topology_init(void) · line 501

Quoted 5 lines
arch/powerpc/kernel/sysfs.c +1 -1
@@ -498,4 +498,4 @@ static int __init topology_init(void)
 
 	return 0;
 }
-__initcall(topology_init);
+subsys_initcall(topology_init);      

topology_init() depends on the register_one_node() stuff being available, which relies on register_node_type() being called AFAICT - which is a postcore_initcall(). So that's OK.

It also creates sysfs files, which is OK because long before initcalls run vfs_caches_init() called mnt_init() which called sysfs_init().

Just to be super safe it'd be good to diff your sysfs before and after the change. But assuming that show's nothing this looks fine to me.

cheers

Signature · 8 lines
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
#4 lore raw

Re: [PATCH 2/2] powerpc: change topology_init() to a subsys_initcall

Hi Michael,

Quoted 21 lines · Michael Ellerman
Quoted 10 lines · Kevin Corry

Change the powerpc version of topology_init() from an __initcall to a subsys_initcall to match all other architectures.

Index: linux-2.6.21/arch/powerpc/kernel/sysfs.c
===================================================================

arch/powerpc/kernel/sysfs.c static int __init topology_init(void) · line 501

Quoted 5 lines
arch/powerpc/kernel/sysfs.c +1 -1
@@ -498,4 +498,4 @@ static int __init topology_init(void)
 
 	return 0;
 }
-__initcall(topology_init);
+subsys_initcall(topology_init);

topology_init() depends on the register_one_node() stuff being available, which relies on register_node_type() being called AFAICT - which is a postcore_initcall(). So that's OK.

Quoted 5 lines

It also creates sysfs files, which is OK because long before initcalls run vfs_caches_init() called mnt_init() which called sysfs_init().

Just to be super safe it'd be good to diff your sysfs before and after the change. But assuming that show's nothing this looks fine to me.

I booted with and without this patch applied, and the file listings of /sys on both kernels show no differences (other than timestamps).

Thanks for the feedback.

Signature · 3 lines
Kevin Corry
[email protected]
http://www.ibm.com/linux/