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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPpZLN56Ew2AvKk6kC-B6sQPAE+JRsnXRPkfWxSEBiR-JdzSHA@mail.gmail.com>
Date:   Wed, 16 Feb 2022 16:44:44 +0200
From:   Tzvetomir Stoyanov <tz.stoyanov@...il.com>
To:     Arnaldo Carvalho de Melo <arnaldo.melo@...il.com>
Cc:     Jiri Olsa <olsajiri@...il.com>, Jiri Olsa <jolsa@...nel.org>,
        Ian Rogers <irogers@...gle.com>,
        Namhyung Kim <namhyung@...nel.org>,
        linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] libperf: Add API for allocating new thread map

On Wed, Feb 16, 2022 at 3:54 PM Arnaldo Carvalho de Melo
<arnaldo.melo@...il.com> 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@...lbox 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@...lbox 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