Re: [PATCH] perf timechart: remove lock_depth from trace_entry

[Arnaldo Carvalho de Melo]

Em Tue, Nov 26, 2013 at 05:47:55PM +0400, Stanislav Fomichev escreveu:
> > Se above, if before this patch the format_field info was obtained from
> > the perf.data file, why should it now get it from the local machine?

> Yes, sorry for confusion, I didn't know that we can obtain trace events
> format from the perf.data. I hence have no concerns regarding your
> implementation :-)
> 
> > Please try. There should be no limitations.

> It does not work because lock_depth and common_pid are common fields,
> while perf_evsel__field looks in non-common fields. And even if we find

Oh, I thought they were linked, my bad, so that makes the implementation
even easier :-)

> lock_depth/common_pid common field we can't do field->next, because
> common and non-common fields are not linked and ->next in the last common
> field is NULL (this is what I got when I tried to use pevent_find_any_field
> instead of pevent_find_field in perf_evsel__field).
> 
> I slightly modified timechart__set_payload_offset from you patch to look
> for the first non-common field and use its offset, but it looks kinda ugly:
> 
> static int timechart__set_payload_offset(struct perf_evlist *evlist)
> {
>        struct perf_evsel *evsel = perf_evlist__first(evlist);
>        struct format_field *field = evsel->tp_format->format.fields;
> 
>        if (!field)
>                return -1;
> 
>        payload_offset = field->offset;
>        return 0;
> }
> 
> Maybe we can add some helper routine which returns first non-common
> field, like:

Humm, I think the above is enough but then having a a helper doesn't
hurt, but I would prefer it to instead be:

struct format_field *perf_evsel__fields(struct perf_evsel *evsel)
{
	return evsel->tp_format->format.fields;
}

The "name" parameter is not needed, and in essence getting "all the
fields" and the "first field" is the same operation, right? We can use
the shorter form.

In the evlist class we need this:

static inline
struct perf_evsel *perf_evlist__first(struct perf_evlist *evlist)
{
        return list_entry(evlist->entries.next, struct perf_evsel, node);
}

Because the open coded equivalent is long, as we use list_head to
maintain that list, but format.fields is a singly linked list.

Can you send a respinned patch with all we discussed, please?

- Arnaldo

> 
> struct format_field *perf_evsel__first_field(struct perf_evsel *evsel, const char *name)
> {
> 	return evsel->tp_format->format.fields;
> }
> 
> and call it in timechart__set_payload_offset?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/