Re: [PATCH 02/27] perf tools: Add comm string into struct thread_map

[Arnaldo Carvalho de Melo]

Em Tue, Jun 23, 2015 at 11:56:18PM +0200, Jiri Olsa escreveu:
> On Tue, Jun 23, 2015 at 06:01:58PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Jun 23, 2015 at 12:36:03AM +0200, Jiri Olsa escreveu:
> > > Adding support to hold comm name together with pids in
> > > 'struct thread_map'. It will be useful for --per-thread
> > > option to display task pid together with task name.

> > > Getting the task name from /proc/$pid/comm.

> > Do we really need to read this all the time? Or just for tools that need
> > this?

> > Can't we have a thread_map__read_comms(map);

> > That we can make this specific --per-thread option in 'stat' call?

> ook

Thanks, this made me think: thread_map is just an array that enumerates
the pids ultimately for use with sys_perf_event_open(), bloating it with
extra details used by just one oddball subfeature of just one tool seems
overkill.

Probably it would be better to rename it to pid_map, that is what is
useful for all tools using it to create the event file descriptors and
have a comm_map that would return another enumeration to obtain the
comms, something like:

	struct thread_map *tids = thread__map_by_foo(parameters);
	struct comm_map *comms = comm_map__new(tids);

That way the various ways of populating the thread_map->map[] (the
_by_foo() variants of thread_map we have) would do that leg work and
the comm_map__new(tids) would just read the comms.

The thread_map users would not have to be touched at all, it would be
kept as simple as it was and just tools that need those comms would go
to the trouble of allocating the needed space for the array and to do
the comm reading.

But nah, I'll probably merge what you did and later, if I find the time,
do it myself, better to have that new feature than to be picky, right?

Thanks,

- Arnaldo
--
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/