Messages in this thread | | | Date | Wed, 17 Jan 2007 09:56:12 -0600 | From | Maynard Johnson <> | Subject | 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 majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |