Skip to contentSkip to conversation

Cell SPU task notification

PATCH In review Maynard Johnson 3 participants 8 messages Jan 12, 2007 – Jan 26, 2007

Subsystems (3)
PowerPC Cell broadband engine architecture SPU file system
lore
Share & tools
lore

Maynard Johnson

author
#0 lore raw

PATCH 1/1 Cell SPU task notification

Subject: Enable SPU switch notification to detect currently active SPU tasks.

From: Maynard Johnson <[email protected]>

This patch adds to the capability of spu_switch_event_register so that the caller is also notified of currently active SPU tasks. It also exports spu_switch_event_register and spu_switch_event_unregister.

Signed-off-by Maynard Johnson <[email protected]>
arch/powerpc/platforms/cell/spufs/sched.c +39 -2
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================
@@ -46,6 +46,8 @@
 
 #define SPU_MIN_TIMESLICE 	(100 * HZ / 1000)
 
+int notify_active[MAX_NUMNODES];
+
 #define SPU_BITMAP_SIZE (((MAX_PRIO+BITS_PER_LONG)/BITS_PER_LONG)+1)
 struct spu_prio_array {
 	unsigned long bitmap[SPU_BITMAP_SIZE];

@@ -81,18 +83,45 @@
 static void spu_switch_notify(struct spu *spu, struct spu_context *ctx)
 {
 	blocking_notifier_call_chain(&spu_switch_notifier,
-			    ctx ? ctx->object_id : 0, spu);
+				     ctx ? ctx->object_id : 0, spu);
+}
+
+static void notify_spus_active(void)
+{
+	int node;
+	/* Wake up the active spu_contexts. When the awakened processes 
+	 * sees their notify_active flag is set, they will call
+	 * spu_notify_already_active().
+	 */
+	for (node = 0; node < MAX_NUMNODES; 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;
+			wake_up_all(&ctx->stop_wq);
+			notify_active[ctx->spu->number] = 1;
+			smp_mb();
+		}
+                mutex_unlock(&spu_prio->active_mutex[node]);
+	}
+	yield();
 }
 
 int spu_switch_event_register(struct notifier_block * n)
 {
-	return blocking_notifier_chain_register(&spu_switch_notifier, n);
+	int ret;
+	ret = blocking_notifier_chain_register(&spu_switch_notifier, n);
+	if (!ret)
+		notify_spus_active();
+	return ret;
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_register);
 
 int spu_switch_event_unregister(struct notifier_block * n)
 {
 	return blocking_notifier_chain_unregister(&spu_switch_notifier, n);
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_unregister);
 
 
 static inline void bind_context(struct spu *spu, struct spu_context *ctx)

@@ -250,6 +279,14 @@
 	return spu_get_idle(ctx, flags);
 }
 
+void spu_notify_already_active(struct spu_context *ctx)
+{
+	struct spu *spu = ctx->spu;
+	if (!spu)
+		return;
+	spu_switch_notify(spu, ctx);
+}
+
 /* The three externally callable interfaces
  * for the scheduler begin here.
  *
arch/powerpc/platforms/cell/spufs/spufs.h +1 -0
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================
@@ -183,6 +183,7 @@
 void spu_yield(struct spu_context *ctx);
 int __init spu_sched_init(void);
 void __exit spu_sched_exit(void);
+void spu_notify_already_active(struct spu_context *ctx);
 
 extern char *isolated_loader;
arch/powerpc/platforms/cell/spufs/run.c +10 -1
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/run.c
===================================================================
@@ -10,6 +10,8 @@
 
 #include "spufs.h"
 
+extern int notify_active[MAX_NUMNODES];
+
 /* interrupt-level stop callback function. */
 void spufs_stop_callback(struct spu *spu)
 {

@@ -45,7 +47,9 @@
 	u64 pte_fault;
 
 	*stat = ctx->ops->status_read(ctx);
-	if (ctx->state != SPU_STATE_RUNNABLE)
+	smp_mb();
+
+	if (ctx->state != SPU_STATE_RUNNABLE || notify_active[ctx->spu->number])
 		return 1;
 	spu = ctx->spu;
 	pte_fault = spu->dsisr &

@@ -319,6 +323,11 @@
 		ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));
 		if (unlikely(ret))
 			break;
