[PATCH]Add notification for active Cell SPU tasks

[PATCH]Add notification for active Cell SPU tasks

[Maynard Johnson]

[-- Attachment #1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #2: spu-notifier.patch --]
[-- Type: text/x-diff, Size: 1828 bytes --]

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 to notify the
caller of currently active SPU tasks.  It also exports spu_switch_event_register
and spu_switch_event_unregister.

Signed-off-by: Maynard Johnson <mpjohn@us.ibm.com>


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-11-24 11:34:44.884455680 -0600
+++ linux-2.6.19-rc6-arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c	2006-12-01 13:57:21.864583264 -0600
@@ -84,15 +84,37 @@
 			    ctx ? ctx->object_id : 0, spu);
 }
 
+static void notify_spus_active(void)
+{
+	int node;
+	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;
+	 			 blocking_notifier_call_chain(&spu_switch_notifier,
+	 		 			 ctx ? ctx->object_id : 0, spu);
+		}
+		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)

Re: [Cbe-oss-dev] [PATCH]Add notification for active Cell SPU tasks

[Arnd Bergmann]

On Friday 01 December 2006 21:01, Maynard Johnson wrote:
> +static void notify_spus_active(void)
> +{
> +       int node;
> +       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;
> +                                blocking_notifier_call_chain(&spu_switch_notifier,
> +                                                ctx ? ctx->object_id : 0, spu);
> +               }
> +               mutex_unlock(&spu_prio->active_mutex[node]);
> +        }

I wonder if this is enough for oprofile. Don't you need to access user
space data of the task running on the SPU? I always assumed you want
to do it via get_user or copy_from_user, which obviously doesn't work
here, when you're running in the oprofile task. Are you using something
like access_process_vm now?

	Arnd <><

Re: [Cbe-oss-dev] [PATCH]Add notification for active Cell SPU tasks

[Maynard Johnson]

Arnd Bergmann wrote:
> On Friday 01 December 2006 21:01, Maynard Johnson wrote:
> 
>>+static void notify_spus_active(void)
>>+{
>>+       int node;
>>+       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;
>>+                                blocking_notifier_call_chain(&spu_switch_notifier,
>>+                                                ctx ? ctx->object_id : 0, spu);
>>+               }
>>+               mutex_unlock(&spu_prio->active_mutex[node]);
>>+        }
> 
> 
> I wonder if this is enough for oprofile. Don't you need to access user
> space data of the task running on the SPU? I always assumed you want
No, I don't need anything from the user app besides access to the 
executable binary, which I get with copy_from_user, specifying the 
'objectid' as from address.
> to do it via get_user or copy_from_user, which obviously doesn't work
> here, when you're running in the oprofile task. Are you using something
> like access_process_vm now?
> 
> 	Arnd <><

Re: [PATCH]Add notification for active Cell SPU tasks

[Maynard Johnson]

Luke Browning wrote:

> linuxppc-dev-bounces+lukebrowning=us.ibm.com@ozlabs.org wrote on 
> 12/04/2006 10:26:57:
>
> > linuxppc-dev-bounces+lukebrowning=us.ibm.com@ozlabs.org wrote on
> > 01/12/2006 06:01:15 PM:
> >
> > >
> > > Subject: Enable SPU switch notification to detect currently 
> activeSPU tasks.
> > >
> > > From: Maynard Johnson <maynardj@us.ibm.com>
> > >
> > > This patch adds to the capability of spu_switch_event_register to 
> notify the
> > > caller of currently active SPU tasks.  It also exports
> > > spu_switch_event_register
> > > and spu_switch_event_unregister.
> > >
> > > Signed-off-by: Maynard Johnson <mpjohn@us.ibm.com>
> > >
> > >
> > > 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-11-24 11:34:
> > > 44.884455680 -0600
> > > +++ linux-2.6.19-rc6-
> > > arnd1+patches/arch/powerpc/platforms/cell/spufs/sched.c   2006-12-01
> > > 13:57:21.864583264 -0600
> > > @@ -84,15 +84,37 @@
> > >               ctx ? ctx->object_id : 0, spu);
> > >  }
> > >  
> > > +static void notify_spus_active(void)
> > > +{
> > > +   int node;
> > > +   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;
> > > +              blocking_notifier_call_chain(&spu_switch_notifier,
> > > +                     ctx ? ctx->object_id : 0, spu);
> > > +      }
> > > +      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)
> >
> > Is this really the right strategy?  First, it serializes all spu 
> context
> > switching at the node level.  Second, it performs 17 callouts for
>
I could be wrong, but I think if we moved the mutex_lock to be inside of 
the list_for_each_entry loop, we could have a race condition.  For 
example, we obtain the next spu item from the spu_prio->active_mutex 
list, then wait on the mutex which is being held for the purpose of 
removing the very spu context we just obtained.

> > every context
> > switch.  Can't oprofile internally derive the list of active spus 
> from the  
> > context switch callout. 
>
Arnd would certainly know the answer to this off the top of his head, 
but when I initially discussed the idea for this patch with him 
(probably a couple months ago or so), he didn't suggest a better 
alternative.  Perhaps there is a way to do this with current SPUFS 
code.  Arnd, any comments on this?

> >
> > Also, the notify_spus_active() callout is dependent on the return 
> code of
> > spu_switch_notify().  Should notification be hierarchical?  If I
> > only register
> > for the second one, should my notification be dependent on the 
> return code
> > of some non-related subsystem's handler. 
>
I'm not exactly sure what you're saying here.  Are you suggesting that a 
user may only be interested in acitve SPU notification and, therefore, 
shouldn't have to be depenent on the "standard" notification 
registration succeeding?  There may be a case for adding a new 
registration function, I suppose; although, I'm not aware of any other 
users of the SPUFS notification mechanism besides OProfile and PDT, and 
we need notification of both active and future SPU tasks.  But I would 
not object to a new function.

> >
> > Does blocking_callchain_notifier internally check for the presence
> > of registered
> > handlers before it takes locks ...?  We should ensure that there is
> > minimal overhead
> > when there are no registered handlers.
>
I won't pretend to be expert enough to critique the performance of that 
code.

> >
> > Regards,
> > Luke___________________
>
> Any comments to my questions above.  Seems like oprofile / pdt could 
> derive the
> list of active spus from a single context switch callout.  This patch 
> will have
> a large impact on the performance of the system.
>
For OProfile, the registration is only done at the time when a user 
starts the profiler to collect performance data, typically focusing on a 
single application, so I don't see this as an impact on normal 
production operations.  Since you must have root authority to run 
OProfile, it cannot be invoked by just any user for nefarious purposes.

-Maynard

>
> Luke
>
>------------------------------------------------------------------------
>
>_______________________________________________
>Linuxppc-dev mailing list
>Linuxppc-dev@ozlabs.org
>https://ozlabs.org/mailman/listinfo/linuxppc-dev
>