Re: [PATCH 1/2] OProfile - Enable SPU switch notification to detect currently active SPU tasks - update

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 20 Jul 2007 14:24:07 -0500
Bob Nelson <rrnelson@linux.vnet.ibm.com> wrote:

> From: Maynard Johnson <mpjohn@us.ibm.com>
> 
> This patch adds to the capability of spu_switch_event_register so that
> the caller is also notified of currently active SPU tasks.
> Exports spu_switch_event_register and spu_switch_event_unregister so
> that OProfile can get access to the notifications provided.
> 
> Signed-off-by: Maynard Johnson <mpjohn@us.ibm.com>
> Signed-off-by: Carl Love <carll@us.ibm.com>
> Signed-off-by: Bob Nelson <rrnelson@us.ibm.com>
> Acked-by: Arnd Bergmann <arnd.bergmann@de.ibm.com>
> Acked-by: Paul Mackerras <paulus@samba.org>
> 
> ---
> 
> We would like this patch included in -mm and 2.6.23
> 
> Changed "for (node = 0; node < MAX_NUMNODES; node++)" loop to 
> for_each_online_node(node).
> Added comment to memory barrier.
> Better info in changelog.

here it is:

--- a/arch/powerpc/platforms/cell/spufs/sched.c~oprofile-enable-spu-switch-notification-to-detect-currently-active-spu-tasks-update
+++ a/arch/powerpc/platforms/cell/spufs/sched.c
@@ -220,13 +220,14 @@ static void notify_spus_active(void)
 	 * When the awakened processes see their "notify_active" flag is set,
 	 * they will call spu_switch_notify();
 	 */
-	for (node = 0; node < MAX_NUMNODES; node++) {
+	for_each_online_node(node) {
 		struct spu *spu;
 		mutex_lock(&spu_prio->active_mutex[node]);
 		list_for_each_entry(spu, &spu_prio->active_list[node], list) {
 			struct spu_context *ctx = spu->ctx;
 			set_bit(SPU_SCHED_NOTIFY_ACTIVE, &ctx->sched_flags);
-			mb();
+			mb();	/* make sure any tasks woken up below */
+				/* can see the bit(s) set above */
 			wake_up_all(&ctx->stop_wq);
 		}
 		mutex_unlock(&spu_prio->active_mutex[node]);
_

I still wonder about that barrier.  At the least it should be smp_mb(). 
But aren't our set_bit() semantics _alone_ sufficient to make this barrier
unneeded?

If it _is_ possible for the effects of a set_bit() to not be visible to a
woken-up thread then I suspect we'll have nasty little problems in quite a
few places.  Maybe wake_up() should itself have a barrier to prevent such
things?

Doing that would be a documentation-only change, I suspect, given that the
current implementation of wake_up() starts out with a spin_lock_irqsave().