I have applied the "cleanup" patch that Arnd sent, but had to fix up a
- Bug fix: Initialize retval in spu_task_sync.c, line 95, otherwise
OProfile this function returns non-zero and OProfile fails.
- Remove unused codes in include/linux/oprofile.h
- Compile warnings: Initialize offset and spu_cookie at lines 283
and 284 in spu_task_sync.c
With these changes and some userspace changes that were necessary to
correspond with Arnd's changes, our testing was successful.
A fixup patch is attached.
P.S. We have a single patch with all these changes applied if anyone
would like us to post it.
Arnd Bergmann wrote:
>On Thursday 22 February 2007, Carl Love wrote:
>>This patch updates the existing arch/powerpc/oprofile/op_model_cell.c
>>to add in the SPU profiling capabilities. In addition, a 'cell' subdirectory
>>was added to arch/powerpc/oprofile to hold Cell-specific SPU profiling
>There was a significant amount of whitespace breakage in this patch,
>which I cleaned up. The patch below consists of the other things
>I changed as a further cleanup. Note that I changed the format
>of the context switch record, which I found too complicated, as
>I described on IRC last week.
> Arnd <><
On Tuesday 27 February 2007, Maynard Johnson wrote:
> I have applied the "cleanup" patch that Arnd sent, but had to fix up a=20
> few things:
> =A0 =A0- =A0Bug fix: =A0Initialize retval in spu_task_sync.c, line 95, ot=
> OProfile this function returns non-zero and OProfile fails.
> =A0 =A0- =A0Remove unused codes in include/linux/oprofile.h
> =A0 =A0- =A0Compile warnings: =A0Initialize offset and spu_cookie at line=
> and 284 in spu_task_sync.c
> With these changes and some userspace changes that were necessary to=20
> correspond with Arnd's changes, our testing was successful.
> A fixup patch is attached.
The patch does not contain any of the metadata I need to apply it
(subject, description, signed-off-by).
> @@ -280,8 +280,8 @@ static int process_context_switch(struct
> unsigned long flags;
> int retval;
> - unsigned int offset;
> - unsigned long spu_cookie, app_dcookie;
> + unsigned int offset =3D 0;
> + unsigned long spu_cookie =3D 0, app_dcookie;
> retval =3D prepare_cached_spu_info(spu, objectId);
> if (retval)
> goto out;
No, this is wrong. Leaving the variables uninitialized at least warns
you about the bug you have in this function: when there is anything wrong,
you just continue writing the record with zero offset and dcookie values
in it. Instead, you should get handle the error condition somewhere
down the code.
It's harmless most of the time, but you really should not be painting
over your bugs by blindly initializing variables.
> diff -paur linux-orig/include/linux/oprofile.h linux-new/include/linux/op=
> --- linux-orig/include/linux/oprofile.h 2007-02-27 14:41:29.000000000 -06=
> +++ linux-new/include/linux/oprofile.h 2007-02-27 14:43:18.000000000 -06=
> @@ -36,9 +36,6 @@
> #define XEN_ENTER_SWITCH_CODE 10
> #define SPU_PROFILING_CODE 11
> #define SPU_CTX_SWITCH_CODE 12
> -#define SPU_OFFSET_CODE 13
> -#define SPU_COOKIE_CODE 14
> -#define SPU_SHLIB_COOKIE_CODE 15
> struct super_block;
> struct dentry;
Right, I forgot about this.