+		if (unlikely(notify_active[ctx->spu->number])) {
+			notify_active[ctx->spu->number] = 0;
+			if (!(status & SPU_STATUS_STOPPED_BY_STOP))
+				spu_notify_already_active(ctx);
+		}
 		if ((status & SPU_STATUS_STOPPED_BY_STOP) &&
 		    (status >> SPU_STOP_STATUS_SHIFT == 0x2104)) {
 			ret = spu_process_callback(ctx);
#1 lore raw
Quoted 7 lines

Subject: Enable SPU switch notification to detect currently active SPU tasks.

From: Maynard Johnson <[email protected]>

This patch adds to the capability of spu_switch_event_register so that the caller is also notified of currently active SPU tasks. It also exports spu_switch_event_register and spu_switch_event_unregister.

Hi Maynard,

It'd be really good if you could convince your mailer to send patches inline :)

arch/powerpc/platforms/cell/spufs/sched.c line 49

Quoted 9 lines

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================

arch/powerpc/platforms/cell/spufs/sched.c +1 -0
@@ -46,6 +46,8 @@
 
 #define SPU_MIN_TIMESLICE 	(100 * HZ / 1000)
 
+int notify_active[MAX_NUMNODES];

You're basing the size of the array on MAX_NUMNODES (1 << CONFIG_NODES_SHIFT), but then indexing it by spu->number.

It's quite possible we'll have a system with MAX_NUMNODES == 1, but > 1 spus, in which case this code is going to break. The PS3 is one such system.

Instead I think you should have a flag in the spu struct.

arch/powerpc/platforms/cell/spufs/sched.c line 87

Quoted 10 lines
arch/powerpc/platforms/cell/spufs/sched.c +2 -1
 #define SPU_BITMAP_SIZE (((MAX_PRIO+BITS_PER_LONG)/BITS_PER_LONG)+1)
 struct spu_prio_array {
 	unsigned long bitmap[SPU_BITMAP_SIZE];

@@ -81,18 +83,45 @@
 static void spu_switch_notify(struct spu *spu, struct spu_context *ctx)
 {
 	blocking_notifier_call_chain(&spu_switch_notifier,
-			    ctx ? ctx->object_id : 0, spu);
+				     ctx ? ctx->object_id : 0, spu);
+}

Try not to make whitespace only changes in the same patch as actual code changes.

Quoted 7 lines
arch/powerpc/platforms/cell/spufs/sched.c +7 -0
+
+static void notify_spus_active(void)
+{
+	int node;
+	/* Wake up the active spu_contexts. When the awakened processes 
+	 * sees their notify_active flag is set, they will call
+	 * spu_notify_already_active().

arch/powerpc/platforms/cell/spufs/sched.c line 104

Quoted 10 lines
arch/powerpc/platforms/cell/spufs/sched.c +10 -0
+	 */
+	for (node = 0; node < MAX_NUMNODES; 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;
+			wake_up_all(&ctx->stop_wq);
+			notify_active[ctx->spu->number] = 1;
+			smp_mb();
+		}

I don't understand why you're setting the notify flag after you do the wake_up_all() ?

You only need a smp_wmb() here.

Does the scheduler guarantee that ctxs won't swap nodes? Otherwise between releasing the lock on one node and getting the lock on the next, a ctx could migrate between them - which would cause either spurious wake ups, or missing a ctx altogether. Although I'm not sure if it's that important.

Quoted 65 lines
arch/powerpc/platforms/cell/spufs/sched.c +18 -1
+                mutex_unlock(&spu_prio->active_mutex[node]);
+	}
+	yield();
 }
 
 int spu_switch_event_register(struct notifier_block * n)
 {
-	return blocking_notifier_chain_register(&spu_switch_notifier, n);
+	int ret;
+	ret = blocking_notifier_chain_register(&spu_switch_notifier, n);
+	if (!ret)
+		notify_spus_active();
+	return ret;
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_register);
 
 int spu_switch_event_unregister(struct notifier_block * n)
 {
 	return blocking_notifier_chain_unregister(&spu_switch_notifier, n);
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_unregister);
 
 
 static inline void bind_context(struct spu *spu, struct spu_context *ctx)

