lkml.org 
[lkml]   [2007]   [Jan]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH] Cell SPU task notification
From
Date
> Subject: Enable SPU switch notification to detect currently active SPU tasks.
>
> From: Maynard Johnson <maynardj@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. 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 :)

> 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-11 09:45:37.918333128 -0600
> @@ -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.

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

> +
> +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();
> + }

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.

> + 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
> ===================================================================
> --- linux-2.6.19-rc6-arnd1+patches.orig/arch/powerpc/platforms/cell/spufs/spufs.h 2007-01-08 18:18:40.093354608 -0600
> +++ linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/spufs.h 2007-01-08 18:31:03.610345792 -0600
> @@ -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
> ===================================================================
> --- linux-2.6.19-rc6-arnd1+patches.orig/arch/powerpc/platforms/cell/spufs/run.c 2007-01-08 18:33:51.979311680 -0600
> +++ linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/run.c 2007-01-11 10:17:20.777344984 -0600
> @@ -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();

And smp_rmb() should be sufficient here.

> + 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);

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
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2007-01-15 03:47    [W:0.857 / U:0.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site