From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751706AbdLBEsE (ORCPT ); Fri, 1 Dec 2017 23:48:04 -0500 Received: from mga07.intel.com ([134.134.136.100]:40248 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751415AbdLBEsD (ORCPT ); Fri, 1 Dec 2017 23:48:03 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,348,1508828400"; d="scan'208";a="1250967389" Subject: Re: [PATCH v5 10/12] perf util: Reuse thread_map__new_by_uid to enumerate threads from /proc To: Arnaldo Carvalho de Melo Cc: jolsa@kernel.org, peterz@infradead.org, mingo@redhat.com, alexander.shishkin@linux.intel.com, Linux-kernel@vger.kernel.org, ak@linux.intel.com, kan.liang@intel.com, yao.jin@intel.com References: <1512125856-22056-1-git-send-email-yao.jin@linux.intel.com> <1512125856-22056-11-git-send-email-yao.jin@linux.intel.com> <20171201144425.GZ3298@kernel.org> From: "Jin, Yao" Message-ID: <261b2265-82a6-7d27-a4e0-627548fed644@linux.intel.com> Date: Sat, 2 Dec 2017 12:47:59 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171201144425.GZ3298@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/1/2017 10:44 PM, Arnaldo Carvalho de Melo wrote: > Em Fri, Dec 01, 2017 at 06:57:34PM +0800, Jin Yao escreveu: >> Perf already has a function thread_map__new_by_uid() which can >> enumerate all threads from /proc by uid. >> >> This patch creates a static function enumerate_threads() which >> reuses the common code in thread_map__new_by_uid() to enumerate >> threads from /proc. >> >> The enumerate_threads() is shared by thread_map__new_by_uid() >> and a new function thread_map__new_threads(). >> >> The new function thread_map__new_threads() is called to enumerate >> all threads from /proc. >> >> Signed-off-by: Jin Yao >> --- >> tools/perf/tests/thread-map.c | 2 +- >> tools/perf/util/evlist.c | 3 ++- >> tools/perf/util/thread_map.c | 19 ++++++++++++++++--- >> tools/perf/util/thread_map.h | 3 ++- >> 4 files changed, 21 insertions(+), 6 deletions(-) >> >> diff --git a/tools/perf/tests/thread-map.c b/tools/perf/tests/thread-map.c >> index dbcb6a1..4de1939 100644 >> --- a/tools/perf/tests/thread-map.c >> +++ b/tools/perf/tests/thread-map.c >> @@ -105,7 +105,7 @@ int test__thread_map_remove(struct test *test __maybe_unused, int subtest __mayb >> TEST_ASSERT_VAL("failed to allocate map string", >> asprintf(&str, "%d,%d", getpid(), getppid()) >= 0); >> >> - threads = thread_map__new_str(str, NULL, 0); >> + threads = thread_map__new_str(str, NULL, 0, false); >> >> TEST_ASSERT_VAL("failed to allocate thread_map", >> threads); >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c >> index 199bb82..05b8f2b 100644 >> --- a/tools/perf/util/evlist.c >> +++ b/tools/perf/util/evlist.c >> @@ -1102,7 +1102,8 @@ int perf_evlist__create_maps(struct perf_evlist *evlist, struct target *target) >> struct cpu_map *cpus; >> struct thread_map *threads; >> >> - threads = thread_map__new_str(target->pid, target->tid, target->uid); >> + threads = thread_map__new_str(target->pid, target->tid, target->uid, >> + target->per_thread); >> >> if (!threads) >> return -1; >> diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c >> index be0d5a7..5672268 100644 >> --- a/tools/perf/util/thread_map.c >> +++ b/tools/perf/util/thread_map.c >> @@ -92,7 +92,7 @@ struct thread_map *thread_map__new_by_tid(pid_t tid) >> return threads; >> } >> >> -struct thread_map *thread_map__new_by_uid(uid_t uid) >> +static struct thread_map *enumerate_threads(uid_t uid) >> { >> DIR *proc; >> int max_threads = 32, items, i; >> @@ -124,7 +124,7 @@ struct thread_map *thread_map__new_by_uid(uid_t uid) >> if (stat(path, &st) != 0) >> continue; > > Look, for the case where you want all threads enumerated you will incur > the above stat() cost for all of them and will not use it at all... > > And new_threads() seems vague, I'm using the ter used by 'perf record' > for system wide sampling, see the patch below: > > diff --git a/tools/perf/util/thread_map.c b/tools/perf/util/thread_map.c > index be0d5a736dea..79d11bd4543a 100644 > --- a/tools/perf/util/thread_map.c > +++ b/tools/perf/util/thread_map.c > @@ -92,7 +92,7 @@ struct thread_map *thread_map__new_by_tid(pid_t tid) > return threads; > } > > -struct thread_map *thread_map__new_by_uid(uid_t uid) > +static struct thread_map *__thread_map__new_all_cpus(uid_t uid) > { > DIR *proc; > int max_threads = 32, items, i; > @@ -113,7 +113,6 @@ struct thread_map *thread_map__new_by_uid(uid_t uid) > while ((dirent = readdir(proc)) != NULL) { > char *end; > bool grow = false; > - struct stat st; > pid_t pid = strtol(dirent->d_name, &end, 10); > > if (*end) /* only interested in proper numerical dirents */ > @@ -121,11 +120,12 @@ struct thread_map *thread_map__new_by_uid(uid_t uid) > > snprintf(path, sizeof(path), "/proc/%s", dirent->d_name); > > - if (stat(path, &st) != 0) > - continue; > + if (uid != UID_MAX) { > + struct stat st; > > - if (st.st_uid != uid) > - continue; > + if (stat(path, &st) != 0 || st.st_uid != uid) > + continue; > + } > > snprintf(path, sizeof(path), "/proc/%d/task", pid); > items = scandir(path, &namelist, filter, NULL); > @@ -178,6 +178,16 @@ struct thread_map *thread_map__new_by_uid(uid_t uid) > goto out_closedir; > } > > +struct thread_map *thread_map__new_all_cpus(void) > +{ > + return __thread__new_all_cpus(UID_MAX); > +} > + > +struct thread_map *thread_map__new_by_uid(uid_t uid) > +{ > + return __thread__new_all_cpus(uid); > +} > + > struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid) > { > if (pid != -1) > diff --git a/tools/perf/util/thread_map.h b/tools/perf/util/thread_map.h > index f15803985435..07a765fb22bb 100644 > --- a/tools/perf/util/thread_map.h > +++ b/tools/perf/util/thread_map.h > @@ -23,6 +23,7 @@ struct thread_map *thread_map__new_dummy(void); > struct thread_map *thread_map__new_by_pid(pid_t pid); > struct thread_map *thread_map__new_by_tid(pid_t tid); > struct thread_map *thread_map__new_by_uid(uid_t uid); > +struct thread_map *thread_map__new_all_cpus(void); > struct thread_map *thread_map__new(pid_t pid, pid_t tid, uid_t uid); > struct thread_map *thread_map__new_event(struct thread_map_event *event); > > It looks good, thanks Arnaldo! Thanks Jin Yao