@@ -250,6 +279,14 @@
 	return spu_get_idle(ctx, flags);
 }
 
+void spu_notify_already_active(struct spu_context *ctx)
+{
+	struct spu *spu = ctx->spu;
+	if (!spu)
+		return;
+	spu_switch_notify(spu, ctx);
+}
+
 /* The three externally callable interfaces
  * for the scheduler begin here.
  *

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================

arch/powerpc/platforms/cell/spufs/spufs.h +1 -0
@@ -183,6 +183,7 @@
 void spu_yield(struct spu_context *ctx);
 int __init spu_sched_init(void);
 void __exit spu_sched_exit(void);
+void spu_notify_already_active(struct spu_context *ctx);
 
 extern char *isolated_loader;

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/run.c
===================================================================

arch/powerpc/platforms/cell/spufs/run.c +2 -0
@@ -10,6 +10,8 @@
 
 #include "spufs.h"
 
+extern int notify_active[MAX_NUMNODES];
+
 /* interrupt-level stop callback function. */
 void spufs_stop_callback(struct spu *spu)
 {

arch/powerpc/platforms/cell/spufs/run.c line 50

Quoted 5 lines
arch/powerpc/platforms/cell/spufs/run.c +1 -1
@@ -45,7 +47,9 @@
 	u64 pte_fault;
 
 	*stat = ctx->ops->status_read(ctx);
-	if (ctx->state != SPU_STATE_RUNNABLE)
+	smp_mb();

And smp_rmb() should be sufficient here.

Quoted 6 lines
arch/powerpc/platforms/cell/spufs/run.c +1 -0
+	if (ctx->state != SPU_STATE_RUNNABLE || notify_active[ctx->spu->number])
 		return 1;
 	spu = ctx->spu;
 	pte_fault = spu->dsisr &

@@ -319,6 +323,11 @@
 		ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));

arch/powerpc/platforms/cell/spufs/run.c line 333

Quoted 10 lines
arch/powerpc/platforms/cell/spufs/run.c +5 -0
 		if (unlikely(ret))
 			break;
