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

["Bayduraev, Alexey V" <alexey.v.bayduraev@linux.intel.com>]

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@linux.intel.com>
>>> Acked-by: Namhyung Kim <namhyung@gmail.com>
>>> Reviewed-by: Riccardo Mancini <rickyman7@gmail.com>
>>> Tested-by: Riccardo Mancini <rickyman7@gmail.com>
>>> Signed-off-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
>>
>> 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
>