Re: [Cbe-oss-dev] [PATCH] Cell SPU task notification - repost of patch with updates
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]>
Index: linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/sched.c
===
--- linux-2.6.20-rc1.orig/arch/powerpc/platforms/cell/spufs/sched.c 2007-01-18 16:43:14.324526032 -0600
+++ linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/sched.c 2007-01-26 16:16:35.219668640 -0600
@@ -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)
Index: linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/spufs.h
===
--- linux-2.6.20-rc1.orig/arch/powerpc/platforms/cell/spufs/spufs.h 2007-01-18 16:43:14.340523600 -0600
+++ linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/spufs.h 2007-01-26 16:26:49.733703448 -0600
@@ -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);
Index: linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/run.c
===
--- linux-2.6.20-rc1.orig/arch/powerpc/platforms/cell/spufs/run.c 2007-01-18 16:43:14.340523600 -0600
+++ linux-2.6.20-rc1/arch/powerpc/platforms/cell/spufs/run.c 2007-01-26 16:24:38.979744856 -0600
@@ -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);
Index: linux-2.6.20-rc1/include/asm-powerpc/spu.h
===
--- linux-2.6.20-rc1.orig/include/asm-powerpc/spu.h 2007-01-18 16:43:19.932605072 -0600
+++ linux-2.6.20-rc1/include/asm-powerpc/spu.h 2007-01-24 12:17:30.209676992 -0600
@@ -144,6 +144,7 @@
void* pdata; /* platform private data */
struct sys_device sysdev;
Re: [Cbe-oss-dev] [PATCH] Cell SPU task notification
On Wed, Jan 17, 2007 at 09:56:12AM -0600, Maynard Johnson wrote: > 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. > 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. > 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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [Cbe-oss-dev] [PATCH] Cell SPU task notification
Christoph Hellwig wrote:
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===
---
linux-2.6.19-rc6-arnd1+patches.orig/arch/powerpc/platforms/cell/spufs/sched.c
2006-12-04 10:56:04.730698720 -0600
+++ linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
2007-01-15 16:22:31.808461448 -0600
@@ -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.
fixed
+ 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.
+ 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
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.
+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.
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?
Removed.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [Cbe-oss-dev] [PATCH] Cell SPU task notification
Index: linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
===
---
linux-2.6.19-rc6-arnd1+patches.orig/arch/powerpc/platforms/cell/spufs/sched.c
2006-12-04 10:56:04.730698720 -0600
+++ linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c
2007-01-15 16:22:31.808461448 -0600
@@ -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?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