+		if (unlikely(notify_active[ctx->spu->number])) {
+			notify_active[ctx->spu->number] = 0;
+			if (!(status & SPU_STATUS_STOPPED_BY_STOP))
+				spu_notify_already_active(ctx);
+		}
 		if ((status & SPU_STATUS_STOPPED_BY_STOP) &&
 		    (status >> SPU_STOP_STATUS_SHIFT == 0x2104)) {
 			ret = spu_process_callback(ctx);

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
author
#2 lore raw

Michael,
Thanks for your comments! My responses are below.

-Maynard

Quoted 4 lines · Michael Ellerman
Quoted 4 lines

Subject: Enable SPU switch notification to detect currently active SPU tasks.

From: Maynard Johnson <[email protected]>

Quoted 8 lines
Quoted 3 lines

This patch adds to the capability of spu_switch_event_register so that the caller is also notified of currently active SPU tasks. It also exports spu_switch_event_register and spu_switch_event_unregister.

Hi Maynard,

It'd be really good if you could convince your mailer to send patches inline :)

Mozilla Mail is my client, and I don't see any option to force patches inline. Of course, there is an option to display received attachments as inline, but that doesn't help you.

Quoted 11 lines

arch/powerpc/platforms/cell/spufs/sched.c line 49

Quoted 11 lines

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================

arch/powerpc/platforms/cell/spufs/sched.c +1 -0
@@ -46,6 +46,8 @@
 
 #define SPU_MIN_TIMESLICE 	(100 * HZ / 1000)
 
+int notify_active[MAX_NUMNODES];

DOH! Right, this is patently wrong. Somehow, I misunderstood what MAX_NUMNODES was. I'll fix it.

Quoted 10 lines

You're basing the size of the array on MAX_NUMNODES (1 << CONFIG_NODES_SHIFT), but then indexing it by spu->number.

It's quite possible we'll have a system with MAX_NUMNODES == 1, but > 1 spus, in which case this code is going to break. The PS3 is one such system.

Instead I think you should have a flag in the spu struct.

Yes, that's certainly an option I'll look at.

Quoted 30 lines

arch/powerpc/platforms/cell/spufs/sched.c line 87

Quoted 12 lines
arch/powerpc/platforms/cell/spufs/sched.c +2 -1
 #define SPU_BITMAP_SIZE (((MAX_PRIO+BITS_PER_LONG)/BITS_PER_LONG)+1)
 struct spu_prio_array {
 	unsigned long bitmap[SPU_BITMAP_SIZE];

@@ -81,18 +83,45 @@
 static void spu_switch_notify(struct spu *spu, struct spu_context *ctx)
 {
 	blocking_notifier_call_chain(&spu_switch_notifier,
-			    ctx ? ctx->object_id : 0, spu);
+				     ctx ? ctx->object_id : 0, spu);
+}

Try not to make whitespace only changes in the same patch as actual code changes.

Quoted 13 lines
arch/powerpc/platforms/cell/spufs/sched.c +13 -0
+
+static void notify_spus_active(void)
+{
+	int node;
+	/* Wake up the active spu_contexts. When the awakened processes 
+	 * sees their notify_active flag is set, they will call
+	 * spu_notify_already_active().
+	 */
+	for (node = 0; node < MAX_NUMNODES; 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;
Quoted 8 lines

arch/powerpc/platforms/cell/spufs/sched.c line 104

Quoted 4 lines
arch/powerpc/platforms/cell/spufs/sched.c +4 -0
+			wake_up_all(&ctx->stop_wq);
+			notify_active[ctx->spu->number] = 1;
+			smp_mb();
+		}

I don't understand why you're setting the notify flag after you do the wake_up_all() ?

Right, I'll move it.

Quoted 2 lines

You only need a smp_wmb() here.

OK.

Quoted 2 lines

Does the scheduler guarantee that ctxs won't swap nodes? Otherwise

Don't know. Arnd would probably know off the top of his head.

Quoted 4 lines

between releasing the lock on one node and getting the lock on the next, a ctx could migrate between them - which would cause either spurious wake ups, or missing a ctx altogether. Although I'm not sure if it's that important.

How important is it? If the SPUs were executing different code or taking different paths through the same code, then seeing the samples for each SPU is important. But if this were the case, the user would easily see the misssing data for the missing SPU(s). Since this sounds like a very small window, re-running the profile would _probably_ yield a complete profile. On the other hand, if we can avoid it, we should. When I repost the fixed-up patch, I'll add a comment/question for Arnd about this issue.

Quoted 69 lines
Quoted 69 lines
arch/powerpc/platforms/cell/spufs/sched.c +18 -1
+                mutex_unlock(&spu_prio->active_mutex[node]);
+	}
+	yield();
 }
 
 int spu_switch_event_register(struct notifier_block * n)
 {
-	return blocking_notifier_chain_register(&spu_switch_notifier, n);
+	int ret;
+	ret = blocking_notifier_chain_register(&spu_switch_notifier, n);
+	if (!ret)
+		notify_spus_active();
+	return ret;
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_register);
 
 int spu_switch_event_unregister(struct notifier_block * n)
 {
 	return blocking_notifier_chain_unregister(&spu_switch_notifier, n);
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_unregister);
 
 
 static inline void bind_context(struct spu *spu, struct spu_context *ctx)

@@ -250,6 +279,14 @@
 	return spu_get_idle(ctx, flags);
 }
 
+void spu_notify_already_active(struct spu_context *ctx)
+{
+	struct spu *spu = ctx->spu;
+	if (!spu)
+		return;
+	spu_switch_notify(spu, ctx);
+}
+
 /* The three externally callable interfaces
 * for the scheduler begin here.
 *

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================

arch/powerpc/platforms/cell/spufs/spufs.h +1 -0
@@ -183,6 +183,7 @@
 void spu_yield(struct spu_context *ctx);
 int __init spu_sched_init(void);
 void __exit spu_sched_exit(void);
+void spu_notify_already_active(struct spu_context *ctx);
 
 extern char *isolated_loader;

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/run.c
===================================================================

arch/powerpc/platforms/cell/spufs/run.c +2 -0
@@ -10,6 +10,8 @@
 
 #include "spufs.h"
 
+extern int notify_active[MAX_NUMNODES];
+
 /* interrupt-level stop callback function. */
 void spufs_stop_callback(struct spu *spu)
 {

@@ -45,7 +47,9 @@
 	u64 pte_fault;
Quoted 6 lines

arch/powerpc/platforms/cell/spufs/run.c line 50

Quoted 3 lines
arch/powerpc/platforms/cell/spufs/run.c +1 -1
 	*stat = ctx->ops->status_read(ctx);
-	if (ctx->state != SPU_STATE_RUNNABLE)
+	smp_mb();

And smp_rmb() should be sufficient here.

OK

Quoted 22 lines
Quoted 8 lines
arch/powerpc/platforms/cell/spufs/run.c +1 -0
+	if (ctx->state != SPU_STATE_RUNNABLE || notify_active[ctx->spu->number])
 		return 1;
 	spu = ctx->spu;
 	pte_fault = spu->dsisr &

@@ -319,6 +323,11 @@
 		ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));

