Re: [PATCH] libperf: Add API for allocating new thread map

[Tzvetomir Stoyanov]

On Wed, Feb 16, 2022 at 3:54 PM Arnaldo Carvalho de Melo
<arnaldo.melo@xxxxxxxxx> wrote:
>
> Em Tue, Feb 15, 2022 at 01:18:31PM +0100, Jiri Olsa escreveu:
> > On Thu, Feb 10, 2022 at 10:52:25AM +0200, Tzvetomir Stoyanov (VMware) wrote:
> > > The existing API perf_thread_map__new_dummy() allocates new thread map
> > > for one thread. I couldn't find a way to reallocate the map with more
> > > threads, or to allocate a new map for more than one thread. Having
> > > multiple threads in a thread map is essential for some use cases.
> > > That's why a new API is proposed, which allocates a new thread map
> > > for given number of threads:
> > >  perf_thread_map__new()
> >
> > I'm ok with that, just wondering if we should call it 'perf_thread_map__new_nr'
> > because we already have perf_cpu_map__new(const char *cpu_list) so
> > it might be better to keep perf_cpu_map and perf_thread_map in sync
>
> > Arnaldo, thoughts on this?
>
> Agreed on trying to have similar semantics for the default, main
> constructor, so we probably need perf_thread_map__new(const char *thread_list).
>
> In tools/perf/util/thread_map.c, from where tools/lib/threadmap.c came
> from we have many alternative constructors:
>
> ⬢[acme@toolbox perf]$ grep 'struct perf_thread_map \*thread_map__new' tools/perf/util/thread_map.c
> struct perf_thread_map *thread_map__new_by_pid(pid_t pid)
> struct perf_thread_map *thread_map__new_by_tid(pid_t tid)
> struct perf_thread_map *thread_map__new_all_cpus(void)
> struct perf_thread_map *thread_map__new_by_uid(uid_t uid)
> struct perf_thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid)
> static struct perf_thread_map *thread_map__new_by_pid_str(const char *pid_str)
> struct perf_thread_map *thread_map__new_by_tid_str(const char *tid_str)
> struct perf_thread_map *thread_map__new_str(const char *pid, const char *tid,
> struct perf_thread_map *thread_map__new_event(struct perf_record_thread_map *event)
> ⬢[acme@toolbox perf]$
>
> The one you want is almost:
>
>         thread_map__new_by_tid_str(const char *tid_str)
>
> but perhaps it would be better to have:
>
> struct perf_thread_map *thread_map__new_array(int nr_threads, pid_t *array);

I like the idea, this API is flexible enough for most of the use
cases. I'll submit the next version of the patch with this
implementation.

>
> But yeah, if your logic needs to first allocate space for the thread_map
> and then populate it, make it so that array == NULL is supported for
> that case, then thread_map__new_array() will not touch it and set
> everything to -1, to be populated later using perf_thread_map__set_pid().
>
> With that thread_map__new_by_tid_str() could be reimplemented as a
> wrapper around thread_map__new_array(). :-)
>
> While we're on this, shouldn't we rename 'thread' in
> tools/lib/perf/include/perf/threadmap.h and threadmap.c to be 'idx' to
> avoid confusion?

You mean the input parameter "int thread", in these APIs ?
  perf_thread_map__set_pid()
  perf_thread_map__comm()
  perf_thread_map__pid()
It makes sense to me, as this is the index in the thread map. I can
submit a separate patch with this renaming.

>
> - Arnaldo
>
> > jirka

Thanks!

[ ... ]


-- 
Tzvetomir (Ceco) Stoyanov
VMware Open Source Technology Center