[PATCH 0/2] powerpc: perfmon2 prereqs

[PATCH 0/2] powerpc: perfmon2 prereqs

[Kevin Corry]

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,
-- 
Kevin Corry
kevcorry@us.ibm.com
http://www.ibm.com/linux/

[PATCH 1/2] powerpc: add smp_call_function_single()

[Kevin Corry]

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.

Signed-off-by: Kevin Corry <kevcorry@us.ibm.com>

Index: linux-2.6.21/arch/powerpc/kernel/smp.c
===================================================================
--- linux-2.6.21.orig/arch/powerpc/kernel/smp.c
+++ 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);

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

[Kevin Corry]

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

Signed-off-by: Kevin Corry <kevcorry@us.ibm.com>

Index: linux-2.6.21/arch/powerpc/kernel/sysfs.c
===================================================================
--- linux-2.6.21.orig/arch/powerpc/kernel/sysfs.c
+++ 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);

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1354 bytes --]

On Wed, 2007-05-02 at 12:11 -0500, Kevin Corry wrote:
> Change the powerpc version of topology_init() from an __initcall to
> a subsys_initcall to match all other architectures.
> 
> Signed-off-by: Kevin Corry <kevcorry@us.ibm.com>
> 
> Index: linux-2.6.21/arch/powerpc/kernel/sysfs.c
> ===================================================================
> --- linux-2.6.21.orig/arch/powerpc/kernel/sysfs.c
> +++ 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);      

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

-- 
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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Kevin Corry]

Hi Michael,

On Wed May 2 2007 8:11 pm, Michael Ellerman wrote:
> On Wed, 2007-05-02 at 12:11 -0500, Kevin Corry wrote:
> > Change the powerpc version of topology_init() from an __initcall to
> > a subsys_initcall to match all other architectures.
> >
> > Signed-off-by: Kevin Corry <kevcorry@us.ibm.com>
> >
> > Index: linux-2.6.21/arch/powerpc/kernel/sysfs.c
> > ===================================================================
> > --- linux-2.6.21.orig/arch/powerpc/kernel/sysfs.c
> > +++ 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);
>
> 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.

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.
-- 
Kevin Corry
kevcorry@us.ibm.com
http://www.ibm.com/linux/