arch/powerpc/platforms/cell/spufs/run.c line 333

Quoted 10 lines
arch/powerpc/platforms/cell/spufs/run.c +5 -0
 		if (unlikely(ret))
 			break;
+		if (unlikely(notify_active[ctx->spu->number])) {
+			notify_active[ctx->spu->number] = 0;
+			if (!(status & SPU_STATUS_STOPPED_BY_STOP))
+				spu_notify_already_active(ctx);
+		}
 		if ((status & SPU_STATUS_STOPPED_BY_STOP) &&
 		    (status >> SPU_STOP_STATUS_SHIFT == 0x2104)) {
 			ret = spu_process_callback(ctx);

cheers

author
#3 lore raw

Re: [PATCH] Cell SPU task notification -- updated patch: #1

In reply to Michael Ellerman

Attached is an updated patch that addresses Michael Ellerman's comments.

One comment made by Michael has not yet been addressed:
The comment was in regard to the for-loop in spufs/sched.c:notify_spus_active(). He wondered if the scheduler can swap a context from one node to another. If so, there's a small window in this loop (where we switch the lock from one node's active list to the next) where it may be possible we might miss waking up a context and send a spurious wakeup to another.
Arnd . . . can you comment on this question?

Thanks.
-Maynard

Subject: Enable SPU switch notification to detect currently active SPU tasks.

From: Maynard Johnson <[email protected]>

This patch adds to the capability of spu_switch_event_register so that the caller is also notified of currently active SPU tasks. It also exports spu_switch_event_register and spu_switch_event_unregister.

Signed-off-by Maynard Johnson <[email protected]>
arch/powerpc/platforms/cell/spufs/sched.c +36 -1
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================
@@ -84,15 +84,42 @@
 			    ctx ? ctx->object_id : 0, spu);
 }
 
+static void notify_spus_active(void)
+{
+       int node;
+	/* Wake up the active spu_contexts. When the awakened processes 
+	 * sees their notify_active flag is set, they will call
+	 * spu_notify_already_active().
+	 */
+	for (node = 0; node < MAX_NUMNODES; 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;
+			spu->notify_active = 1;
+			wake_up_all(&ctx->stop_wq);
+			smp_wmb();
+		}
+                mutex_unlock(&spu_prio->active_mutex[node]);
+	}
+	yield();
+}
+
 int spu_switch_event_register(struct notifier_block * n)
 {
-	return blocking_notifier_chain_register(&spu_switch_notifier, n);
+	int ret;
+	ret = blocking_notifier_chain_register(&spu_switch_notifier, n);
+	if (!ret)
+		notify_spus_active();
+	return ret;
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_register);
 
 int spu_switch_event_unregister(struct notifier_block * n)
 {
 	return blocking_notifier_chain_unregister(&spu_switch_notifier, n);
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_unregister);
 
 
 static inline void bind_context(struct spu *spu, struct spu_context *ctx)

@@ -250,6 +277,14 @@
 	return spu_get_idle(ctx, flags);
 }
 
