| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <1168826871.4622.32.camel@concordia.ozlabs.ibm.com>
Date: Mon, 15 Jan 2007 13:07:51 +1100
From: Michael Ellerman <michael@...erman.id.au>
To: Maynard Johnson <maynardj@...ibm.com>
Cc: cbe-oss-dev@...abs.org, linux-kernel@...r.kernel.org,
linuxppc-dev@...abs.org, Arnd Bergmann <arnd.bergmann@...ibm.com>
Subject: Re: [PATCH] Cell SPU task notification
> Subject: Enable SPU switch notification to detect currently active SPU tasks.
>
> From: Maynard Johnson <maynardj@...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
Download attachment "signature.asc" of type "application/pgp-signature" (190 bytes)
Powered by blists - more mailing lists