From: Maynard Johnson <hidden> Date: 2007-01-29 19:45:25
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:
=20
=A0 =A0 =A0 =A0 -It removes some macro definitions that are only used once
=A0 =A0 =A0 =A0 =A0with the actual code.
=A0 =A0 =A0 =A0 -Some comments were added to clarify the code based on fe=
edback
=A0 =A0 =A0 =A0 =A0from the community.
=A0 =A0 =A0 =A0 -The write_pm_cntrl() and set_count_mode() were passed a =
structure
=A0 =A0 =A0 =A0 =A0element from a global variable. =A0The argument was re=
moved so the
=A0 =A0 =A0 =A0 =A0functions now just operate on the global directly.
=A0 =A0 =A0 =A0 -The set_pm_event() function call in the cell_virtual_cnt=
r() routine
=A0 =A0 =A0 =A0 =A0was moved to a for-loop before the for_each_cpu loop
=20
Signed-off-by: Carl Love <redacted>
Signed-off-by: Maynard Johnson <redacted>
=20
Acked-by: Arnd Bergmann <redacted>
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 <><
From: Carl Love <hidden> Date: 2007-01-30 22:50:04
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:
quoted
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
cbe-oss-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/cbe-oss-dev
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-30 23:00:21
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.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-30 23:03:06
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.
From: Christoph Hellwig <hch@lst.de> Date: 2007-01-31 08:48:40
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:
quoted
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..
On Monday 29 January 2007 20:47, Maynard Johnson wrote:
The code was setting up the debug bus for group 21 when profiling on th=
e=20
event PPU CYCLES. =A0The debug bus is not actually used by the hardware=20
performance counters when counting PPU CYCLES. =A0Setting up the debug bus
for PPU CYCLES causes signal routing conflicts on the debug bus when=20
profiling PPU cycles and another PPU event. =A0This patch fixes the code =
to=20
only setup the debug bus to route the performance signals for the non
PPU CYCLE events.
=20
Signed-off-by: Maynard Johnson <redacted>
Signed-off-by: Carl Love <redacted>
Acked-by: Arnd Bergmann <redacted>
Any suggestion for a one-line patch title?
From: Carl Love <hidden> Date: 2007-01-30 23:51:10
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:
quoted
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 <redacted>
Signed-off-by: Carl Love <redacted>
Acked-by: Arnd Bergmann <redacted>
Any suggestion for a one-line patch title?
_______________________________________________
cbe-oss-dev mailing list
cbe-oss-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/cbe-oss-dev
On Monday 29 January 2007 20:48, Maynard Johnson wrote:
Subject: Enable SPU switch notification to detect currently active SPU tasks.
From: Maynard Johnson <redacted>
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 <redacted>
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]
@@ -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 <><
From: Maynard Johnson <hidden> Date: 2007-01-30 15:31:44
Arnd Bergmann wrote:
On Monday 29 January 2007 20:48, Maynard Johnson wrote:
quoted
Subject: Enable SPU switch notification to detect currently active SPU tasks.
From: Maynard Johnson <redacted>
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 <redacted>
I looked through it again, and think I found a serious bug, but that
should be easy enough to solve:
quoted
+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]
{
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
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-30 07:56:09
quoted
+/* Defines used for sync_start */
+#define SKIP_GENERIC_SYNC 0
+#define SYNC_START_ERROR -1
+#define DO_GENERIC_SYNC 1
+
+typedef struct vma_map
+{
+ struct vma_map *next;
+ unsigned int vma;
+ unsigned int size;
+ unsigned int offset;
+ unsigned int guard_ptr;
+ unsigned int guard_val;
+} vma_map_t;
I haven't had time to look in details yet but in that context, what does
"vma" stands for ? There's already an important vm data structure in
linux routinely called "vma" and thus I suspect this is a poor naming
choice as it will cause confusion.
Cheers,
Ben.
From: Christoph Hellwig <hch@lst.de> Date: 2007-01-30 10:41:37
On Tue, Jan 30, 2007 at 06:53:50PM +1100, Benjamin Herrenschmidt wrote:
quoted
quoted
+/* Defines used for sync_start */
+#define SKIP_GENERIC_SYNC 0
+#define SYNC_START_ERROR -1
+#define DO_GENERIC_SYNC 1
+
+typedef struct vma_map
+{
+ struct vma_map *next;
+ unsigned int vma;
+ unsigned int size;
+ unsigned int offset;
+ unsigned int guard_ptr;
+ unsigned int guard_val;
+} vma_map_t;
I haven't had time to look in details yet but in that context, what does
"vma" stands for ? There's already an important vm data structure in
linux routinely called "vma" and thus I suspect this is a poor naming
choice as it will cause confusion.
It looks like it actually is dealing with vma to me. But then again:
- please don't use typedefs for structures
- there might be a more descriptive name for this than just vma_map
From: Maynard Johnson <hidden> Date: 2007-01-30 23:09:17
Christoph Hellwig wrote:
On Tue, Jan 30, 2007 at 06:53:50PM +1100, Benjamin Herrenschmidt wrote:
quoted
quoted
quoted
+/* Defines used for sync_start */
+#define SKIP_GENERIC_SYNC 0
+#define SYNC_START_ERROR -1
+#define DO_GENERIC_SYNC 1
+
+typedef struct vma_map
+{
+ struct vma_map *next;
+ unsigned int vma;
+ unsigned int size;
+ unsigned int offset;
+ unsigned int guard_ptr;
+ unsigned int guard_val;
+} vma_map_t;
I haven't had time to look in details yet but in that context, what does
"vma" stands for ? There's already an important vm data structure in
linux routinely called "vma" and thus I suspect this is a poor naming
choice as it will cause confusion.
It looks like it actually is dealing with vma to me. But then again:
- please don't use typedefs for structures
- there might be a more descriptive name for this than just vma_map
Yes, I'll come up with some (hopefully) better name.
From: Maynard Johnson <hidden> Date: 2007-01-30 21:41:20
Arnd Bergmann wrote:
On Monday 29 January 2007 20:48, Maynard Johnson wrote:
quoted
Subject: Add support to OProfile for profiling Cell BE SPUs
From: Maynard Johnson <redacted>
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
code.
Signed-off-by: Carl Love <redacted>
Signed-off-by: Maynard Johnson <redacted>
I can't really say much about the common oprofile files that you are
touching, maybe someone from oprofile-list (Philippe?) to look over them
and ack/nack them.
Anton (added to cc list) may be another good reviewer of
drivers/oprofile changes.
I've been discussing with benh about a better way to do this. We should
change all the references to nodes and cpu numbers to something more
correct in the future, so we get rid of the assumption that each
numa node is a cell chip. It's probably best to leave your code as
is for now, but we need to remember this one when cleaning it up.
You should however convert this into an inline function
of prototype
static inline int number_of_online_nodes(void)
instead of a macro.
OK.
quoted
+/* Defines used for sync_start */
+#define SKIP_GENERIC_SYNC 0
+#define SYNC_START_ERROR -1
+#define DO_GENERIC_SYNC 1
+
+typedef struct vma_map
+{
+ struct vma_map *next;
+ unsigned int vma;
+ unsigned int size;
+ unsigned int offset;
+ unsigned int guard_ptr;
+ unsigned int guard_val;
+} vma_map_t;
please don't typedef structures.
Sure.
[snip]
quoted
+
+static int spu_prof_running = 0;
+static unsigned int profiling_interval = 0;
+
+extern int num_nodes;
+extern unsigned int khzfreq;
You really can't have global variable with such generic names. For
static variables, it's less of a problem, since they are not in the
same name space, but for easier debugging, it's good to always have
the name of your module (e.g. spu_prof_) as a prefix to the identifier.
OK, we'll add 'spu_prof' prefix to them.
Of course, the best way would be to avoid global and static variables
entirely, but that's not always possible.
[snip]
quoted
+ // process the collected SPU PC for each node
+ for_each_online_cpu(cpu) {
+ if (cbe_get_hw_thread_id(cpu))
+ continue;
+
+ node = cbe_cpu_to_node(cpu);
+ node_factor = node * SPUS_PER_NODE;
+ /* number of valid entries for this node */
+ entry = 0;
+
+ trace_addr = cbe_read_pm(cpu, trace_address);
+ while ((trace_addr & CBE_PM_TRACE_BUF_EMPTY) != 0x400)
+ {
+ /* there is data in the trace buffer to process */
+ cbe_read_trace_buffer(cpu, trace_buffer);
+ spu_mask = 0xFFFF000000000000;
+
+ /* Each SPU PC is 16 bits; hence, four spus in each of
+ * the two 64-bit buffer entries that make up the
+ * 128-bit trace_buffer entry. Process the upper and
+ * lower 64-bit values simultaneously.
+ */
+ for (spu = 0; spu < SPUS_PER_TB_ENTRY; spu++) {
+ spu_pc_lower = spu_mask & trace_buffer[0];
+ spu_pc_lower = spu_pc_lower >> (NUM_SPU_BITS_TRBUF
+ * (SPUS_PER_TB_ENTRY-spu-1));
+
+ spu_pc_upper = spu_mask & trace_buffer[1];
+ spu_pc_upper = spu_pc_upper >> (NUM_SPU_BITS_TRBUF
+ * (SPUS_PER_TB_ENTRY-spu-1));
+
+ spu_mask = spu_mask >> NUM_SPU_BITS_TRBUF;
+
+ /* spu PC trace entry is upper 16 bits of the
+ * 18 bit SPU program counter
+ */
+ spu_pc_lower = spu_pc_lower << 2;
+ spu_pc_upper = spu_pc_upper << 2;
+
+ samples[((node_factor + spu) * TRACE_ARRAY_SIZE) + entry]
+ = (u32) spu_pc_lower;
+ samples[((node_factor + spu + SPUS_PER_TB_ENTRY) * TRACE_ARRAY_SIZE) + entry]
+ = (u32) spu_pc_upper;
+ }
+
+ entry++;
+
+ if (entry >= TRACE_ARRAY_SIZE)
+ /* spu_samples is full */
+ break;
+
+ trace_addr = cbe_read_pm(cpu, trace_address);
+ }
+ samples_per_node[node] = entry;
+ }
+}
While I can't see anything technically wrong with this function, it would be
good to split it into smaller functions. Since you are nesting three
loops, it should be possible to make a separate function from one of the
inner loops without changing the actual logic behind it.
Will do.
quoted
+
+static int profile_spus(struct hrtimer * timer)
+{
+ ktime_t kt;
+ int cpu, node, k, num_samples, spu_num;
whitespace damage
fixed
quoted
+
+ if (!spu_prof_running)
+ goto STOP;
+
+ cell_spu_pc_collection();
+ for_each_online_cpu(cpu) {
+ if (cbe_get_hw_thread_id(cpu))
+ continue;
Here, you enter the same top-level loop again, why not make it
for_each_online_cpu(cpu) {
if (cbe_get_hw_thread_id(cpu))
continue;
num_samples = cell_spu_pc_collection(cpu);
...
is hrtimer_forward really the right interface here? You are ignoring
the number of overruns anyway, so hrtimer_start(,,) sounds more
correct to me.
According to Tom Gleixner, "hrtimer_forward is a convenience function to
move the expiry time of a timer forward in multiples of the interval, so
it is in the future. After setting the expiry time you restart the
timer either with [sic] a return HRTIMER_RESTART (if you are in the
timer callback function)."
+void start_spu_profiling(unsigned int cycles_reset) {
+
+ ktime_t kt;
+
+ /* To calculate a timeout in nanoseconds, the basic
+ * formula is ns = cycles_reset * (NSEC_PER_SEC / cpu frequency).
+ * To avoid floating point math, we use the scale math
+ * technique as described in linux/jiffies.h. We use
+ * a scale factor of SCALE_SHIFT,which provides 4 decimal places
+ * of precision, which is close enough for the purpose at hand.
+ */
+
+ /* Since cpufreq_quick_get returns frequency in kHz, we use
+ * USEC_PER_SEC here vs NSEC_PER_SEC.
+ */
+ unsigned long nsPerCyc = (USEC_PER_SEC << SCALE_SHIFT)/khzfreq;
+ profiling_interval = (nsPerCyc * cycles_reset) >> SCALE_SHIFT;
+
+ pr_debug("timer resolution: %lu\n",
+ TICK_NSEC);
Don't you need to adapt the profiling_interval at run time, when cpufreq
changes the core frequency? You should probably use
cpufreq_register_notifier() to update this.
Since OProfile is a statistical profiler, the exact frequency is not
critical. The user is going to be looking for hot spots in their code,
so it's all relative. With that said, I don't imagine using the
cpufreq notiication would be a big deal. We'll look at it.
shouldn't you set spu_prof_running = 0 before doing any of the other things?
It looks to me like you could otherwise get into a use-after-free
situation. If I'm wrong with that, you probably don't need spu_prof_running
at all ;-)
No, you're right. :-)
quoted
+/* Conainer for caching information about an active SPU task.
+ * :map -- pointer to a list of vma_maps
+ * :spu -- the spu for this active SPU task
+ * :list -- potentially could be used to contain the cached_infos
+ * for inactive SPU tasks.
Documenting structures is good, but please use the common kerneldoc format
for it. There are a number of examples for this in include/linux/
OK
quoted
+ *
+ * Ideally, we would like to be able to create the cached_info for
+ * an SPU task just one time -- when libspe first loads the SPU
+ * binary file. We would store the cached_info in a list. Then, as
+ * SPU tasks are switched out and new ones switched in, the cached_info
+ * for inactive tasks would be kept, and the active one would be placed
+ * at the head of the list. But this technique may not with
+ * current spufs functionality since the spu used in bind_context may
+ * be a different spu than was used in a previous bind_context for a
+ * reactivated SPU task. Additionally, a reactivated SPU task may be
+ * assigned to run on a different physical SPE. We will investigate
+ * further if this can be done.
+ *
+ */
You should stuff a pointer to cached_info into struct spu_context,
e.g. 'void *profile_private'.
And replace the 'the_spu' member with a back pointer to the
spu_context if you need it.
quoted
+
+/* A data structure for cached information about active SPU tasks.
+ * Storage is dynamically allocated, sized as
+ * "number of active nodes multplied by 8".
+ * The info_list[n] member holds 0 or more
+ * 'struct cached_info' objects for SPU#=n.
+ *
+ * As currently implemented, there will only ever be one cached_info
+ * in the list for a given SPU. If we can devise a way to maintain
+ * multiple cached_infos in our list, then it would make sense
+ * to also cache the dcookie representing the PPU task application.
+ * See above description of struct cached_info for more details.
+ */
+struct spu_info_stacks {
+ struct list_head * info_list;
+};
Why do you store pointers to list_head structures? If you want to store
lists, you should have a lists_head itself in here.
info_list is an array of n lists, where n is the number of SPUs.
Why do you store them per spu in the first place? The physical spu
doesn't have any relevance to this at all, the only data that is
per spu is the sample data collected on a profiling interrupt,
which you can then copy in the per-context data on a context switch.
The sample data is written out to the event buffer on every profiling
interrupt. But we don't write out the SPU program counter samples
directly to the event buffer. First, we have to find the cached_info
for the appropriate SPU context to retrieve the cached vma-to-fileoffset
map. Then we do the vma_map_lookup to find the fileoffset corresponding
to the SPU PC sample, which we then write out to the event buffer. This
is one of the most time-critical pieces of the SPU profiling code, so I
used an array to hold the cached_info for fast random access. But as I
stated in a code comment above, the negative implication of this current
implementation is that the array can only hold the cached_info for
currently running SPU tasks. I need to give this some more thought.
quoted
+/* Looks for cached info for the passed spu. If not found, the
+ * cached info is created for the passed spu.
+ * Returns 0 for success; otherwise, -1 for error.
+ */
+static int
+prepare_cached_spu_info(struct spu * spu, unsigned int objectId)
+{
see above, this should get the spu_context pointer as its argument,
not the spu.
quoted
+ vma_map_t * new_map;
+ unsigned long flags = 0;
+ int retval = 0;
+ /* spu->number is a system-wide value, not a per-node value. */
+ struct cached_info * info = get_cached_info(spu->number);
+ if (info == NULL) {
if you revert the logic to
if (info)
goto out;
then the bulk of your function doesn't need to get indented,
which helps readability.
OK
quoted
+ /* create cached_info and add it to the list for SPU #<n>.*/
+ info = kzalloc(sizeof(struct cached_info), GFP_ATOMIC);
GFP_KERNEL
OK
quoted
+ if (!info) {
+ printk(KERN_ERR "SPU_PROF: "
+ "%s, line %d: create vma_map failed\n",
+ __FUNCTION__, __LINE__);
+ goto ERR_ALLOC;
+ }
+ new_map = create_vma_map(spu, objectId);
+ if (!new_map) {
+ printk(KERN_ERR "SPU_PROF: "
+ "%s, line %d: create vma_map failed\n",
+ __FUNCTION__, __LINE__);
+ goto ERR_ALLOC;
+ }
+
+ pr_debug("Created vma_map\n");
+ info->map = new_map;
+ info->the_spu = spu;
+ kref_init(&info->cache_ref);
+ spin_lock_irqsave(&cache_lock, flags);
+ list_add(&info->list, &spu_info->info_list[spu->number]);
+ spin_unlock_irqrestore(&cache_lock, flags);
+ goto OUT;
+ } else {
+ /* Immedidately put back reference to cached_info since we don't
+ * really need it -- just checking whether we have it.
+ */
+ put_cached_info(info);
+ pr_debug("Found cached SPU info.\n");
+ }
+
+ERR_ALLOC:
+ retval = -1;
+OUT:
+ return retval;
+}
quoted
+/* Look up the dcookie for the task's first VM_EXECUTABLE mapping,
+ * which corresponds loosely to "application name". Also, determine
+ * the offset for the SPU ELF object. If computed offset is
+ * non-zero, it implies an embedded SPU object; otherwise, it's a
+ * separate SPU binary, in which case we retrieve it's dcookie.
+ */
+static unsigned long
+get_exec_dcookie_and_offset(
+ struct spu * spu, unsigned int * offsetp,
+ unsigned long * spu_bin_dcookie,
+ unsigned int spu_ref)
+{
+ unsigned long cookie = 0;
+ unsigned int my_offset = 0;
+ struct vm_area_struct * vma;
+ struct mm_struct * mm = spu->mm;
indenting
uh-huh
quoted
+ if (!mm)
+ goto OUT;
+
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ if (!vma->vm_file)
+ continue;
+ if (!(vma->vm_flags & VM_EXECUTABLE))
+ continue;
+ cookie = fast_get_dcookie(vma->vm_file->f_dentry,
+ vma->vm_file->f_vfsmnt);
+ pr_debug("got dcookie for %s\n",
+ vma->vm_file->f_dentry->d_name.name);
+ break;
+ }
+
+ for (vma = mm->mmap; vma; vma = vma->vm_next) {
+ if (vma->vm_start > spu_ref || vma->vm_end < spu_ref)
+ continue;
+ my_offset = spu_ref - vma->vm_start;
+ pr_debug("Found spu ELF at "
+ " %X for file %s\n", my_offset,
+ vma->vm_file->f_dentry->d_name.name);
+ *offsetp = my_offset;
+ if (my_offset == 0) {
+ if (!vma->vm_file) {
+ goto FAIL_NO_SPU_COOKIE;
+ }
+ *spu_bin_dcookie = fast_get_dcookie(
+ vma->vm_file->f_dentry,
+ vma->vm_file->f_vfsmnt);
+ pr_debug("got dcookie for %s\n",
+ vma->vm_file->f_dentry->d_name.name);
+ }
+ break;
+ }
+
+OUT:
+ return cookie;
+
+FAIL_NO_SPU_COOKIE:
+ printk(KERN_ERR "SPU_PROF: "
+ "%s, line %d: Cannot find dcookie for SPU binary\n",
+ __FUNCTION__, __LINE__);
+ goto OUT;
+}
+
+
+
+/* This function finds or creates cached context information for the
+ * passed SPU and records SPU context information into the OProfile
+ * event buffer.
+ */
+static int process_context_switch(struct spu * spu, unsigned int objectId)
+{
+ unsigned long flags = 0;
+ int retval = 0;
+ unsigned int offset = 0;
+ unsigned long spu_cookie = 0, app_dcookie = 0;
+ retval = prepare_cached_spu_info(spu, objectId);
+ if (retval == -1) {
+ goto OUT;
+ }
+ /* Get dcookie first because a mutex_lock is taken in that
+ * code path, so interrupts must not be disabled.
+ */
+ app_dcookie = get_exec_dcookie_and_offset(spu, &offset,
+ &spu_cookie, objectId);
+
+ /* Record context info in event buffer */
+ spin_lock_irqsave(&buffer_lock, flags);
+ add_event_entry(ESCAPE_CODE);
+ add_event_entry(SPU_CTX_SWITCH_CODE);
+ add_event_entry(spu->number);
+ add_event_entry(spu->pid);
+ add_event_entry(spu->tgid);
+ add_event_entry(app_dcookie);
+
+ add_event_entry(ESCAPE_CODE);
+ if (offset) {
+ /* When offset is non-zero, this means the SPU ELF was embedded;
+ * otherwise, it was loaded from a separate binary file. For the
+ * embedded case, we record the offset of the SPU ELF into the PPU
+ * executable; for the non-embedded case, we record a dcookie that
+ * points to the location of the SPU binary that was loaded.
+ */
+ add_event_entry(SPU_OFFSET_CODE);
+ add_event_entry(offset);
+ } else {
+ add_event_entry(SPU_COOKIE_CODE);
+ add_event_entry(spu_cookie);
+ }
I don't get it. What is the app_dcookie used for? If the spu binary is
embedded into a library, you are still missing the dcookie to that .so file,
because you return only an offset.
For embedded SPU app, the post-processing tool opens the PPE binary app
file and obtains the SPU ELF embedded thereine, and from that, we obtain
the name of the SPU binary. Also, the app name is included in the
report, along with the SPU binary filename, if the report contains
samples from more than one application.
<nitpicking>
quoted
+ unsigned long flags = 0;
no need to initialize
quoted
+ struct spu * the_spu = (struct spu *) data;
no need for the cast
quoted
+ pr_debug("SPU event notification arrived\n");
+ if (val == 0){
if (!val)
quoted
+ pr_debug("spu_sync_start -- running.\n");
+OUT:
out:
quoted
+ return ret;
+}
</nitpicking>
OK, we'll fix up.
quoted
@@ -480,7 +491,22 @@ struct op_system_config *sys, int num_ctrs)
{
int i, j, cpu;
+ spu_cycle_reset = 0;
+ /* The cpufreq_quick_get function requires that cbe_cpufreq module
+ * be loaded. This function is not actually provided and exported
+ * by cbe_cpufreq, but it relies on cbe_cpufreq initialize kernel
+ * data structures. Since there's no way for depmod to realize
+ * that our OProfile module depends on cbe_cpufreq, we currently
+ * are letting the userspace tool, opcontrol, ensure that the
+ * cbe_cpufreq module is loaded.
+ */
+ khzfreq = cpufreq_quick_get(smp_processor_id());
You should probably have a fallback in here in case the cpufreq module
is not loaded. There is a global variable ppc_proc_freq (in Hz) that
you can access.
Our userspace tool ensures the cpufreq module is loaded.
I don't have the slightest idea what this code is about, but
Me neither. Carl, can you comment?
it certainly looks inefficient to loop 16 million times to
compute a constant. Could you use a faster algorithm instead,
or at least add a comment about why you do it this way?
This looks weird as well. I suppose it's a limitation of the hardware
that you can only do either ppu or spu profiling. However, making that
dependent of whether the 'spu_cycle_reset' variable is set sounds
rather bogus.
It is the only file-scoped variable relating to SPU profiling, and will
always be non-zero when the user selects to perform SPU profiling.
Seemed like a logical-enough choice to me.
I don't know what the best interface for choosing the target from
user space would be, but you probably also want to be able to
switch between them at run time.
The hardware setup is so completely different, I don't think there's a
viable way of switching between PPU profiling and SPU profiling without
a "stop" in between.
From: Maynard Johnson <hidden> Date: 2007-01-30 22:54:11
Maynard Johnson wrote:
Arnd Bergmann wrote:
quoted
On Monday 29 January 2007 20:48, Maynard Johnson wrote:
quoted
Subject: Add support to OProfile for profiling Cell BE SPUs
From: Maynard Johnson <redacted>
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
code.
Signed-off-by: Carl Love <redacted>
Signed-off-by: Maynard Johnson <redacted>
[snip]
quoted
quoted
+ *
+ * Ideally, we would like to be able to create the cached_info for
+ * an SPU task just one time -- when libspe first loads the SPU
+ * binary file. We would store the cached_info in a list. Then, as
+ * SPU tasks are switched out and new ones switched in, the cached_info
+ * for inactive tasks would be kept, and the active one would be placed
+ * at the head of the list. But this technique may not with
+ * current spufs functionality since the spu used in bind_context may
+ * be a different spu than was used in a previous bind_context for a
+ * reactivated SPU task. Additionally, a reactivated SPU task may be
+ * assigned to run on a different physical SPE. We will investigate
+ * further if this can be done.
+ *
+ */
You should stuff a pointer to cached_info into struct spu_context,
e.g. 'void *profile_private'.
And replace the 'the_spu' member with a back pointer to the
spu_context if you need it.
quoted
+
+/* A data structure for cached information about active SPU tasks.
+ * Storage is dynamically allocated, sized as
+ * "number of active nodes multplied by 8".
+ * The info_list[n] member holds 0 or more
+ * 'struct cached_info' objects for SPU#=n.
+ *
+ * As currently implemented, there will only ever be one cached_info
+ * in the list for a given SPU. If we can devise a way to maintain
+ * multiple cached_infos in our list, then it would make sense
+ * to also cache the dcookie representing the PPU task application.
+ * See above description of struct cached_info for more details.
+ */
+struct spu_info_stacks {
+ struct list_head * info_list;
+};
Why do you store pointers to list_head structures? If you want to store
lists, you should have a lists_head itself in here.
info_list is an array of n lists, where n is the number of SPUs.
quoted
Why do you store them per spu in the first place? The physical spu
doesn't have any relevance to this at all, the only data that is
per spu is the sample data collected on a profiling interrupt,
which you can then copy in the per-context data on a context switch.
The sample data is written out to the event buffer on every profiling
interrupt. But we don't write out the SPU program counter samples
directly to the event buffer. First, we have to find the cached_info
for the appropriate SPU context to retrieve the cached vma-to-fileoffset
map. Then we do the vma_map_lookup to find the fileoffset corresponding
to the SPU PC sample, which we then write out to the event buffer. This
is one of the most time-critical pieces of the SPU profiling code, so I
used an array to hold the cached_info for fast random access. But as I
stated in a code comment above, the negative implication of this current
implementation is that the array can only hold the cached_info for
currently running SPU tasks. I need to give this some more thought.
I've given this some more thought, and I'm coming to the conclusion that
a pure array-based implementation for holding cached_info (getting rid
of the lists) would work well for the vast majority of cases in which
OProfile will be used. Yes, it is true that the mapping of an SPU
context to a phsyical spu-numbered array location cannot be guaranteed
to stay valid, and that's why I discard the cached_info at that array
location when the SPU task is switched out. Yes, it would be terribly
inefficient if the same SPU task gets switched back in later and we
would have to recreate the cached_info. However, I contend that
OProfile users are interested in profiling one application at a time.
They are not going to want to muddy the waters with multiple SPU apps
running at the same time. I can't think of any reason why someone would
conscisouly choose to do that.
Any thoughts from the general community, especially OProfile users?
Thanks.
-Maynard
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2007-01-30 23:34:32
I've given this some more thought, and I'm coming to the conclusion that
a pure array-based implementation for holding cached_info (getting rid
of the lists) would work well for the vast majority of cases in which
OProfile will be used. Yes, it is true that the mapping of an SPU
context to a phsyical spu-numbered array location cannot be guaranteed
to stay valid, and that's why I discard the cached_info at that array
location when the SPU task is switched out. Yes, it would be terribly
inefficient if the same SPU task gets switched back in later and we
would have to recreate the cached_info. However, I contend that
OProfile users are interested in profiling one application at a time.
They are not going to want to muddy the waters with multiple SPU apps
running at the same time. I can't think of any reason why someone would
conscisouly choose to do that.
Any thoughts from the general community, especially OProfile users?
Well, it's my understanding that quite a few typical usage scenario
involve different tasks running on different SPUs passing each other
data around.
Ben.
From: Maynard Johnson <hidden> Date: 2007-01-31 00:29:45
Benjamin Herrenschmidt wrote:
quoted
I've given this some more thought, and I'm coming to the conclusion that
a pure array-based implementation for holding cached_info (getting rid
of the lists) would work well for the vast majority of cases in which
OProfile will be used. Yes, it is true that the mapping of an SPU
context to a phsyical spu-numbered array location cannot be guaranteed
to stay valid, and that's why I discard the cached_info at that array
location when the SPU task is switched out. Yes, it would be terribly
inefficient if the same SPU task gets switched back in later and we
would have to recreate the cached_info. However, I contend that
OProfile users are interested in profiling one application at a time.
They are not going to want to muddy the waters with multiple SPU apps
running at the same time. I can't think of any reason why someone would
conscisouly choose to do that.
Any thoughts from the general community, especially OProfile users?
Well, it's my understanding that quite a few typical usage scenario
involve different tasks running on different SPUs passing each other
data around.
That shouldn't be a problem. I would consider this to be "one large
application" consisting of multiple SPU binaries running simultaneously.
Such a scenario can be handled with no negative performance impact
using a simple 16 element array of cached_info objects -- as long as
there isn't (much) SPU task switching being done.
-Maynard
On Tuesday 30 January 2007 23:54, Maynard Johnson wrote:
quoted
quoted
Why do you store them per spu in the first place? The physical spu
doesn't have any relevance to this at all, the only data that is
per spu is the sample data collected on a profiling interrupt,
which you can then copy in the per-context data on a context switch.
=20
The sample data is written out to the event buffer on every profiling=20
interrupt. =A0But we don't write out the SPU program counter samples=20
directly to the event buffer. =A0First, we have to find the cached_info=
=20
quoted
for the appropriate SPU context to retrieve the cached vma-to-fileoffse=
t=20
quoted
map. =A0Then we do the vma_map_lookup to find the fileoffset correspond=
ing=20
quoted
to the SPU PC sample, which we then write out to the event buffer. =A0T=
his=20
quoted
is one of the most time-critical pieces of the SPU profiling code, so I=
=20
quoted
used an array to hold the cached_info for fast random access. =A0But as=
I=20
quoted
stated in a code comment above, the negative implication of this curren=
t=20
quoted
implementation is that the array can only hold the cached_info for=20
currently running SPU tasks. =A0I need to give this some more thought.
=20
I've given this some more thought, and I'm coming to the conclusion that=
=20
a pure array-based implementation for holding cached_info (getting rid=20
of the lists) would work well for the vast majority of cases in which=20
OProfile will be used. =A0Yes, it is true that the mapping of an SPU=20
context to a phsyical spu-numbered array location cannot be guaranteed=20
to stay valid, and that's why I discard the cached_info at that array=20
location when the SPU task is switched out. =A0Yes, it would be terribly=
=20
inefficient if the same SPU task gets switched back in later and we=20
would have to recreate the cached_info. =A0However, I contend that=20
OProfile users are interested in profiling one application at a time.=20
They are not going to want to muddy the waters with multiple SPU apps=20
running at the same time. =A0I can't think of any reason why someone woul=
d=20
conscisouly choose to do that.
=20
Any thoughts from the general community, especially OProfile users?
=20
Please assume that in the near future we will be scheduling SPU contexts
in and out multiple times a second. Even in a single application, you
can easily have more contexts than you have physical SPUs.
The event buffer by definition needs to be per context. If you for some
reason want to collect the samples per physical SPU during an event
interrupt, you should at least make sure that they are copied into the
per-context event buffer on a context switch.
At the context switch point, you probably also want to drain the
hw event counters, so that you account all events correctly.
We also want to be able to profile the context switch code itself, which
means that we also need one event buffer associated with the kernel to
collect events that for a zero context_id.
Of course, the recording of raw samples in the per-context buffer does
not need to have the dcookies along with it, you can still resolve
the pointers when the SPU context gets destroyed (or an object gets
unmapped).
Arnd <><
From: Maynard Johnson <hidden> Date: 2007-02-02 16:47:33
Arnd Bergmann wrote:
On Tuesday 30 January 2007 23:54, Maynard Johnson wrote:
quoted
quoted
quoted
Why do you store them per spu in the first place? The physical spu
doesn't have any relevance to this at all, the only data that is
per spu is the sample data collected on a profiling interrupt,
which you can then copy in the per-context data on a context switch.
The sample data is written out to the event buffer on every profiling
interrupt. But we don't write out the SPU program counter samples
directly to the event buffer. First, we have to find the cached_info
for the appropriate SPU context to retrieve the cached vma-to-fileoffset
map. Then we do the vma_map_lookup to find the fileoffset corresponding
to the SPU PC sample, which we then write out to the event buffer. This
is one of the most time-critical pieces of the SPU profiling code, so I
used an array to hold the cached_info for fast random access. But as I
stated in a code comment above, the negative implication of this current
implementation is that the array can only hold the cached_info for
currently running SPU tasks. I need to give this some more thought.
I've given this some more thought, and I'm coming to the conclusion that
a pure array-based implementation for holding cached_info (getting rid
of the lists) would work well for the vast majority of cases in which
OProfile will be used. Yes, it is true that the mapping of an SPU
context to a phsyical spu-numbered array location cannot be guaranteed
to stay valid, and that's why I discard the cached_info at that array
location when the SPU task is switched out. Yes, it would be terribly
inefficient if the same SPU task gets switched back in later and we
would have to recreate the cached_info. However, I contend that
OProfile users are interested in profiling one application at a time.
They are not going to want to muddy the waters with multiple SPU apps
running at the same time. I can't think of any reason why someone would
conscisouly choose to do that.
Any thoughts from the general community, especially OProfile users?
Please assume that in the near future we will be scheduling SPU contexts
in and out multiple times a second. Even in a single application, you
can easily have more contexts than you have physical SPUs.
Arnd, thanks for pointing this out. That's definitely a good reason why
my simplistic approach won't work. I'll look at other options.
The event buffer by definition needs to be per context. If you for some
Yes, and it is. Right now, with the current simplistic approach, the
context and the physical SPU are kept in sync.
reason want to collect the samples per physical SPU during an event
interrupt, you should at least make sure that they are copied into the
per-context event buffer on a context switch.
At the context switch point, you probably also want to drain the
hw event counters, so that you account all events correctly.
Yeah, that's a good idea. The few extraneous invalid samples would
probably never rise above the noise level, but we should do this anyway
for completeness.
We also want to be able to profile the context switch code itself, which
means that we also need one event buffer associated with the kernel to
collect events that for a zero context_id.
The hardware design precludes tracing both SPU and PPU simultaneously.
-Maynard
Of course, the recording of raw samples in the per-context buffer does
not need to have the dcookies along with it, you can still resolve
the pointers when the SPU context gets destroyed (or an object gets
unmapped).
Arnd <><
On Friday 02 February 2007 17:47, Maynard Johnson wrote:
quoted
We also want to be able to profile the context switch code itself, which
means that we also need one event buffer associated with the kernel to
collect events that for a zero context_id.
The hardware design precludes tracing both SPU and PPU simultaneously.
I mean the SPU-side part of the context switch code, which you can find
in arch/powerpc/platforms/cell/spufs/spu_{save,restore}*.
This code is the one that runs when context_id == 0 is passed to the
callback.
Arnd <><
From: Maynard Johnson <hidden> Date: 2007-02-03 20:03:10
Arnd Bergmann wrote:
On Friday 02 February 2007 17:47, Maynard Johnson wrote:
quoted
quoted
We also want to be able to profile the context switch code itself, which
means that we also need one event buffer associated with the kernel to
collect events that for a zero context_id.
The hardware design precludes tracing both SPU and PPU simultaneously.
I mean the SPU-side part of the context switch code, which you can find
in arch/powerpc/platforms/cell/spufs/spu_{save,restore}*.
This code is the one that runs when context_id == 0 is passed to the
callback.
I presume you mean 'object_id'. What you're asking for is a new
requirement, and one which I don't believe is achievable in the current
timeframe. Since this is spufs code that's dynamicaly loaded into the
SPU at runtime, the symbol information for this code is not accessible
to the userspace post-processing tools. It would require an altogether
different mechanism to record samples along with necessary information,
not to mention the changes required in the post-processing tools. This
will have to be a future enhancement.
-Maynard
On Saturday 03 February 2007 21:03, Maynard Johnson wrote:
I presume you mean 'object_id'.=20
Right, sorry for the confusion.
What you're asking for is a new =20
requirement, and one which I don't believe is achievable in the current=20
timeframe. =A0Since this is spufs code that's dynamicaly loaded into the=
=20
SPU at runtime, the symbol information for this code is not accessible=20
to the userspace post-processing tools.
We can always fix the user space tool later, but it is important to
get at least the kernel interface right, so we can add the functionality
later without breaking new user space on old kernels or vice versa.
It would require an altogether =20
different mechanism to record samples along with necessary information,=20
not to mention the changes required in the post-processing tools. =A0This=
=20
will have to be a future enhancement.
So what do you do with the samples for object_id =3D=3D 0? I would expect t=
hem
to be simply added to the sample buffer for the kernel, which is sort
of special in oprofile anyway.
Arnd <><
From: Maynard Johnson <hidden> Date: 2007-02-04 17:11:56
Arnd Bergmann wrote:
On Saturday 03 February 2007 21:03, Maynard Johnson wrote:
quoted
I presume you mean 'object_id'.
Right, sorry for the confusion.
quoted
What you're asking for is a new
requirement, and one which I don't believe is achievable in the current
timeframe. Since this is spufs code that's dynamicaly loaded into the
SPU at runtime, the symbol information for this code is not accessible
to the userspace post-processing tools.
We can always fix the user space tool later, but it is important to
get at least the kernel interface right, so we can add the functionality
later without breaking new user space on old kernels or vice versa.
There's no obvious solution to this problem, so it's going to take some
design effort to come up with a solution. We can work on the problem,
but I don't think we can allow it to get in the way of getting our
currently proposed SPU profiling functionality into the kernel.
quoted
It would require an altogether
different mechanism to record samples along with necessary information,
not to mention the changes required in the post-processing tools. This
will have to be a future enhancement.
So what do you do with the samples for object_id == 0? I would expect them
to be simply added to the sample buffer for the kernel, which is sort
of special in oprofile anyway.
There is no sample buffer for the kernel in SPU profiling. When
OProfile gets notified of an SPU task switching out (object_if == 0), we
stop recording samples for the corresponding SPU. If SPUFs sends the
notification after the spu_save operation, we still would be collecting
samples during that time; however, since the VMAs of these samples would
not map to any fileoffset in the SPU binary that's executing, our
current implementation throws them away. We could change that behavior
and record them in the samples buffer as "anonymous samples". Not sure
it that would help too much, as you wouldn't be able to distinguish
between spu_save samples and samples from generated stubs that are
executing from the stack.
-Maynard
From: Carl Love <hidden> Date: 2007-01-30 23:31:13
On Tue, 2007-01-30 at 15:41 -0600, Maynard Johnson wrote:
[snip
[snip]
quoted
quoted
+ // process the collected SPU PC for each node
+ for_each_online_cpu(cpu) {
+ if (cbe_get_hw_thread_id(cpu))
+ continue;
+
+ node = cbe_cpu_to_node(cpu);
+ node_factor = node * SPUS_PER_NODE;
+ /* number of valid entries for this node */
+ entry = 0;
+
+ trace_addr = cbe_read_pm(cpu, trace_address);
+ while ((trace_addr & CBE_PM_TRACE_BUF_EMPTY) != 0x400)
+ {
+ /* there is data in the trace buffer to process */
+ cbe_read_trace_buffer(cpu, trace_buffer);
+ spu_mask = 0xFFFF000000000000;
+
+ /* Each SPU PC is 16 bits; hence, four spus in each of
+ * the two 64-bit buffer entries that make up the
+ * 128-bit trace_buffer entry. Process the upper and
+ * lower 64-bit values simultaneously.
+ */
+ for (spu = 0; spu < SPUS_PER_TB_ENTRY; spu++) {
+ spu_pc_lower = spu_mask & trace_buffer[0];
+ spu_pc_lower = spu_pc_lower >> (NUM_SPU_BITS_TRBUF
+ * (SPUS_PER_TB_ENTRY-spu-1));
+
+ spu_pc_upper = spu_mask & trace_buffer[1];
+ spu_pc_upper = spu_pc_upper >> (NUM_SPU_BITS_TRBUF
+ * (SPUS_PER_TB_ENTRY-spu-1));
+
+ spu_mask = spu_mask >> NUM_SPU_BITS_TRBUF;
+
+ /* spu PC trace entry is upper 16 bits of the
+ * 18 bit SPU program counter
+ */
+ spu_pc_lower = spu_pc_lower << 2;
+ spu_pc_upper = spu_pc_upper << 2;
+
+ samples[((node_factor + spu) * TRACE_ARRAY_SIZE) + entry]
+ = (u32) spu_pc_lower;
+ samples[((node_factor + spu + SPUS_PER_TB_ENTRY) * TRACE_ARRAY_SIZE) + entry]
+ = (u32) spu_pc_upper;
+ }
+
+ entry++;
+
+ if (entry >= TRACE_ARRAY_SIZE)
+ /* spu_samples is full */
+ break;
+
+ trace_addr = cbe_read_pm(cpu, trace_address);
+ }
+ samples_per_node[node] = entry;
+ }
+}
While I can't see anything technically wrong with this function, it would be
good to split it into smaller functions. Since you are nesting three
loops, it should be possible to make a separate function from one of the
inner loops without changing the actual logic behind it.
Will do.
quoted
quoted
+
+static int profile_spus(struct hrtimer * timer)
+{
+ ktime_t kt;
+ int cpu, node, k, num_samples, spu_num;
whitespace damage
fixed
quoted
quoted
+
+ if (!spu_prof_running)
+ goto STOP;
+
+ cell_spu_pc_collection();
+ for_each_online_cpu(cpu) {
+ if (cbe_get_hw_thread_id(cpu))
+ continue;
Here, you enter the same top-level loop again, why not make it
for_each_online_cpu(cpu) {
if (cbe_get_hw_thread_id(cpu))
continue;
num_samples = cell_spu_pc_collection(cpu);
...
Yes, good suggestion.
I believe what you are asking here is why can't the
cell_spu_pc_collection() function put the data in to the samples array
for a given node, in the loop that then processes the samples array for
that node. Yes, I believe that this can be done. The only restriction
is that cell_spu_pc_collection() will have to extract the SPU program
counter data for all SPUs on that node. This is due to the fact the
data for the 8 SPUs are all stored in a single entry of the hardware
trace buffer. Once the hardware trace buffer is read, the hardware
advances the read pointer so there is no way to go back and re-read the
entry.
[snip]
[snip]
I don't have the slightest idea what this code is about, but
Me neither. Carl, can you comment?
quoted
it certainly looks inefficient to loop 16 million times to
compute a constant. Could you use a faster algorithm instead,
or at least add a comment about why you do it this way?
An LFSR sequence is similar to a pseudo random number sequence. For a 24
bit LFSR sequence each number between 0 and 2^24 will occur once in the
sequence but not in a normal counting order. The hardware uses the LFSR
sequence to count to since it is much simpler to implement in hardware
then a normal counter. Unfortunately, the only way we know how to
figure out what the LFSR value that corresponds to the number in the
sequence that is N before the last value (0xFFFFFF) is to calculate the
previous value N times. It is like trying to ask what is the pseudo
random number that is N before this pseudo random number?
I will add a short comment to the code that will summarize the above
paragraph.
[snip]
From: Christian Krafft <hidden> Date: 2007-01-31 01:50:50
On Tue, 30 Jan 2007 15:31:09 -0800
Carl Love [off-list ref] wrote:
An LFSR sequence is similar to a pseudo random number sequence. For a 24
bit LFSR sequence each number between 0 and 2^24 will occur once in the
sequence but not in a normal counting order. The hardware uses the LFSR
sequence to count to since it is much simpler to implement in hardware
then a normal counter. Unfortunately, the only way we know how to
figure out what the LFSR value that corresponds to the number in the
sequence that is N before the last value (0xFFFFFF) is to calculate the
previous value N times. It is like trying to ask what is the pseudo
random number that is N before this pseudo random number?
That should be no problem.=20
You can just revers your algorithm and let it run x times instead of 0xFFFF=
FF-x.
=20
I will add a short comment to the code that will summarize the above
paragraph.
[snip]
=20
_______________________________________________
cbe-oss-dev mailing list
cbe-oss-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/cbe-oss-dev
--=20
Mit freundlichen Gr=FCssen,
kind regards,
Christian Krafft
IBM Systems & Technology Group,=20
Linux Kernel Development
IT Specialist
On Wednesday 31 January 2007 00:31, Carl Love wrote:
Unfortunately, the only way we know how to
figure out what the LFSR value that corresponds to the number in the
sequence that is N before the last value (0xFFFFFF) is to calculate the
previous value N times. =A0It is like trying to ask what is the pseudo
random number that is N before this pseudo random number?
Well, you can at least implement the lfsr both ways, and choose the one
that is faster to get at, like
u32 get_lfsr(u32 v)
{
int i;
u32 r =3D 0xffffff;
if (v < 0x7fffff) {
for (i =3D 0; i < v; i++)
r =3D lfsr_forwards(r);
} else {
for (i =3D 0; i < (0x1000000 - v); i++)
r =3D lfsr_backwards(r);
}
return r;
}
Also, if the value doesn't have to be really exact, you could have
a small lookup table with precomputed values, like:
u32 get_lfsr(u32 v)
{
static const lookup[256] =3D {
0xab3492, 0x3e3f34, 0xc47610c, ... /* insert actual values */
};
return lookup[v >> 16];
}
Arnd <><
is hrtimer_forward really the right interface here? You are ignoring
the number of overruns anyway, so hrtimer_start(,,) sounds more
correct to me.
According to Tom Gleixner, "hrtimer_forward is a convenience function to
move the expiry time of a timer forward in multiples of the interval, so
it is in the future. After setting the expiry time you restart the
timer either with [sic] a return HRTIMER_RESTART (if you are in the
timer callback function)."
quoted
Ok, I see. Have you seen the timer actually coming in late, resulting
in hrtimer_forward returning non-zero? I guess it's not a big problem
for statistic data collection if that happens, but you might still want
to be able to see it.
quoted
quoted
+ /* Since cpufreq_quick_get returns frequency in kHz, we use
+ * USEC_PER_SEC here vs NSEC_PER_SEC.
+ */
+ unsigned long nsPerCyc = (USEC_PER_SEC << SCALE_SHIFT)/khzfreq;
+ profiling_interval = (nsPerCyc * cycles_reset) >> SCALE_SHIFT;
+
+ pr_debug("timer resolution: %lu\n",
+ TICK_NSEC);
Don't you need to adapt the profiling_interval at run time, when cpufreq
changes the core frequency? You should probably use
cpufreq_register_notifier() to update this.
Since OProfile is a statistical profiler, the exact frequency is not
critical. The user is going to be looking for hot spots in their code,
so it's all relative. With that said, I don't imagine using the
cpufreq notiication would be a big deal. We'll look at it.
quoted
quoted
@@ -480,7 +491,22 @@ struct op_system_config *sys, int num_ctrs)
{
int i, j, cpu;
+ spu_cycle_reset = 0;
+ /* The cpufreq_quick_get function requires that cbe_cpufreq module
+ * be loaded. This function is not actually provided and exported
+ * by cbe_cpufreq, but it relies on cbe_cpufreq initialize kernel
+ * data structures. Since there's no way for depmod to realize
+ * that our OProfile module depends on cbe_cpufreq, we currently
+ * are letting the userspace tool, opcontrol, ensure that the
+ * cbe_cpufreq module is loaded.
+ */
+ khzfreq = cpufreq_quick_get(smp_processor_id());
You should probably have a fallback in here in case the cpufreq module
is not loaded. There is a global variable ppc_proc_freq (in Hz) that
you can access.
Our userspace tool ensures the cpufreq module is loaded.
You should not rely on user space tools to do the right thing in the kernel.
Moreover, if the exact frequency is not that important, as you mentioned
above, you can probably just hardcode a compile-time constant here.
quoted
quoted
+ *
+ * Ideally, we would like to be able to create the cached_info for
+ * an SPU task just one time -- when libspe first loads the SPU
+ * binary file. We would store the cached_info in a list. Then, as
+ * SPU tasks are switched out and new ones switched in, the cached_info
+ * for inactive tasks would be kept, and the active one would be placed
+ * at the head of the list. But this technique may not with
+ * current spufs functionality since the spu used in bind_context may
+ * be a different spu than was used in a previous bind_context for a
+ * reactivated SPU task. Additionally, a reactivated SPU task may be
+ * assigned to run on a different physical SPE. We will investigate
+ * further if this can be done.
+ *
+ */
You should stuff a pointer to cached_info into struct spu_context,
e.g. 'void *profile_private'.
And replace the 'the_spu' member with a back pointer to the
spu_context if you need it.
quoted
+
+/* A data structure for cached information about active SPU tasks.
+ * Storage is dynamically allocated, sized as
+ * "number of active nodes multplied by 8".
+ * The info_list[n] member holds 0 or more
+ * 'struct cached_info' objects for SPU#=n.
+ *
+ * As currently implemented, there will only ever be one cached_info
+ * in the list for a given SPU. If we can devise a way to maintain
+ * multiple cached_infos in our list, then it would make sense
+ * to also cache the dcookie representing the PPU task application.
+ * See above description of struct cached_info for more details.
+ */
+struct spu_info_stacks {
+ struct list_head * info_list;
+};
Why do you store pointers to list_head structures? If you want to store
lists, you should have a lists_head itself in here.
info_list is an array of n lists, where n is the number of SPUs.
My point was that it's not an array of lists, but an array of pointers
to lists. The way that include/list.h works is by having a struct list_head
as the anchor and then add nodes to it. By simply pointing to a list_head,
you won't be able to use the list_for_each_entry() macro the way it is meant.
quoted
I don't get it. What is the app_dcookie used for? If the spu binary is
embedded into a library, you are still missing the dcookie to that .so file,
because you return only an offset.
For embedded SPU app, the post-processing tool opens the PPE binary app
file and obtains the SPU ELF embedded thereine, and from that, we obtain
the name of the SPU binary. Also, the app name is included in the
report, along with the SPU binary filename, if the report contains
samples from more than one application.
That's not what I meant. If you have an embedded spu ELF file in a shared
library object, you end up recording the file of the main PPE binary, and
the offset of the SPU ELF inside of the .so file, but not the name of the
.so file itself.
You also say that the you record the name of the application on purpose for
separate SPU ELF files. My assumption was that this is not necessary, but
I don't know much about that aspect of oprofile. My feeling is that the
samples for an external SPU ELF file should be handled the same way that
oprofile handles events in shared libraries on the PPU (e.g. in libc.so).
Arnd <><
is hrtimer_forward really the right interface here? You are ignoring
the number of overruns anyway, so hrtimer_start(,,) sounds more
correct to me.
According to Tom Gleixner, "hrtimer_forward is a convenience function to
move the expiry time of a timer forward in multiples of the interval, so
it is in the future. After setting the expiry time you restart the
timer either with [sic] a return HRTIMER_RESTART (if you are in the
timer callback function)."
Ok, I see. Have you seen the timer actually coming in late, resulting
in hrtimer_forward returning non-zero? I guess it's not a big problem
for statistic data collection if that happens, but you might still want
to be able to see it.
I don't think there's much point in knowing if we have overrun(s).
We're not going to schedule the timer any differently. We want to keep
the timer interrupts as consistent as possible according to the user's
request.
quoted
quoted
quoted
+ /* Since cpufreq_quick_get returns frequency in kHz, we use
+ * USEC_PER_SEC here vs NSEC_PER_SEC.
+ */
+ unsigned long nsPerCyc = (USEC_PER_SEC << SCALE_SHIFT)/khzfreq;
+ profiling_interval = (nsPerCyc * cycles_reset) >> SCALE_SHIFT;
+
+ pr_debug("timer resolution: %lu\n",
+ TICK_NSEC);
Don't you need to adapt the profiling_interval at run time, when cpufreq
changes the core frequency? You should probably use
cpufreq_register_notifier() to update this.
Since OProfile is a statistical profiler, the exact frequency is not
critical. The user is going to be looking for hot spots in their code,
so it's all relative. With that said, I don't imagine using the
cpufreq notiication would be a big deal. We'll look at it.
quoted
quoted
@@ -480,7 +491,22 @@ struct op_system_config *sys, int num_ctrs)
{
int i, j, cpu;
+ spu_cycle_reset = 0;
+ /* The cpufreq_quick_get function requires that cbe_cpufreq module
+ * be loaded. This function is not actually provided and exported
+ * by cbe_cpufreq, but it relies on cbe_cpufreq initialize kernel
+ * data structures. Since there's no way for depmod to realize
+ * that our OProfile module depends on cbe_cpufreq, we currently
+ * are letting the userspace tool, opcontrol, ensure that the
+ * cbe_cpufreq module is loaded.
+ */
+ khzfreq = cpufreq_quick_get(smp_processor_id());
You should probably have a fallback in here in case the cpufreq module
is not loaded. There is a global variable ppc_proc_freq (in Hz) that
you can access.
Our userspace tool ensures the cpufreq module is loaded.
You should not rely on user space tools to do the right thing in the kernel.
Ok, we'll look at the fallback option you suggest. I don't recall if I
even knew about ppc_proc_freq before or why I originally chose
cpufreq_guick_get. Maybe we can do without the cpufreq and use
ppc_proc_freq all the time. We'll see . . .
Moreover, if the exact frequency is not that important, as you mentioned
above, you can probably just hardcode a compile-time constant here.
Well, exact frequency isn't critical, but it should, as close as
possible, correspond with the user's requested value for "spu cycle reset".
quoted
quoted
quoted
+ *
+ * Ideally, we would like to be able to create the cached_info for
+ * an SPU task just one time -- when libspe first loads the SPU
+ * binary file. We would store the cached_info in a list. Then, as
+ * SPU tasks are switched out and new ones switched in, the cached_info
+ * for inactive tasks would be kept, and the active one would be placed
+ * at the head of the list. But this technique may not with
+ * current spufs functionality since the spu used in bind_context may
+ * be a different spu than was used in a previous bind_context for a
+ * reactivated SPU task. Additionally, a reactivated SPU task may be
+ * assigned to run on a different physical SPE. We will investigate
+ * further if this can be done.
+ *
+ */
You should stuff a pointer to cached_info into struct spu_context,
e.g. 'void *profile_private'.
And replace the 'the_spu' member with a back pointer to the
spu_context if you need it.
quoted
+
+/* A data structure for cached information about active SPU tasks.
+ * Storage is dynamically allocated, sized as
+ * "number of active nodes multplied by 8".
+ * The info_list[n] member holds 0 or more
+ * 'struct cached_info' objects for SPU#=n.
+ *
+ * As currently implemented, there will only ever be one cached_info
+ * in the list for a given SPU. If we can devise a way to maintain
+ * multiple cached_infos in our list, then it would make sense
+ * to also cache the dcookie representing the PPU task application.
+ * See above description of struct cached_info for more details.
+ */
+struct spu_info_stacks {
+ struct list_head * info_list;
+};
Why do you store pointers to list_head structures? If you want to store
lists, you should have a lists_head itself in here.
info_list is an array of n lists, where n is the number of SPUs.
My point was that it's not an array of lists, but an array of pointers
to lists. The way that include/list.h works is by having a struct list_head
as the anchor and then add nodes to it. By simply pointing to a list_head,
you won't be able to use the list_for_each_entry() macro the way it is meant.
I've got to rework this implementation anyway . . .
From: Maynard Johnson <hidden> Date: 2007-02-03 23:48:59
Arnd Bergmann wrote:
On Monday 29 January 2007 20:48, Maynard Johnson wrote:
quoted
Subject: Add support to OProfile for profiling Cell BE SPUs
[snip]
quoted
+ *
+ * Ideally, we would like to be able to create the cached_info for
+ * an SPU task just one time -- when libspe first loads the SPU
+ * binary file. We would store the cached_info in a list. Then, as
+ * SPU tasks are switched out and new ones switched in, the cached_info
+ * for inactive tasks would be kept, and the active one would be placed
+ * at the head of the list. But this technique may not with
+ * current spufs functionality since the spu used in bind_context may
+ * be a different spu than was used in a previous bind_context for a
+ * reactivated SPU task. Additionally, a reactivated SPU task may be
+ * assigned to run on a different physical SPE. We will investigate
+ * further if this can be done.
+ *
+ */
You should stuff a pointer to cached_info into struct spu_context,
e.g. 'void *profile_private'.
I seem to recall looking at this option a while back, but didn't go that
route since struct spu_context is opaque to me. With such a teqnique, I
could then use a simple 16-element array of pointers to cached_info
objects, creating them as needed when spu_context->profile_private is
NULL. I suppose the better option for now is to add a
get_profile_private() function to SPUFs, rather than requiring
spu_context to be visible. Don't know why I didn't think to do that
before. Ah, well, live and learn.
-Maynard
On Sunday 04 February 2007 00:49, Maynard Johnson wrote:
I seem to recall looking at this option a while back, but didn't go that=
=20
route since struct spu_context is opaque to me. =A0With such a teqnique, =
I=20
could then use a simple 16-element array of =A0pointers to cached_info=20
objects, creating them as needed when spu_context->profile_private is=20
NULL. =A0I suppose the better option for now is to add a=20
get_profile_private() function to SPUFs, rather than requiring=20
spu_context to be visible.
Yes, that sounds good. Note that the file providing the=20
spufs_get_profile_private (and respective spufs_set_profile_private)
functions needs to be compiled into the kernel then in case oprofile
gets linked in but spufs is a module.
I think it would also be necessary to have another interface for cleaning
up this data when spufs destroys the context. That could possibly
a variation of the existing notifier call, or a new call, or you
establish the convention that if the private pointer is non-NULL,
spufs will kfree it.
Arnd <><
From: Maynard Johnson <hidden> Date: 2007-02-04 17:33:45
Arnd Bergmann wrote:
On Sunday 04 February 2007 00:49, Maynard Johnson wrote:
quoted
I seem to recall looking at this option a while back, but didn't go that
route since struct spu_context is opaque to me. With such a teqnique, I
could then use a simple 16-element array of pointers to cached_info
objects, creating them as needed when spu_context->profile_private is
NULL. I suppose the better option for now is to add a
get_profile_private() function to SPUFs, rather than requiring
spu_context to be visible.
Yes, that sounds good. Note that the file providing the
spufs_get_profile_private (and respective spufs_set_profile_private)
functions needs to be compiled into the kernel then in case oprofile
gets linked in but spufs is a module.
Hmm . . . we already depend on the register/unregister functions in
sched.c, so my patch changes the oprofile Kconfig to default to 'm' and
'depends on SPU_FS'.
I think it would also be necessary to have another interface for cleaning
up this data when spufs destroys the context. That could possibly
a variation of the existing notifier call, or a new call, or you
establish the convention that if the private pointer is non-NULL,
spufs will kfree it.
Yes, I was thnking along the lines of your last suggestion. I presume
OProfile gets notified (object_id == 0) before the context is actually
destroyed. At that time, we would NULL-out the reference to the
cached_info, so then SPUFS would kfree it at destroy time.
-Maynard
From: Milton Miller <hidden> Date: 2007-01-31 09:24:56
I've actually read most of the replys. Hopefully I included enough
headers to get my mailer to put this in the right thread.
Sorry if I missed someone on cc, the mail archives don't
give one that info.
On Jan 30, 2007, at 5:48 AM, Maynard Johnson wrote:
quoted hunk
Subject: Add support to OProfile for profiling Cell BE SPUs
From: Maynard Johnson <redacted>
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
code.
Signed-off-by: Carl Love <redacted>
Signed-off-by: Maynard Johnson <redacted>
Index: linux-2.6.20-rc1/arch/powerpc/oprofile/cell/pr_util.h
===================================================================
+typedef struct vma_map
+{
+ struct vma_map *next;
+ unsigned int vma;
+ unsigned int size;
+ unsigned int offset;
+ unsigned int guard_ptr;
+ unsigned int guard_val;
+} vma_map_t;
+
+/* The three functions below are for maintaining and accessing
+ * the vma-to-file offset map.
+ */
+vma_map_t * create_vma_map(const struct spu * spu, u64 objectid);
+unsigned int vma_map_lookup(vma_map_t *map, unsigned int vma,
+ const struct spu * aSpu);
+void vma_map_free(struct vma_map *map);
+
Why would the SPU to cookie translation need to be different
than the standard vm one? Is it that spufs takes refs on the
pages but doesn't have the standard vma? Maybe an approach
of creating them would reuse the oprofile code.
+#define TRACE_ARRAY_SIZE 1024
+static u32 * samples;
+static u32 * samples_per_node;
+
+static int spu_prof_running = 0;
+static unsigned int profiling_interval = 0;
+
+extern int num_nodes;
+extern unsigned int khzfreq;
+
+/*
+ * Oprofile setup functions
+ */
+
+#define NUM_SPU_BITS_TRBUF 16
+#define SPUS_PER_TB_ENTRY 4
+#define SPUS_PER_NODE 8
+
+/*
+ * Collect the SPU program counter samples from the trace buffer.
+ * The global variable usage is as follows:
+ * samples[<total-spus>][TRACE_ARRAY_SIZE] - array to store SPU PC
samples
+ * Assumption, the array will be all zeros on entry.
+ * u32 samples_per_node[num_nodes] - array of how many valid
samples per node
+ */
+static void cell_spu_pc_collection(void)
+{
+ int cpu;
+ int node;
+ int spu;
+ u32 trace_addr;
+ /* the trace buffer is 128 bits */
+ u64 trace_buffer[2];
+ u64 spu_pc_lower;
+ u64 spu_pc_upper;
+ u64 spu_mask;
+ int entry, node_factor;
+ // process the collected SPU PC for each node
+ for_each_online_cpu(cpu) {
+ if (cbe_get_hw_thread_id(cpu))
+ continue;
+
+ node = cbe_cpu_to_node(cpu);
+ node_factor = node * SPUS_PER_NODE;
+ /* number of valid entries for this node */
+ entry = 0;
+
+ trace_addr = cbe_read_pm(cpu, trace_address);
+ while ((trace_addr & CBE_PM_TRACE_BUF_EMPTY) != 0x400)
+ {
+ /* there is data in the trace buffer to process */
+ cbe_read_trace_buffer(cpu, trace_buffer);
+ spu_mask = 0xFFFF000000000000;
+
+ /* Each SPU PC is 16 bits; hence, four spus in each of
+ * the two 64-bit buffer entries that make up the
+ * 128-bit trace_buffer entry. Process the upper and
+ * lower 64-bit values simultaneously.
+ */
+ for (spu = 0; spu < SPUS_PER_TB_ENTRY; spu++) {
+ spu_pc_lower = spu_mask & trace_buffer[0];
+ spu_pc_lower = spu_pc_lower >> (NUM_SPU_BITS_TRBUF
+ * (SPUS_PER_TB_ENTRY-spu-1));
Calculating the shift each time through the loop has to be inefficient.
As mentioned by others, I would suggest making this loop split
the value into the 4 parts. It would probably be better to
shift the raw data to the left each pass, and then always take
the top 16 bits and shift them down low.
looks more like a for loop to me ... its
for (a=func; a & bit, a = func)
Actually, i'd probably change this to
for (entry = 0; entry < trace size; entry++)
{
a = read_trace_data(x);
if (trace_not_valid)
break;
spilt sample per notes aove;
}
Rational: the stop for entrys is absolute
you are processing data to fill, which eliminates
duplicate obtain_data code.
Its obvious you should stop if you are full
(aside) the timer should be set to avoid full,
but delays could cause it to fill. Perhaps
if we fill we should slightly reduce the timer
period?
+ samples_per_node[node] = entry;
+ }
+}
+
+
+/*
+ * Entry point for SPU profiling.
+ * NOTE: SPU profiling is done system-wide, not per-CPU.
+ *
+ * cycles_reset is the count value specified by the user when
+ * setting up OProfile to count SPU_CYCLES.
+ */
+void start_spu_profiling(unsigned int cycles_reset) {
+
+ ktime_t kt;
+
+ /* To calculate a timeout in nanoseconds, the basic
+ * formula is ns = cycles_reset * (NSEC_PER_SEC / cpu frequency).
+ * To avoid floating point math, we use the scale math
+ * technique as described in linux/jiffies.h. We use
+ * a scale factor of SCALE_SHIFT,which provides 4 decimal places
+ * of precision, which is close enough for the purpose at hand.
+ */
+
+ /* Since cpufreq_quick_get returns frequency in kHz, we use
+ * USEC_PER_SEC here vs NSEC_PER_SEC.
+ */
+ unsigned long nsPerCyc = (USEC_PER_SEC << SCALE_SHIFT)/khzfreq;
+ profiling_interval = (nsPerCyc * cycles_reset) >> SCALE_SHIFT;
+
+ pr_debug("timer resolution: %lu\n",
+ TICK_NSEC);
+ kt = ktime_set(0, profiling_interval);
+ hrtimer_init(&timer, CLOCK_MONOTONIC, HRTIMER_REL);
+ timer.expires = kt;
+ timer.function = profile_spus;
+
+ /* Allocate arrays for collecting SPU PC samples */
+ samples = (u32 *) kzalloc(num_nodes * SPUS_PER_NODE *
TRACE_ARRAY_SIZE * sizeof(u32), GFP_ATOMIC);
+ samples_per_node = (u32 *) kzalloc(num_nodes * sizeof(u32),
GFP_ATOMIC);
+
+ spu_prof_running = 1;
+ hrtimer_start(&timer, kt, HRTIMER_REL);
+}
So we are doing all this calculations, including adding cpufreq
dependencys, just to caculate once how often to dump the
trace array once?
And then we have all the convert-to-lfsr in the kernel, which
as people noticed will run up to 2^24 loops of bit manuplitions
as specified by the user ....
I would propose instead the user space code passes (1) the
requested dump interval and (2) the computed lfsr value to
load.
The kernel can impose sanity checks on the interrupt rate.
The invalid lfsr value is known (0), so that means that
that the worse that could happen is (1) user space programs
too small of a count, and we don't dump the trace competely.
or (2) we dump more often that we need to. If we want to
correct or tune, we (1) can be detected and the kernel
gradually reduce the poll interval; (2) can be detected by
empty trace arrays on timer with no intervening context switch
dump; and only results in interrupt overhead wich can be
limited by sanity checks (bounds testing).
This would also allow a path in the future for userspace to
change the requested sample rates ....
milton
On Wednesday 31 January 2007 10:24, Milton Miller wrote:
quoted
+/* The three functions below are for maintaining and accessing
+ * the vma-to-file offset map.
+ */
+vma_map_t * create_vma_map(const struct spu * spu, u64 objectid);
+unsigned int vma_map_lookup(vma_map_t *map, unsigned int vma,
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =
=A0const struct spu * aSpu);
quoted
+void vma_map_free(struct vma_map *map);
+
=20
Why would the SPU to cookie translation need to be different
than the standard vm one? =A0 Is it that spufs takes refs on the
pages but doesn't have the standard vma? =A0 Maybe an approach
of creating them would reuse the oprofile code.
It's a two stage process for SPUs:
* samples are relative to the local store, and for each sample
we need to know what context was running. The context is
identified over a pointer to user space effective addresses.
* The effective address identifies the SPU ELF binary mapped
at that address. It may however be in the middle of a VMA,
so you get another offset into the mapped file.
=46or each sample, you then get an offset into the ls, an offset
into the file to identify the ELF object, and the dcookie
for the file containing that object.
As a consequence, you only need dcookies for the case where
a context switch happens (the executable changes), but not
for each of the samples during the a time slice, they all
point to the same object.
Arnd <><
From: Milton Miller <hidden> Date: 2007-02-01 18:56:46
On Jan 31, 2007, at 9:42 AM, Arnd Bergmann wrote:
On Wednesday 31 January 2007 10:24, Milton Miller wrote:
quoted
quoted
+/* The three functions below are for maintaining and accessing
+ * the vma-to-file offset map.
+ */
+vma_map_t * create_vma_map(const struct spu * spu, u64 objectid);
+unsigned int vma_map_lookup(vma_map_t *map, unsigned int vma,
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =A0 =
=A0const struct spu * aSpu);
quoted
quoted
+void vma_map_free(struct vma_map *map);
+
Why would the SPU to cookie translation need to be different
than the standard vm one? =A0 Is it that spufs takes refs on the
pages but doesn't have the standard vma? =A0 Maybe an approach
of creating them would reuse the oprofile code.
It's a two stage process for SPUs:
* samples are relative to the local store, and for each sample
we need to know what context was running. The context is
identified over a pointer to user space effective addresses.
Ok I'm with you so far. There is a separate address space, the
local store, which you abbreviate ls below. The trace gives
this local address.
* The effective address identifies the SPU ELF binary mapped
at that address. It may however be in the middle of a VMA,
so you get another offset into the mapped file.
I think this is where we start to diverge.
Can you only map 1 linear range of 1 file as the SPU local store?
I thought you were mentioning shared libraries, and had mmap,
shared mappings of text, etc.
For each sample, you then get an offset into the ls, an offset
into the file to identify the ELF object, and the dcookie
for the file containing that object
You are supplying
(1) offset in local store
(2) offset from dcookie to local store (? you said ELF object)
(3) file containing #2
So there is exactly one backing object for the entire local
store, and its mapped linearly?
As a consequence, you only need dcookies for the case where
a context switch happens (the executable changes), but not
for each of the samples during the a time slice, they all
point to the same object.
My understanding is the dcookie is supposed to represent a backing
file object. The combination (dcookie, offset) should lead to
the backing object, which userspace can then disassemble, etc.
Therefore, if you allow multiple pieces to be mapped into local store,
then you should be reverse translating each ls address into (file,=20
offset)
of that file system object. The fact that its contained in a bigger
elf file may mean that userspace needs some more info, but it still
needs the same info.
If you do allow more than 1 backing object, then my suggestion was
to use the common code by setting up a fake vm context that has
kernel vmas and let the generic code lookup the file from this context.
milton=
On Thursday 01 February 2007 19:56, Milton Miller wrote:
quoted
* The effective address identifies the SPU ELF binary mapped
=A0 at that address. It may however be in the middle of a VMA,
=A0 so you get another offset into the mapped file.
=20
I think this is where we start to diverge.
=20
Can you only map 1 linear range of 1 file as the SPU local store?
=20
I thought you were mentioning shared libraries, and had mmap,
shared mappings of text, etc.
There are two address spaces in the SPU. Code and local variables
are all in the local store (256kb), so the samples that oprofile
takes come from there.=20
The other address space is the DMA space, which is defined by
the mm_struct of the process, and established on the PPU (the
linux program). Shared libraries and mappings are all in this
space.
To load an SPU program, the SPU ELF file is mapped into the
process address space (DMA space) and the ELF loader transfers
sections from it to the local store of the SPU. Consequently,
there is a unique translation from every local store address
to a file offset in the original ELF file, but there can
be multiple linear ranges.
quoted
For each sample, you then get an offset into the ls, an offset
into the file to identify the ELF object, and the dcookie
for the file containing that object
=20
You are supplying
=A0 =A0 (1) offset in local store
=A0 =A0 (2) offset from dcookie to local store (? =A0 you said ELF object)
=A0 =A0 (3) file containing #2
=20
So there is exactly one backing object for the entire local
store, and its mapped linearly?
1 object, but not linearly. To make things worse, there can
also be overlays (think turbo pascal on MS-DOS), so the
object can be larger than the actual local store, and parts
of it get loaded on demand.
Also, a file can contain multiple SPU ELF binaries, we have
the embedspu tool that encapsulates a statically linked
SPU program into a powerpc object containing a single
symbol. You can then link multiple such objects into a
shared library or your main powerpc-side application.
quoted
As a consequence, you only need dcookies for the case where
a context switch happens (the executable changes), but not
for each of the samples during the a time slice, they all
point to the same object.
=20
=20
My understanding is the dcookie is supposed to represent a backing
file object. =A0The combination (dcookie, offset) should lead to
the backing object, which userspace can then disassemble, etc.
The (dcookie, offset) tuple first identifies the SPU ELF binary,
offset here being the file offset where the actual ELF image
starts. You need the second offset to identify the pointer in there.
Therefore, if you allow multiple pieces to be mapped into local store,
then you should be reverse translating each ls address into (file,=20
offset)
of that file system object. =A0 The fact that its contained in a bigger
elf file may mean that userspace needs some more info, but it still
needs the same info.
At the minimum, userspaces needs information like
* spu3 is <dcookie=3D87656323, offset=3D4567> /* first program gets loaded =
*/
* sample at <spu=3D3, offset=3D1234> /* samples */
* sample at <spu=3D3, offset=3D1248>
* sample at <spu=3D3, offset=3D160c>
* spu3 is <dcookie=3D34563287, offset=3D5476> /* context switch to other */
* sample at <spu=3D3, offset=3D5a32> /* samples in new program */
* sample at <spu=3D3, offset=3D7231>
The same can be expressed as
* sample at <dcookie=3D87656323, foffset=3D4567, offset=3D1234> /* samples =
*/
* sample at <dcookie=3D87656323, foffset=3D4567, offset=3D1248>
* sample at <dcookie=3D87656323, foffset=3D4567, offset=3D160c>
* sample at <dcookie=3D34563287, foffset=3D5476, offset=3D5a32> /* samples =
in new program */
* sample at <dcookie=3D34563287, foffset=3D5476, offset=3D7231>
And potentially other information per sample.
If you do allow more than 1 backing object, then my suggestion was
to use the common code by setting up a fake vm context that has
kernel vmas and let the generic code lookup the file from this context.
The problem of more than one backing object is only if you look
the samples being collected per physical SPU, because that means
it will see context switches. If the samples get collected per
context, there is only one backing object.
Arnd <><
On Monday 29 January 2007 20:45, Maynard Johnson wrote:
On December 14, 2006, I posted a patch that added support to the=20
OProfile kernel driver for profiling Cell SPUs. =A0There have been some=20
changes/fixes to this patch since the original posting (including=20
forward porting from 2.6.18-based kernel to 2.6.20-rc1), so I am=20
reposting the patch for review now. =A0This patch relies upon the=20
following patches that have not been accepted yet:
=A0 =A01. oprofile cleanup patch (submitted on Nov 27)
=A0 =A02. Fix for PPU profiling (not submitted yet, since it depends on #=
1)
=A0 =A03. SPU task notification patch (last submitted on Jan 26)
=20
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 <><