On December 14, 2006, I posted a patch that added support to the
OProfile kernel driver for profiling Cell SPUs. There have been some
changes/fixes to this patch since the original posting (including
forward porting from 2.6.18-based kernel to 2.6.20-rc1), so I am
reposting the patch for review now. This patch relies upon the
following patches that have not been accepted yet:
1. oprofile cleanup patch (submitted on Nov 27)
2. Fix for PPU profiling (not submitted yet, since it depends on #1)
3. SPU task notification patch (last submitted on Jan 26)
For those who may want to apply and build the oprofile SPU support
patch, it would be necessary to first apply the above patches. For
convenience, I will post all three of the above patches, along with the
oprofile SPU support patch.
Comments appreciated.
Thank you.
Maynard Johnson
IBM LTC Toolchain
On Monday 29 January 2007 20:46, Maynard Johnson wrote:
> This is a clean up patch that includes the following changes:
>
> ? ? ? ? -It removes some macro definitions that are only used once
> ? ? ? ? ?with the actual code.
> ? ? ? ? -Some comments were added to clarify the code based on feedback
> ? ? ? ? ?from the community.
> ? ? ? ? -The write_pm_cntrl() and set_count_mode() were passed a structure
> ? ? ? ? ?element from a global variable. ?The argument was removed so the
> ? ? ? ? ?functions now just operate on the global directly.
> ? ? ? ? -The set_pm_event() function call in the cell_virtual_cntr() routine
> ? ? ? ? ?was moved to a for-loop before the for_each_cpu loop
>
> Signed-off-by: Carl Love <[email protected]>
> Signed-off-by: Maynard Johnson <[email protected]>
>
Acked-by: Arnd Bergmann <[email protected]>
Just a small side note: Please give each of your patches a one-line
summary in the subject of the email. I'm filing this one under:
"cell: oprofile cleanup".
It would also be good if you could use a mailer that sends out
patches as inline, so you don't need to resort to using attachments.
Arnd <><
On Monday 29 January 2007 20:47, Maynard Johnson wrote:
> The code was setting up the debug bus for group 21 when profiling on the
> event PPU CYCLES. ?The debug bus is not actually used by the hardware
> performance counters when counting PPU CYCLES. ?Setting up the debug bus
> for PPU CYCLES causes signal routing conflicts on the debug bus when
> profiling PPU cycles and another PPU event. ?This patch fixes the code to
> only setup the debug bus to route the performance signals for the non
> PPU CYCLE events.
>
> Signed-off-by: Maynard Johnson <[email protected]>
> Signed-off-by: Carl Love <[email protected]>
Acked-by: Arnd Bergmann <[email protected]>
Any suggestion for a one-line patch title?
On Monday 29 January 2007 20:48, Maynard Johnson wrote:
> Subject: Enable SPU switch notification to detect currently active SPU tasks.
>
> From: Maynard Johnson <[email protected]>
>
> 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.
>
> Signed-off-by: Maynard Johnson <[email protected]>
I looked through it again, and think I found a serious bug, but that
should be easy enough to solve:
> +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_switch_notify();
> + */
> + 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;
[side note]
There is a small whitespace breakage in here, please make sure you always
use tabs for indenting, not space characters.
[/side note]
> @@ -45,9 +45,10 @@
> u64 pte_fault;
>
> *stat = ctx->ops->status_read(ctx);
> - if (ctx->state != SPU_STATE_RUNNABLE)
> - return 1;
> +
> spu = ctx->spu;
> + if (ctx->state != SPU_STATE_RUNNABLE || spu->notify_active)
> + return 1;
> pte_fault = spu->dsisr &
> (MFC_DSISR_PTE_NOT_FOUND | MFC_DSISR_ACCESS_DENIED);
> return (!(*stat & 0x1) || pte_fault || spu->class_0_pending) ? 1 : 0;
> @@ -305,6 +306,7 @@
> u32 *npc, u32 *event)
> {
> int ret;
> + struct spu * spu;
> u32 status;
>
> if (down_interruptible(&ctx->run_sema))
> @@ -318,8 +320,16 @@
>
> do {
> ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));
> + spu = ctx->spu;
> if (unlikely(ret))
> break;
> + if (unlikely(spu->notify_active)) {
> + spu->notify_active = 0;
> + if (!(status & SPU_STATUS_STOPPED_BY_STOP)) {
> + spu_switch_notify(spu, ctx);
> + continue;
> + }
> + }
This is before spu_reacquire_runnable, so in case the spu got
preempted at the same time when oprofile was enabled, ctx->spu
is NULL, and you can't load the notify_active flag from it.
On solution would be to move the notify_active flag from ctx->spu
into ctx itself, but maybe there are other ways to solve this.
Thanks,
Arnd <><
On Monday 29 January 2007 20:45, Maynard Johnson wrote:
> On December 14, 2006, I posted a patch that added support to the
> OProfile kernel driver for profiling Cell SPUs. ?There have been some
> changes/fixes to this patch since the original posting (including
> forward porting from 2.6.18-based kernel to 2.6.20-rc1), so I am
> reposting the patch for review now. ?This patch relies upon the
> following patches that have not been accepted yet:
> ? ?1. oprofile cleanup patch (submitted on Nov 27)
> ? ?2. Fix for PPU profiling (not submitted yet, since it depends on #1)
> ? ?3. SPU task notification patch (last submitted on Jan 26)
>
Sorry for taking so much time before reviewing this. I knew it would
be a long patch that requires time to go through in detail (though
certainly not nearly as much time as it took you to write it), so I
kept procrastinating.
Arnd <><
On Mon, Jan 29, 2007 at 01:46:50PM -0600, Maynard Johnson wrote:
>
>
I don't think the macro removal is helpful, getting rid of the names
makes the code less readable to me.
Arnd Bergmann wrote:
> On Monday 29 January 2007 20:48, Maynard Johnson wrote:
>
>>Subject: Enable SPU switch notification to detect currently active SPU tasks.
>>
>>From: Maynard Johnson <[email protected]>
>>
>>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.
>>
>>Signed-off-by: Maynard Johnson <[email protected]>
>
>
> I looked through it again, and think I found a serious bug, but that
> should be easy enough to solve:
>
>
>>+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_switch_notify();
>>+ */
>>+ 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;
>
>
> [side note]
> There is a small whitespace breakage in here, please make sure you always
> use tabs for indenting, not space characters.
> [/side note]
>
>
>>@@ -45,9 +45,10 @@
>> u64 pte_fault;
>>
>> *stat = ctx->ops->status_read(ctx);
>>- if (ctx->state != SPU_STATE_RUNNABLE)
>>- return 1;
>>+
>> spu = ctx->spu;
>>+ if (ctx->state != SPU_STATE_RUNNABLE || spu->notify_active)
>>+ return 1;
>> pte_fault = spu->dsisr &
>> (MFC_DSISR_PTE_NOT_FOUND | MFC_DSISR_ACCESS_DENIED);
>> return (!(*stat & 0x1) || pte_fault || spu->class_0_pending) ? 1 : 0;
>>@@ -305,6 +306,7 @@
>> u32 *npc, u32 *event)
>> {
>> int ret;
>>+ struct spu * spu;
>> u32 status;
>>
>> if (down_interruptible(&ctx->run_sema))
>>@@ -318,8 +320,16 @@
>>
>> do {
>> ret = spufs_wait(ctx->stop_wq, spu_stopped(ctx, &status));
>>+ spu = ctx->spu;
>> if (unlikely(ret))
>> break;
>>+ if (unlikely(spu->notify_active)) {
>>+ spu->notify_active = 0;
>>+ if (!(status & SPU_STATUS_STOPPED_BY_STOP)) {
>>+ spu_switch_notify(spu, ctx);
>>+ continue;
>>+ }
>>+ }
>
>
> This is before spu_reacquire_runnable, so in case the spu got
> preempted at the same time when oprofile was enabled, ctx->spu
> is NULL, and you can't load the notify_active flag from it.
>
> On solution would be to move the notify_active flag from ctx->spu
> into ctx itself, but maybe there are other ways to solve this.
In an earlier review of this patch, Christopher Hellwig suggested I move
the notify_active flag to be a bit in the sched_flags field that's added
in his scheduler patch series. If this patch series will be a available
in an "Arnd" tree that we'll be using for our current OProfile
development, perhaps I should wait until that time to change this, since
the window of vulnerability is quite small. What do you think?
-Maynard
>
> Thanks,
>
> Arnd <><
Christoph:
In our earlier work on the PPU profiling patch, Benjamin Herrenschmidt
said that we should remove macros that are only used once and just put
the actual code in. That is why the macros were removed.
Carl Love
On Tue, 2007-01-30 at 11:39 +0100, Christoph Hellwig wrote:
> On Mon, Jan 29, 2007 at 01:46:50PM -0600, Maynard Johnson wrote:
> >
> >
>
> I don't think the macro removal is helpful, getting rid of the names
> makes the code less readable to me.
> _______________________________________________
> cbe-oss-dev mailing list
> [email protected]
> https://ozlabs.org/mailman/listinfo/cbe-oss-dev
On Tue, 2007-01-30 at 14:49 -0800, Carl Love wrote:
> Christoph:
>
> In our earlier work on the PPU profiling patch, Benjamin Herrenschmidt
> said that we should remove macros that are only used once and just put
> the actual code in. That is why the macros were removed.
I've looked at the macros you remove and indeed, they look like stuff
you actually want to keep in macros. I don't have off the top of my head
the circumstances where I asked you to remove macros in the PPE code,
but I'm sure it was different.
Ben.
On Tue, 2007-01-30 at 14:49 -0800, Carl Love wrote:
> Christoph:
>
> In our earlier work on the PPU profiling patch, Benjamin Herrenschmidt
> said that we should remove macros that are only used once and just put
> the actual code in. That is why the macros were removed.
Heh... there is a balance to be found... In some cases, inline functions
might be better too.
Ben.
Arnd:
Well most of the patch is simply cleaning up the
pm_rtas_activate_signals() routine. I would be think
"pm_rtas_activat_signals routine cleanup" would be good.
Carl Love
On Tue, 2007-01-30 at 05:08 +0100, Arnd Bergmann wrote:
> On Monday 29 January 2007 20:47, Maynard Johnson wrote:
> > The code was setting up the debug bus for group 21 when profiling on the
> > event PPU CYCLES. The debug bus is not actually used by the hardware
> > performance counters when counting PPU CYCLES. Setting up the debug bus
> > for PPU CYCLES causes signal routing conflicts on the debug bus when
> > profiling PPU cycles and another PPU event. This patch fixes the code to
> > only setup the debug bus to route the performance signals for the non
> > PPU CYCLE events.
> >
> > Signed-off-by: Maynard Johnson <[email protected]>
> > Signed-off-by: Carl Love <[email protected]>
>
> Acked-by: Arnd Bergmann <[email protected]>
>
> Any suggestion for a one-line patch title?
> _______________________________________________
> cbe-oss-dev mailing list
> [email protected]
> https://ozlabs.org/mailman/listinfo/cbe-oss-dev
On Tuesday 30 January 2007 16:31, Maynard Johnson wrote:
>
> > On solution would be to move the notify_active flag from ctx->spu
> > into ctx itself, but maybe there are other ways to solve this.
> In an earlier review of this patch, Christopher Hellwig suggested I move
> the notify_active flag to be a bit in the sched_flags field that's added
> in his scheduler patch series. ?If this patch series will be a available
> in an "Arnd" tree that we'll be using for our current OProfile
> development, perhaps I should wait until that time to change this, since
> the window of vulnerability is quite small. ?What do you think?
Sounds good to me.
Arnd <><
On Wed, Jan 31, 2007 at 09:57:26AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2007-01-30 at 14:49 -0800, Carl Love wrote:
> > Christoph:
> >
> > In our earlier work on the PPU profiling patch, Benjamin Herrenschmidt
> > said that we should remove macros that are only used once and just put
> > the actual code in. That is why the macros were removed.
>
> Heh... there is a balance to be found... In some cases, inline functions
> might be better too.
Well, unless there's a very good reasons against it (token pasting,
header pollution) inlines are always preferable over macros, but I
didn't want to bring that issue up aswell..