Date:	Wed, 06 Dec 2006 16:04:53 -0600
From:	Maynard Johnson <maynardj@...ibm.com>
To:	Luke Browning <lukeb@...ibm.com>,
	Arnd Bergmann <arnd.bergmann@...ibm.com>
CC:	Luke Browning <lukebrowning@...ibm.com>,
	maynardj@...ux.vnet.ibm.com,
	linuxppc-dev-bounces+lukebrowning=us.ibm.com@...abs.org,
	linux-kernel@...r.kernel.org, linuxppc-dev@...abs.org,
	oprofile-list@...ts.sourceforge.net, cbe-oss-dev@...abs.org
Subject: Re: [PATCH]Add notification for active Cell SPU tasks

Luke Browning wrote:

> linuxppc-dev-bounces+lukebrowning=us.ibm.com@...abs.org wrote on 
> 12/04/2006 10:26:57:
>
> > linuxppc-dev-bounces+lukebrowning=us.ibm.com@...abs.org wrote on
> > 01/12/2006 06:01:15 PM:
> >
> > >
> > > Subject: Enable SPU switch notification to detect currently 
> activeSPU tasks.
> > >
> > > From: Maynard Johnson <maynardj@...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@...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@...abs.org
>https://ozlabs.org/mailman/listinfo/linuxppc-dev
>


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists