Re: [PATCH v13 01/16] perf record: Introduce thread affinity and mmap masks

From: Bayduraev, Alexey V
Date: Tue Feb 01 2022 - 06:46:13 EST

Next message: Geert Uytterhoeven: "Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays"
Previous message: Javier Martinez Canillas: "Re: [PATCH 3/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 01.02.2022 0:16, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 31, 2022 at 06:00:31PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Mon, Jan 17, 2022 at 09:34:21PM +0300, Alexey Bayduraev escreveu:
>>> Introduce affinity and mmap thread masks. Thread affinity mask
>>> defines CPUs that a thread is allowed to run on. Thread maps
>>> mask defines mmap data buffers the thread serves to stream
>>> profiling data from.
>>>
>>> Acked-by: Andi Kleen <ak@xxxxxxxxxxxxxxx>
>>> Acked-by: Namhyung Kim <namhyung@xxxxxxxxx>
>>> Reviewed-by: Riccardo Mancini <rickyman7@xxxxxxxxx>
>>> Tested-by: Riccardo Mancini <rickyman7@xxxxxxxxx>
>>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@xxxxxxxxxxxxxxx>
>>
>> Some simplifications I added here while reviewing this patchkit:
>
> But then, why allocate these even without using them? I.e. the init
> should be left for when we are sure that we'll actually use this, i.e.
> when the user asks for parallel mode.
>
> We already have lots of needless initializations, reading of files that
> may not be needed, so we should avoid doing things till we really know
> that we'll use those allocations, readings, etc.
>
> Anyway, continuing to review, will leave what I have at a separata
> branch so that we can continue from there.

In the current design, we assume that without --threads option nr_threads==1
and we allocate rec->thread_masks and rec->thread_data as arrays of size 1,
also we move some variables from "struct record" to rec->thread_data and
use thread_data[0] (thru "thread" pointer) in main thread instead of
"struct record". This simplifies the later implementation.

With another approach we could assume nr_threads==0 and use only necessary
"struct record" variables, but this adds many if(record__threads_enabled())

Regards,
Alexey

>
> - Arnaldo
>
>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>> index 41998f2140cd5119..53b88c8600624237 100644
>> --- a/tools/perf/builtin-record.c
>> +++ b/tools/perf/builtin-record.c
>> @@ -2213,35 +2213,33 @@ static int record__parse_affinity(const struct option *opt, const char *str, int
>>
>> static int record__mmap_cpu_mask_alloc(struct mmap_cpu_mask *mask, int nr_bits)
>> {
>> - mask->nbits = nr_bits;
>> mask->bits = bitmap_zalloc(mask->nbits);
>> if (!mask->bits)
>> return -ENOMEM;
>>
>> + mask->nbits = nr_bits;
>> return 0;
>> }
>>
>> static void record__mmap_cpu_mask_free(struct mmap_cpu_mask *mask)
>> {
>> bitmap_free(mask->bits);
>> + mask->bits = NULL;
>> mask->nbits = 0;
>> }
>>
>> static int record__thread_mask_alloc(struct thread_mask *mask, int nr_bits)
>> {
>> - int ret;
>> + int ret = record__mmap_cpu_mask_alloc(&mask->maps, nr_bits);
>>
>> - ret = record__mmap_cpu_mask_alloc(&mask->maps, nr_bits);
>> if (ret) {
>> mask->affinity.bits = NULL;
>> return ret;
>> }
>>
>> ret = record__mmap_cpu_mask_alloc(&mask->affinity, nr_bits);
>> - if (ret) {
>> + if (ret)
>> record__mmap_cpu_mask_free(&mask->maps);
>> - mask->maps.bits = NULL;
>> - }
>>
>> return ret;
>> }
>> @@ -2733,18 +2731,14 @@ struct option *record_options = __record_options;
>>
>> static void record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cpu_map *cpus)
>> {
>> - int c;
>> -
>> - for (c = 0; c < cpus->nr; c++)
>> + for (int c = 0; c < cpus->nr; c++)
>> set_bit(cpus->map[c].cpu, mask->bits);
>> }
>>
>> static void record__free_thread_masks(struct record *rec, int nr_threads)
>> {
>> - int t;
>> -
>> if (rec->thread_masks)
>> - for (t = 0; t < nr_threads; t++)
>> + for (int t = 0; t < nr_threads; t++)
>> record__thread_mask_free(&rec->thread_masks[t]);
>>
>> zfree(&rec->thread_masks);
>> @@ -2752,7 +2746,7 @@ static void record__free_thread_masks(struct record *rec, int nr_threads)
>>
>> static int record__alloc_thread_masks(struct record *rec, int nr_threads, int nr_bits)
>> {
>> - int t, ret;
>> + int ret;
>>
>> rec->thread_masks = zalloc(nr_threads * sizeof(*(rec->thread_masks)));
>> if (!rec->thread_masks) {
>> @@ -2760,7 +2754,7 @@ static int record__alloc_thread_masks(struct record *rec, int nr_threads, int nr
>> return -ENOMEM;
>> }
>>
>> - for (t = 0; t < nr_threads; t++) {
>> + for (int t = 0; t < nr_threads; t++) {
>> ret = record__thread_mask_alloc(&rec->thread_masks[t], nr_bits);
>> if (ret) {
>> pr_err("Failed to allocate thread masks[%d]\n", t);
>> @@ -2778,9 +2772,7 @@ static int record__alloc_thread_masks(struct record *rec, int nr_threads, int nr
>>
>> static int record__init_thread_default_masks(struct record *rec, struct perf_cpu_map *cpus)
>> {
>> - int ret;
>> -
>> - ret = record__alloc_thread_masks(rec, 1, cpu__max_cpu().cpu);
>> + int ret = record__alloc_thread_masks(rec, 1, cpu__max_cpu().cpu);
>> if (ret)
>> return ret;
>>
>>
>>
>>> ---
>>> tools/perf/builtin-record.c | 123 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 123 insertions(+)
>>>
>>> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
>>> index bb716c953d02..41998f2140cd 100644
>>> --- a/tools/perf/builtin-record.c
>>> +++ b/tools/perf/builtin-record.c
>>> @@ -87,6 +87,11 @@ struct switch_output {
>>> int cur_file;
>>> };
>>>
>>> +struct thread_mask {
>>> + struct mmap_cpu_mask maps;
>>> + struct mmap_cpu_mask affinity;
>>> +};
>>> +
>>> struct record {
>>> struct perf_tool tool;
>>> struct record_opts opts;
>>> @@ -112,6 +117,8 @@ struct record {
>>> struct mmap_cpu_mask affinity_mask;
>>> unsigned long output_max_size; /* = 0: unlimited */
>>> struct perf_debuginfod debuginfod;
>>> + int nr_threads;
>>> + struct thread_mask *thread_masks;
>>> };
>>>
>>> static volatile int done;
>>> @@ -2204,6 +2211,47 @@ static int record__parse_affinity(const struct option *opt, const char *str, int
>>> return 0;
>>> }
>>>
>>> +static int record__mmap_cpu_mask_alloc(struct mmap_cpu_mask *mask, int nr_bits)
>>> +{
>>> + mask->nbits = nr_bits;
>>> + mask->bits = bitmap_zalloc(mask->nbits);
>>> + if (!mask->bits)
>>> + return -ENOMEM;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static void record__mmap_cpu_mask_free(struct mmap_cpu_mask *mask)
>>> +{
>>> + bitmap_free(mask->bits);
>>> + mask->nbits = 0;
>>> +}
>>> +
>>> +static int record__thread_mask_alloc(struct thread_mask *mask, int nr_bits)
>>> +{
>>> + int ret;
>>> +
>>> + ret = record__mmap_cpu_mask_alloc(&mask->maps, nr_bits);
>>> + if (ret) {
>>> + mask->affinity.bits = NULL;
>>> + return ret;
>>> + }
>>> +
>>> + ret = record__mmap_cpu_mask_alloc(&mask->affinity, nr_bits);
>>> + if (ret) {
>>> + record__mmap_cpu_mask_free(&mask->maps);
>>> + mask->maps.bits = NULL;
>>> + }
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static void record__thread_mask_free(struct thread_mask *mask)
>>> +{
>>> + record__mmap_cpu_mask_free(&mask->maps);
>>> + record__mmap_cpu_mask_free(&mask->affinity);
>>> +}
>>> +
>>> static int parse_output_max_size(const struct option *opt,
>>> const char *str, int unset)
>>> {
>>> @@ -2683,6 +2731,73 @@ static struct option __record_options[] = {
>>>
>>> struct option *record_options = __record_options;
>>>
>>> +static void record__mmap_cpu_mask_init(struct mmap_cpu_mask *mask, struct perf_cpu_map *cpus)
>>> +{
>>> + int c;
>>> +
>>> + for (c = 0; c < cpus->nr; c++)
>>> + set_bit(cpus->map[c].cpu, mask->bits);
>>> +}
>>> +
>>> +static void record__free_thread_masks(struct record *rec, int nr_threads)
>>> +{
>>> + int t;
>>> +
>>> + if (rec->thread_masks)
>>> + for (t = 0; t < nr_threads; t++)
>>> + record__thread_mask_free(&rec->thread_masks[t]);
>>> +
>>> + zfree(&rec->thread_masks);
>>> +}
>>> +
>>> +static int record__alloc_thread_masks(struct record *rec, int nr_threads, int nr_bits)
>>> +{
>>> + int t, ret;
>>> +
>>> + rec->thread_masks = zalloc(nr_threads * sizeof(*(rec->thread_masks)));
>>> + if (!rec->thread_masks) {
>>> + pr_err("Failed to allocate thread masks\n");
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + for (t = 0; t < nr_threads; t++) {
>>> + ret = record__thread_mask_alloc(&rec->thread_masks[t], nr_bits);
>>> + if (ret) {
>>> + pr_err("Failed to allocate thread masks[%d]\n", t);
>>> + goto out_free;
>>> + }
>>> + }
>>> +
>>> + return 0;
>>> +
>>> +out_free:
>>> + record__free_thread_masks(rec, nr_threads);
>>> +
>>> + return ret;
>>> +}
>>> +
>>> +static int record__init_thread_default_masks(struct record *rec, struct perf_cpu_map *cpus)
>>> +{
>>> + int ret;
>>> +
>>> + ret = record__alloc_thread_masks(rec, 1, cpu__max_cpu().cpu);
>>> + if (ret)
>>> + return ret;
>>> +
>>> + record__mmap_cpu_mask_init(&rec->thread_masks->maps, cpus);
>>> +
>>> + rec->nr_threads = 1;
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int record__init_thread_masks(struct record *rec)
>>> +{
>>> + struct perf_cpu_map *cpus = rec->evlist->core.cpus;
>>> +
>>> + return record__init_thread_default_masks(rec, cpus);
>>> +}
>>> +
>>> int cmd_record(int argc, const char **argv)
>>> {
>>> int err;
>>> @@ -2948,6 +3063,12 @@ int cmd_record(int argc, const char **argv)
>>> goto out;
>>> }
>>>
>>> + err = record__init_thread_masks(rec);
>>> + if (err) {
>>> + pr_err("Failed to initialize parallel data streaming masks\n");
>>> + goto out;
>>> + }
>>> +
>>> if (rec->opts.nr_cblocks > nr_cblocks_max)
>>> rec->opts.nr_cblocks = nr_cblocks_max;
>>> pr_debug("nr_cblocks: %d\n", rec->opts.nr_cblocks);
>>> @@ -2966,6 +3087,8 @@ int cmd_record(int argc, const char **argv)
>>> symbol__exit();
>>> auxtrace_record__free(rec->itr);
>>> out_opts:
>>> + record__free_thread_masks(rec, rec->nr_threads);
>>> + rec->nr_threads = 0;
>>> evlist__close_control(rec->opts.ctl_fd, rec->opts.ctl_fd_ack, &rec->opts.ctl_fd_close);
>>> return err;
>>> }
>>> --
>>> 2.19.0
>>
>> --
>>
>> - Arnaldo
>

Next message: Geert Uytterhoeven: "Re: [PATCH 0/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays"
Previous message: Javier Martinez Canillas: "Re: [PATCH 3/4] drm/tiny: Add driver for Solomon SSD1307 OLED displays"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]