+void spu_notify_already_active(struct spu_context *ctx)
+{
+	struct spu *spu = ctx->spu;
+	if (!spu)
+		return;
+	spu_switch_notify(spu, ctx);
+}
+
 /* The three externally callable interfaces
  * for the scheduler begin here.
  *
arch/powerpc/platforms/cell/spufs/spufs.h +1 -0
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================
@@ -183,6 +183,7 @@
 void spu_yield(struct spu_context *ctx);
 int __init spu_sched_init(void);
 void __exit spu_sched_exit(void);
+void spu_notify_already_active(struct spu_context *ctx);
 
 extern char *isolated_loader;
arch/powerpc/platforms/cell/spufs/run.c +13 -2
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/run.c
===================================================================
@@ -45,9 +45,11 @@
 	u64 pte_fault;
 
 	*stat = ctx->ops->status_read(ctx);
-	if (ctx->state != SPU_STATE_RUNNABLE)
-		return 1;
+	smp_rmb();
+
 	spu = ctx->spu;
+	if (ctx->state != SPU_STATE_RUNNABLE || spu->notify_active)
+		return 1;
 	pte_fault = spu->dsisr &
 	    (MFC_DSISR_PTE_NOT_FOUND | MFC_DSISR_ACCESS_DENIED);
 	return (!(*stat & 0x1) || pte_fault || spu->class_0_pending) ? 1 : 0;

@@ -304,6 +306,7 @@
 		   u32 *npc, u32 *event)
 {
 	int ret;
+	struct * spu;
 	u32 status;
 
 	if (down_interruptible(&ctx->run_sema))

@@ -317,8 +320,16 @@
 
 	do {
 		ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));
+		spu = ctx->spu;
 		if (unlikely(ret))
 			break;
+		if (unlikely(spu->notify_active)) {
+			spu->notify_active = 0;
+			if (!(status & SPU_STATUS_STOPPED_BY_STOP)) {
+				spu_notify_already_active(ctx);
+				continue;
+			}
+		}
 		if ((status & SPU_STATUS_STOPPED_BY_STOP) &&
 		    (status >> SPU_STOP_STATUS_SHIFT == 0x2104)) {
 			ret = spu_process_callback(ctx);
#4 lore raw
In reply to Maynard Johnson
arch/powerpc/platforms/cell/spufs/sched.c +11 -0
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================
@@ -84,15 +84,42 @@
 			    ctx ? ctx->object_id : 0, spu);
 }
 
+static void notify_spus_active(void)
+{
+       int node;
+	/* Wake up the active spu_contexts. When the awakened processes 
+	 * sees their notify_active flag is set, they will call
+	 * spu_notify_already_active().
+	 */
+	for (node = 0; node < MAX_NUMNODES; node++) {
+		struct spu *spu;
+		mutex_lock(&spu_prio->active_mutex[node]);
+                list_for_each_entry(spu, &spu_prio->active_list[node], list) {

You seem to have some issues with tabs vs spaces for indentation here.

+ struct spu_context *ctx = spu->ctx;
+ spu->notify_active = 1;

Please make this a bit in the sched_flags field that's added in the scheduler patch series I sent out.

+ wake_up_all(&ctx->stop_wq);
+ smp_wmb();
+ }
+ mutex_unlock(&spu_prio->active_mutex[node]);
+ }
+ yield();
+}

Why do you add the yield() here? yield is pretty much a sign for a bug

+void spu_notify_already_active(struct spu_context *ctx) +{
+ struct spu *spu = ctx->spu;
+ if (!spu)
+ return;
+ spu_switch_notify(spu, ctx);
+}

Please just call spu_switch_notify directly from the only caller. Also the check for ctx->spu beeing there is not required if you look a the caller.

*stat = ctx->ops->status_read(ctx);
- if (ctx->state != SPU_STATE_RUNNABLE)
- return 1;
+ smp_rmb();

What do you need the barrier for here?

author
#5 lore raw
Quoted 14 lines · Christoph Hellwig

Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================

arch/powerpc/platforms/cell/spufs/sched.c +6 -0
@@ -84,15 +84,42 @@
 			    ctx ? ctx->object_id : 0, spu);
 }
 
