Received: by 2002:a05:6a10:206:0:0:0:0 with SMTP id 6csp1328646pxj; Fri, 21 May 2021 11:25:05 -0700 (PDT) X-Google-Smtp-Source: ABdhPJywrd2R5dC24yRkwLKqUKyn3biW1k/2I7CoRH9BtVxyXI5jsEDflJxhXmToQvpfJYDT8TCp X-Received: by 2002:a17:906:2596:: with SMTP id m22mr11798233ejb.175.1621621504970; Fri, 21 May 2021 11:25:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1621621504; cv=none; d=google.com; s=arc-20160816; b=MWo13DmWJlLKbShH6jTtOgqMloqtXtLGCXjyRYfSoqDY5zmDqHilHbph7cX75g72yZ zWSqKXgRf9LZuoi5fDi9epIAW5OAKAv1nxA3bUAkIix18IOXZ1VONPFhqclWKGoDSwDU sTRDEH8yXNWDCxNqvJBSz/MDSezXQcrnmw4cwWrhgonv5NQIu4Gq7y1sQJksSG60nMCq QteClUx/43bynoVPSQ20QZuD+FEnSPTK+W0LZHIJd1LGIC/m316qxD2lxH4FcV3MkHbf xPcWpZmrXB2U7ZZYWxgT0xtDRvN75uP9kiqHHwGbFMlOQaRFBIk2Xub5YVCHLGlaFcRv Vqxw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version; bh=ElKZNpBOYHsMBQ+c8xKewRsmPuyu5uM+aVGvNwwS/uk=; b=qBUBub2OccpbNa/l2C/aBB5OxVpboRkB8Plj15rKqhUA0y4NojEHajBPn1+1uC+ciN WFPEO2+Mwx+mj1G/lYmTwRO6k7qGzdz1aV8YZfXSseJ2lJdN8FaDeU/6kZOyJPQvuiYT Sdjq07snRVJrCs2da6PpFLfS+mgCHfPb8HqhrCj0JyeH2lg8nYX7pp2REA5nEmhNcGZf TYLuuAtej9P0ColyxThh+Onp55sTWMwjmIa3kdBkDBtFcQGvk0zEh68DL6WGWr0sBgyy Om7qaMG3XbIItlkX+hcY0nHG1K4SVIMoMQDCHWOdxUcK2AQwhy6TRmznjE+Qil1hSICW Dj5A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id bi14si5802291ejb.441.2021.05.21.11.24.41; Fri, 21 May 2021 11:25:04 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234232AbhEUGvv (ORCPT + 99 others); Fri, 21 May 2021 02:51:51 -0400 Received: from mail-lj1-f174.google.com ([209.85.208.174]:36629 "EHLO mail-lj1-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234290AbhEUGvn (ORCPT ); Fri, 21 May 2021 02:51:43 -0400 Received: by mail-lj1-f174.google.com with SMTP id 131so22699851ljj.3 for ; Thu, 20 May 2021 23:50:19 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=ElKZNpBOYHsMBQ+c8xKewRsmPuyu5uM+aVGvNwwS/uk=; b=Z+dZd4AyWqbRhKIK3vV6lDQue17vvvosWru9L6//AttnrmJaQqp0m2qflg9ZVqWIdD e/m3iRB1rK1wF1qKa6Xoa4s4vjLNIpuPhOa8Z0XnGQ2TZhJOBejKqTN7D6LToZRQWT3D 9rNCSUWrhz0Op2tuSy2/O5oJ4WcK1XlN3qXFGu2d25PM/bwUrnzcuJG4LdmulSPBk53Y 3aOO4VOYvvq6OCG2t/sHkTPrlZHqDeLwOMmK0u9mv7nqVptjvvZPSD40e9fJfEVOUKD3 gR2qTnj+0sr26Xszt3qftS2xdI45IOCSgtmKaCOp1sLOmI9GwhcYscN7qYKg+Pp3HATC Vi9A== X-Gm-Message-State: AOAM5334Y2CqBlZZrAZMNIGE9+VHCyy1njdpJvb8CQvf8GmnNhDwawEq zgoleywBRGZGlOwFHX6v5gNygeuDsA7OJkT7UKkutX4Q X-Received: by 2002:a2e:a795:: with SMTP id c21mr5779290ljf.26.1621579818544; Thu, 20 May 2021 23:50:18 -0700 (PDT) MIME-Version: 1.0 References: <581727f20df452b50d5dece1b1ef1a5fc9ef45e7.1619781188.git.alexey.v.bayduraev@linux.intel.com> In-Reply-To: <581727f20df452b50d5dece1b1ef1a5fc9ef45e7.1619781188.git.alexey.v.bayduraev@linux.intel.com> From: Namhyung Kim Date: Thu, 20 May 2021 23:50:07 -0700 Message-ID: Subject: Re: [PATCH v5 10/20] perf record: introduce --threads= command line option To: Alexey Bayduraev Cc: Arnaldo Carvalho de Melo , Jiri Olsa , Alexander Shishkin , Peter Zijlstra , Ingo Molnar , linux-kernel , Andi Kleen , Adrian Hunter , Alexander Antonov , Alexei Budankov Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello, On Tue, May 4, 2021 at 12:06 AM Alexey Bayduraev wrote: > > Provide --threads option in perf record command line interface. > The option can have a value in the form of masks that specify > cpus to be monitored with data streaming threads and its layout > in system topology. The masks can be filtered using cpu mask > provided via -C option. > > The specification value can be user defined list of masks. Masks > separated by colon define cpus to be monitored by one thread and > affinity mask of that thread is separated by slash. For example: > /:/ > specifies parallel threads layout that consists of two threads > with corresponding assigned cpus to be monitored. > > The specification value can be a string e.g. "cpu", "core" or > "socket" meaning creation of data streaming thread for every > cpu or core or socket to monitor distinct cpus or cpus grouped > by core or socket. > > The option provided with no or empty value defaults to per-cpu > parallel threads layout creating data streaming thread for every > cpu being monitored. > > Feature design and implementation are based on prototypes [1], [2]. > > [1] git clone https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git -b perf/record_threads > [2] https://lore.kernel.org/lkml/20180913125450.21342-1-jolsa@kernel.org/ > > Suggested-by: Jiri Olsa > Suggested-by: Namhyung Kim > Acked-by: Andi Kleen > Signed-off-by: Alexey Bayduraev > --- > tools/perf/builtin-record.c | 342 +++++++++++++++++++++++++++++++++++- > tools/perf/util/record.h | 1 + > 2 files changed, 341 insertions(+), 2 deletions(-) > > diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c > index bf730e1220dc..e5ea1334cc7d 100644 > --- a/tools/perf/builtin-record.c > +++ b/tools/perf/builtin-record.c > @@ -51,6 +51,7 @@ > #include "util/evlist-hybrid.h" > #include "asm/bug.h" > #include "perf.h" > +#include "cputopo.h" > > #include > #include > @@ -122,6 +123,20 @@ static const char *thread_msg_tags[THREAD_MSG__MAX] = { > "UNDEFINED", "READY" > }; > > +enum thread_spec { > + THREAD_SPEC__UNDEFINED = 0, > + THREAD_SPEC__CPU, > + THREAD_SPEC__CORE, > + THREAD_SPEC__SOCKET, > + THREAD_SPEC__NUMA, > + THREAD_SPEC__USER, > + THREAD_SPEC__MAX, > +}; > + > +static const char *thread_spec_tags[THREAD_SPEC__MAX] = { > + "undefined", "cpu", "core", "socket", "numa", "user" > +}; > + > struct record { > struct perf_tool tool; > struct record_opts opts; > @@ -2704,6 +2719,70 @@ static void record__thread_mask_free(struct thread_mask *mask) > record__mmap_cpu_mask_free(&mask->affinity); > } > > +static int record__thread_mask_or(struct thread_mask *dest, struct thread_mask *src1, > + struct thread_mask *src2) > +{ > + if (src1->maps.nbits != src2->maps.nbits || > + dest->maps.nbits != src1->maps.nbits || > + src1->affinity.nbits != src2->affinity.nbits || > + dest->affinity.nbits != src1->affinity.nbits) > + return -EINVAL; > + > + bitmap_or(dest->maps.bits, src1->maps.bits, > + src2->maps.bits, src1->maps.nbits); > + bitmap_or(dest->affinity.bits, src1->affinity.bits, > + src2->affinity.bits, src1->affinity.nbits); > + > + return 0; > +} > + > +static int record__thread_mask_intersects(struct thread_mask *mask_1, struct thread_mask *mask_2) > +{ > + int res1, res2; > + > + if (mask_1->maps.nbits != mask_2->maps.nbits || > + mask_1->affinity.nbits != mask_2->affinity.nbits) > + return -EINVAL; > + > + res1 = bitmap_intersects(mask_1->maps.bits, mask_2->maps.bits, > + mask_1->maps.nbits); > + res2 = bitmap_intersects(mask_1->affinity.bits, mask_2->affinity.bits, > + mask_1->affinity.nbits); > + if (res1 || res2) > + return 1; > + > + return 0; > +} > + > +static int record__parse_threads(const struct option *opt, const char *str, int unset) > +{ > + int s; > + struct record_opts *opts = opt->value; > + > + if (unset || !str || !strlen(str)) { > + opts->threads_spec = THREAD_SPEC__CPU; > + } else { > + for (s = 1; s < THREAD_SPEC__MAX; s++) { > + if (s == THREAD_SPEC__USER) { > + opts->threads_user_spec = strdup(str); > + opts->threads_spec = THREAD_SPEC__USER; > + break; > + } > + if (!strncasecmp(str, thread_spec_tags[s], strlen(thread_spec_tags[s]))) { > + opts->threads_spec = s; > + break; > + } > + } > + } > + > + pr_debug("threads_spec: %s", thread_spec_tags[opts->threads_spec]); > + if (opts->threads_spec == THREAD_SPEC__USER) > + pr_debug("=[%s]", opts->threads_user_spec); > + pr_debug("\n"); > + > + return 0; > +} > + > static int parse_output_max_size(const struct option *opt, > const char *str, int unset) > { > @@ -3134,6 +3213,9 @@ static struct option __record_options[] = { > "\t\t\t Optionally send control command completion ('ack\\n') to ack-fd descriptor.\n" > "\t\t\t Alternatively, ctl-fifo / ack-fifo will be opened and used as ctl-fd / ack-fd.", > parse_control_option), > + OPT_CALLBACK_OPTARG(0, "threads", &record.opts, NULL, "spec", > + "write collected trace data into several data files using parallel threads", > + record__parse_threads), > OPT_END() > }; > > @@ -3147,6 +3229,17 @@ static void record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_c > set_bit(cpus->map[c], mask->bits); > } > > +static void record__mmap_cpu_mask_init_spec(struct mmap_cpu_mask *mask, char *mask_spec) > +{ > + struct perf_cpu_map *cpus; > + > + cpus = perf_cpu_map__new(mask_spec); > + if (cpus) { > + record__mmap_cpu_mask_init(mask, cpus); > + perf_cpu_map__put(cpus); > + } > +} > + > static int record__alloc_thread_masks(struct record *rec, int nr_threads, int nr_bits) > { > int t, ret; > @@ -3166,6 +3259,224 @@ static int record__alloc_thread_masks(struct record *rec, int nr_threads, int nr > > return 0; > } > + > +static int record__init_thread_cpu_masks(struct record *rec, struct perf_cpu_map *cpus) > +{ > + int t, ret, nr_cpus = perf_cpu_map__nr(cpus); > + > + ret = record__alloc_thread_masks(rec, nr_cpus, cpu__max_cpu()); > + if (ret) > + return ret; > + > + rec->nr_threads = nr_cpus; > + pr_debug("threads: nr_threads=%d\n", rec->nr_threads); > + > + for (t = 0; t < rec->nr_threads; t++) { > + set_bit(cpus->map[t], rec->thread_masks[t].maps.bits); > + pr_debug("thread_masks[%d]: maps mask [%d]\n", t, cpus->map[t]); > + set_bit(cpus->map[t], rec->thread_masks[t].affinity.bits); > + pr_debug("thread_masks[%d]: affinity mask [%d]\n", t, cpus->map[t]); > + } > + > + return 0; > +} > + > +static int record__init_thread_masks_spec(struct record *rec, struct perf_cpu_map *cpus, > + char **maps_spec, char **affinity_spec, u32 nr_spec) > +{ > + u32 s; > + int ret = 0, nr_threads = 0; > + struct mmap_cpu_mask cpus_mask; > + struct thread_mask thread_mask, full_mask, *prev_masks; > + > + ret = record__mmap_cpu_mask_alloc(&cpus_mask, cpu__max_cpu()); > + if (ret) > + goto out; > + record__mmap_cpu_mask_init(&cpus_mask, cpus); > + ret = record__thread_mask_alloc(&thread_mask, cpu__max_cpu()); > + if (ret) > + goto out_free_cpu_mask; > + ret = record__thread_mask_alloc(&full_mask, cpu__max_cpu()); > + if (ret) > + goto out_free_thread_mask; > + record__thread_mask_clear(&full_mask); > + > + for (s = 0; s < nr_spec; s++) { > + record__thread_mask_clear(&thread_mask); > + > + record__mmap_cpu_mask_init_spec(&thread_mask.maps, maps_spec[s]); > + record__mmap_cpu_mask_init_spec(&thread_mask.affinity, affinity_spec[s]); > + > + if (!bitmap_and(thread_mask.maps.bits, thread_mask.maps.bits, > + cpus_mask.bits, thread_mask.maps.nbits) || > + !bitmap_and(thread_mask.affinity.bits, thread_mask.affinity.bits, > + cpus_mask.bits, thread_mask.affinity.nbits)) > + continue; > + > + ret = record__thread_mask_intersects(&thread_mask, &full_mask); > + if (ret) > + goto out_free_full_mask; > + record__thread_mask_or(&full_mask, &full_mask, &thread_mask); > + > + prev_masks = rec->thread_masks; > + rec->thread_masks = realloc(rec->thread_masks, > + (nr_threads + 1) * sizeof(struct thread_mask)); > + if (!rec->thread_masks) { > + pr_err("Failed to allocate thread masks\n"); > + rec->thread_masks = prev_masks; > + ret = -ENOMEM; > + goto out_free_full_mask; > + } > + rec->thread_masks[nr_threads] = thread_mask; > + pr_debug("thread_masks[%d]: addr=", nr_threads); > + mmap_cpu_mask__scnprintf(&rec->thread_masks[nr_threads].maps, "maps"); > + pr_debug("thread_masks[%d]: addr=", nr_threads); > + mmap_cpu_mask__scnprintf(&rec->thread_masks[nr_threads].affinity, "affinity"); It's not clear mmap_cpu_mask__scnprintf() will call pr_debug() and it also has some more work to prepare the buffer. Maybe we can add an 'if (verbose)' block to inform it's all for debug information and skip the unnecessary work. > + nr_threads++; > + ret = record__thread_mask_alloc(&thread_mask, cpu__max_cpu()); > + if (ret) > + goto out_free_full_mask; I wonder if it's possible to fail on the first bitmap and have a pointer to the old bitmap for the second one. It might lead to double free then.. > + } > + > + rec->nr_threads = nr_threads; > + pr_debug("threads: nr_threads=%d\n", rec->nr_threads); > + > +out_free_full_mask: > + record__thread_mask_free(&full_mask); > +out_free_thread_mask: > + record__thread_mask_free(&thread_mask); > +out_free_cpu_mask: > + record__mmap_cpu_mask_free(&cpus_mask); > +out: > + return ret; > +} > + > +static int record__init_thread_core_masks(struct record *rec, struct perf_cpu_map *cpus) > +{ > + int ret; > + struct cpu_topology *topo; > + > + topo = cpu_topology__new(); > + if (!topo) > + return -EINVAL; > + > + ret = record__init_thread_masks_spec(rec, cpus, topo->thread_siblings, > + topo->thread_siblings, topo->thread_sib); > + cpu_topology__delete(topo); > + > + return ret; > +} > + > +static int record__init_thread_socket_masks(struct record *rec, struct perf_cpu_map *cpus) > +{ > + int ret; > + struct cpu_topology *topo; > + > + topo = cpu_topology__new(); > + if (!topo) > + return -EINVAL; > + > + ret = record__init_thread_masks_spec(rec, cpus, topo->core_siblings, > + topo->core_siblings, topo->core_sib); > + cpu_topology__delete(topo); > + > + return ret; > +} > + > +static int record__init_thread_numa_masks(struct record *rec, struct perf_cpu_map *cpus) > +{ > + u32 s; > + int ret; > + char **spec; > + struct numa_topology *topo; > + > + topo = numa_topology__new(); > + if (!topo) > + return -EINVAL; > + spec = zalloc(topo->nr * sizeof(char *)); > + if (!spec) { > + ret = -ENOMEM; > + goto out_delete_topo; > + } > + for (s = 0; s < topo->nr; s++) > + spec[s] = topo->nodes[s].cpus; > + > + ret = record__init_thread_masks_spec(rec, cpus, spec, spec, topo->nr); > + > + zfree(&spec); > + > +out_delete_topo: > + numa_topology__delete(topo); > + > + return ret; > +} > + > +static int record__init_thread_user_masks(struct record *rec, struct perf_cpu_map *cpus) > +{ > + int t, ret; > + u32 s, nr_spec = 0; > + char **maps_spec = NULL, **affinity_spec = NULL, **prev_spec; > + char *spec, *spec_ptr, *user_spec, *mask, *mask_ptr; > + > + for (t = 0, user_spec = (char *)rec->opts.threads_user_spec; ; t++, user_spec = NULL) { > + spec = strtok_r(user_spec, ":", &spec_ptr); > + if (spec == NULL) > + break; > + pr_debug(" spec[%d]: %s\n", t, spec); > + mask = strtok_r(spec, "/", &mask_ptr); > + if (mask == NULL) > + break; > + pr_debug(" maps mask: %s\n", mask); > + prev_spec = maps_spec; > + maps_spec = realloc(maps_spec, (nr_spec + 1) * sizeof(char *)); > + if (!maps_spec) { > + pr_err("Failed to realloc maps_spec\n"); > + maps_spec = prev_spec; > + ret = -ENOMEM; > + goto out_free_all_specs; > + } > + maps_spec[nr_spec] = strdup(mask); > + if (!maps_spec[nr_spec]) { > + pr_err("Failed to alloc maps_spec[%d]\n", nr_spec); > + ret = -ENOMEM; > + goto out_free_all_specs; > + } > + mask = strtok_r(NULL, "/", &mask_ptr); > + if (mask == NULL) > + break; > + pr_debug(" affinity mask: %s\n", mask); > + prev_spec = affinity_spec; > + affinity_spec = realloc(affinity_spec, (nr_spec + 1) * sizeof(char *)); > + if (!affinity_spec) { > + pr_err("Failed to realloc affinity_spec\n"); > + affinity_spec = prev_spec; > + ret = -ENOMEM; > + goto out_free_all_specs; This will leak the last maps_spec as nr_spec was not increased. > + } > + affinity_spec[nr_spec] = strdup(mask); > + if (!affinity_spec[nr_spec]) { > + pr_err("Failed to alloc affinity_spec[%d]\n", nr_spec); > + ret = -ENOMEM; > + goto out_free_all_specs; Ditto. Thanks, Namhyung > + } > + nr_spec++; > + } > + > + ret = record__init_thread_masks_spec(rec, cpus, maps_spec, affinity_spec, nr_spec); > + > +out_free_all_specs: > + for (s = 0; s < nr_spec; s++) { > + if (maps_spec) > + free(maps_spec[s]); > + if (affinity_spec) > + free(affinity_spec[s]); > + } > + free(affinity_spec); > + free(maps_spec); > + > + return ret; > +} > + > static int record__init_thread_default_masks(struct record *rec, struct perf_cpu_map *cpus) > { > int ret; > @@ -3183,9 +3494,33 @@ static int record__init_thread_default_masks(struct record *rec, struct perf_cpu > > static int record__init_thread_masks(struct record *rec) > { > + int ret = 0; > struct perf_cpu_map *cpus = rec->evlist->core.cpus; > > - return record__init_thread_default_masks(rec, cpus); > + if (!record__threads_enabled(rec)) > + return record__init_thread_default_masks(rec, cpus); > + > + switch (rec->opts.threads_spec) { > + case THREAD_SPEC__CPU: > + ret = record__init_thread_cpu_masks(rec, cpus); > + break; > + case THREAD_SPEC__CORE: > + ret = record__init_thread_core_masks(rec, cpus); > + break; > + case THREAD_SPEC__SOCKET: > + ret = record__init_thread_socket_masks(rec, cpus); > + break; > + case THREAD_SPEC__NUMA: > + ret = record__init_thread_numa_masks(rec, cpus); > + break; > + case THREAD_SPEC__USER: > + ret = record__init_thread_user_masks(rec, cpus); > + break; > + default: > + break; > + } > + > + return ret; > } > > static int record__fini_thread_masks(struct record *rec) > @@ -3436,7 +3771,10 @@ int cmd_record(int argc, const char **argv) > > err = record__init_thread_masks(rec); > if (err) { > - pr_err("record__init_thread_masks failed, error %d\n", err); > + if (err > 0) > + pr_err("ERROR: parallel data streaming masks (--threads) intersect.\n"); > + else > + pr_err("record__init_thread_masks failed, error %d\n", err); > goto out; > } > > diff --git a/tools/perf/util/record.h b/tools/perf/util/record.h > index 4d68b7e27272..3da156498f47 100644 > --- a/tools/perf/util/record.h > +++ b/tools/perf/util/record.h > @@ -78,6 +78,7 @@ struct record_opts { > int ctl_fd_ack; > bool ctl_fd_close; > int threads_spec; > + const char *threads_user_spec; > }; > > extern const char * const *record_usage; > -- > 2.19.0 >