Re: [PATCH 1/4 -v2] perf: rework the whole read vs group stuff

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Fri, 2009-08-21 at 16:47 +0200, stephane eranian wrote:
> Hi,
> 
> Well, I tried that and it brings more complexity than needed
> especially with regards to
> extracting the ID for events when you're doing grouping.
> 
> To extract the ID, one has to read out a struct as follows:
>  *      { u64           nr;
>  *        { u64         time_enabled; } && PERF_FORMAT_ENABLED
>  *        { u64         time_running; } && PERF_FORMAT_RUNNING
>  *        { u64         value;
>  *          { u64       id;           } && PERF_FORMAT_ID
>  *        }             cntr[nr];
>  *      } && PERF_FORMAT_GROUP
> 
> Supposedly, you should have to do this only once per group. Reading
> this stuff using the group leader should yield the values of all the other
> events. This is not what I have observed. All events report the same
> ID as the leader. Not clear why.

Hmm, that's a bug alright.

> As I suggested in a previous message, I don't think all of this is necessary.
> If a tool was able to pass the ID to associate with an event, then there
> would be no need for a read(). Furthermore, it would make it easier to pick
> an ID which suites the tool's data structure. For instance, if you create
> 4 events in a group, the ID could be 0,1,2,3 and would most likely map to
> an index in the array used by the tool to manage the perf_counter structures.
> That would also make it easier in the critical path in the signal handler. No
> need to have a lookup table to map "random" ID to ID more relevant
> for the tool. The ID does not need to be very wide. IDs are relevant only if
> one uses group sampling. Therefore the ID needs to identify an event within
> a group. Could use a reserved field in perf_counter_attr or add an ioctl() to
> assign an ID.

ID are also needed when you want to allow mixing of the output buffers
(in any sense). That is, perf writes all mmap() data into a single file,
which mixes the streams, the other example is the output redirection
patch I posted the other day which basically does the same in-kernel.

A user-provided ID is still possible by extending perf_counter_attr. We
could add this later.

We still need a per-counter unique ID for the inherited thing, and I was
worrying that perhaps we would need to worry about collisions, but I
think these id spaces can be considered separate.

---
Subject: perf_counter: Fix typo in read() output generation

When you iterate a list, using the iterator is useful.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
 kernel/perf_counter.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kernel/perf_counter.c b/kernel/perf_counter.c
index 36f65e2..f274e19 100644
--- a/kernel/perf_counter.c
+++ b/kernel/perf_counter.c
@@ -1791,7 +1791,7 @@ static int perf_counter_read_group(struct perf_counter *counter,
 	size += err;
 
 	list_for_each_entry(sub, &leader->sibling_list, list_entry) {
-		err = perf_counter_read_entry(counter, read_format,
+		err = perf_counter_read_entry(sub, read_format,
 				buf + size);
 		if (err < 0)
 			return err;