+static void notify_spus_active(void)
+{
+       int node;
+	/* Wake up the active spu_contexts. When the awakened processes
+	 * sees their notify_active flag is set, they will call
+	 * spu_notify_already_active().

arch/powerpc/platforms/cell/spufs/sched.c line 102

Quoted 10 lines
arch/powerpc/platforms/cell/spufs/sched.c +5 -0
+	 */
+	for (node = 0; node < MAX_NUMNODES; node++) {
+		struct spu *spu;
+		mutex_lock(&spu_prio->active_mutex[node]);
+                list_for_each_entry(spu, &spu_prio->active_list[node], list) {
 
 	You seem to have some issues with tabs vs spaces for indentation
 	here.

fixed

arch/powerpc/platforms/cell/spufs/sched.c line 110

Quoted 8 lines
arch/powerpc/platforms/cell/spufs/sched.c +2 -0
+			struct spu_context *ctx = spu->ctx;
+			spu->notify_active = 1;
 
 
 	Please make this a bit in the sched_flags field that's added in
 	the scheduler patch series I sent out.

I haven't seen that the scheduler patch series got applied yet. This Cell spu task notification patch is a pre-req for OProfile development to support profiling SPUs. When the scheduler patch gets applied to a kernel version that fits our needs for our OProfile development, I don't see any problem in using the sched_flags field instead of notify_active.

Quoted 8 lines
arch/powerpc/platforms/cell/spufs/sched.c +7 -0
+			wake_up_all(&ctx->stop_wq);
+			smp_wmb();
+		}
+                mutex_unlock(&spu_prio->active_mutex[node]);
+	}
+	yield();
+}
Quoted 4 lines

Why do you add the yield() here? yield is pretty much a sign for a bug

Yes, the yield() and the memory barriers were leftovers from an earlier ill-conceived attempt at solving this problem. They should have been removed. They're gone now.

Quoted 11 lines
arch/powerpc/platforms/cell/spufs/sched.c +7 -0
+void spu_notify_already_active(struct spu_context *ctx)
+{
+	struct spu *spu = ctx->spu;
+	if (!spu)
+		return;
+	spu_switch_notify(spu, ctx);
+}

Please just call spu_switch_notify directly from the only

I hesitated doing this since it would entail changing spu_switch_notify from being static to non-static. I'd like to get Arnd's opinion on this question before going ahead and making such a change.

Quoted 11 lines
	caller.  Also the check for ctx->spu beeing there is not
	required if you look a the caller.


	*stat = ctx->ops->status_read(ctx);
arch/powerpc/platforms/cell/spufs/sched.c +1 -2
-	if (ctx->state != SPU_STATE_RUNNABLE)
-		return 1;
+	smp_rmb();
Quoted 3 lines

What do you need the barrier for here?

Removed.

#6 lore raw
Quoted 5 lines · Maynard Johnson

I haven't seen that the scheduler patch series got applied yet. This Cell spu task notification patch is a pre-req for OProfile development
to support profiling SPUs. When the scheduler patch gets applied to a
kernel version that fits our needs for our OProfile development, I don't see any problem in using the sched_flags field instead of notify_active.

I'll hopefull commit these patches this weekend, I'm at a conference currently so not really able to do a lot of work. If you need to make more progress until than just apply the hunk that introduces sched_flags before doing your patch.

Quoted 3 lines

Yes, the yield() and the memory barriers were leftovers from an earlier ill-conceived attempt at solving this problem. They should have been removed. They're gone now.

Ok.

Quoted 3 lines

I hesitated doing this since it would entail changing spu_switch_notify from being static to non-static. I'd like to get Arnd's opinion on this question before going ahead and making such a change.

There is no difference in impact between marking a function non-static and adding a trivial wrapper around it, only that the latter creates more bloat. So I don't think there's a good argument against this.

author
#7 lore raw

Re: [Cbe-oss-dev] [PATCH] Cell SPU task notification - repost of patch with updates

In reply to Maynard Johnson

This patch makes all the changes suggested by Christopher, with the exception of the suggestion to use the sched_flags field versus a new member to the spu_context struct to signal the need to "notify already active". I don't have the sched_flags change in my 2.6.20-rc1 tree. I can send another patch later if/when the sched_flags changes appears in the kernel version we end up picking for final oprofile-spu development.

Comments welcome. Thanks.

-Maynard

Subject: Enable SPU switch notification to detect currently active SPU tasks.

From: Maynard Johnson <[email protected]>

This patch adds to the capability of spu_switch_event_register so that the caller is also notified of currently active SPU tasks. It also exports spu_switch_event_register and spu_switch_event_unregister.

Signed-off-by Maynard Johnson <[email protected]>
arch/powerpc/platforms/cell/spufs/sched.c +27 -2
Index: linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/sched.c
===================================================================
@@ -78,21 +78,46 @@
 
 static BLOCKING_NOTIFIER_HEAD(spu_switch_notifier);
 
-static void spu_switch_notify(struct spu *spu, struct spu_context *ctx)
+void spu_switch_notify(struct spu *spu, struct spu_context *ctx)
 {
 	blocking_notifier_call_chain(&spu_switch_notifier,
 			    ctx ? ctx->object_id : 0, spu);
 }
 
+static void notify_spus_active(void)
+{
+       int node;
+	/* Wake up the active spu_contexts. When the awakened processes 
+	 * sees their notify_active flag is set, they will call
+	 * spu_switch_notify();
+	 */
+	for (node = 0; node < MAX_NUMNODES; 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;
+			spu->notify_active = 1;
+			wake_up_all(&ctx->stop_wq);
+		}
+                mutex_unlock(&spu_prio->active_mutex[node]);
+	}
+}
+
 int spu_switch_event_register(struct notifier_block * n)
 {
-	return blocking_notifier_chain_register(&spu_switch_notifier, n);
+	int ret;
+	ret = blocking_notifier_chain_register(&spu_switch_notifier, n);
+	if (!ret)
+		notify_spus_active();
+	return ret;
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_register);
 
 int spu_switch_event_unregister(struct notifier_block * n)
 {
 	return blocking_notifier_chain_unregister(&spu_switch_notifier, n);
 }
+EXPORT_SYMBOL_GPL(spu_switch_event_unregister);
 
 
 static inline void bind_context(struct spu *spu, struct spu_context *ctx)
arch/powerpc/platforms/cell/spufs/spufs.h +1 -0
Index: linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/spufs.h
===================================================================
@@ -180,6 +180,7 @@
 int spu_activate(struct spu_context *ctx, u64 flags);
 void spu_deactivate(struct spu_context *ctx);
 void spu_yield(struct spu_context *ctx);
+void spu_switch_notify(struct spu *spu, struct spu_context *ctx);
 int __init spu_sched_init(void);
 void __exit spu_sched_exit(void);
arch/powerpc/platforms/cell/spufs/run.c +12 -2
Index: linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/run.c
===================================================================
@@ -45,9 +45,10 @@
 	u64 pte_fault;
 
 	*stat = ctx->ops->status_read(ctx);
-	if (ctx->state != SPU_STATE_RUNNABLE)
-		return 1;
+
 	spu = ctx->spu;
+	if (ctx->state != SPU_STATE_RUNNABLE || spu->notify_active)
+		return 1;
 	pte_fault = spu->dsisr &
 	    (MFC_DSISR_PTE_NOT_FOUND | MFC_DSISR_ACCESS_DENIED);
 	return (!(*stat & 0x1) || pte_fault || spu->class_0_pending) ? 1 : 0;

@@ -305,6 +306,7 @@
 		   u32 *npc, u32 *event)
 {
 	int ret;
+	struct spu * spu;
 	u32 status;
 
 	if (down_interruptible(&ctx->run_sema))

@@ -318,8 +320,16 @@
 
 	do {
 		ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));
+		spu = ctx->spu;
 		if (unlikely(ret))
 			break;
+		if (unlikely(spu->notify_active)) {
+			spu->notify_active = 0;
+			if (!(status & SPU_STATUS_STOPPED_BY_STOP)) {
+				spu_switch_notify(spu, ctx);
+				continue;
+			}
+		}
 		if ((status & SPU_STATUS_STOPPED_BY_STOP) &&
 		    (status >> SPU_STOP_STATUS_SHIFT == 0x2104)) {
 			ret = spu_process_callback(ctx);
include/asm-powerpc/spu.h +1 -0
Index: linux-2.6.20-rc1/include/asm-powerpc/spu.h
===================================================================
@@ -144,6 +144,7 @@
 
 	void* pdata; /* platform private data */
 	struct sys_device sysdev;
+	int notify_active;
 };
 
 struct spu *spu_alloc(void);