linux-kernel - Re: [PATCH v13 05/16] perf record: Introduce thread local variable

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 1 Feb 2022 10:35:16 +0300
From:   "Bayduraev, Alexey V" <alexey.v.bayduraev@...ux.intel.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Jiri Olsa <jolsa@...hat.com>, Namhyung Kim <namhyung@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Alexander Antonov <alexander.antonov@...ux.intel.com>,
        Alexei Budankov <abudankov@...wei.com>,
        Riccardo Mancini <rickyman7@...il.com>
Subject: Re: [PATCH v13 05/16] perf record: Introduce thread local variable

On 01.02.2022 0:45, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jan 17, 2022 at 09:34:25PM +0300, Alexey Bayduraev escreveu:
>> Introduce thread local variable and use it for threaded trace streaming.
>> Use thread affinity mask instead of record affinity mask in affinity
>> modes. Use evlist__ctlfd_update() to propagate control commands from
>> thread object to global evlist object to enable evlist__ctlfd_*
>> functionality. Move waking and sample statistic to struct record_thread
>> and introduce record__waking function to calculate the total number of
>> wakes.

SNIP

>>  	if (record__open(rec) != 0) {
>>  		err = -1;
>> -		goto out_child;
>> +		goto out_free_threads;
>>  	}

SNIP

>>  
>>  out_child:
>> -	evlist__finalize_ctlfd(rec->evlist);
>> +	record__stop_threads(rec);
>>  	record__mmap_read_all(rec, true);
>> +out_free_threads:
>>  	record__free_thread_data(rec);
>> +	evlist__finalize_ctlfd(rec->evlist);
> 
> You changed the calling order, moving evlist__finalize_ctlfd to after
> record__mmap_read_all, is that ok? And if so, should be in a separate
> patch, right?

This is necessary because record__mmap_read_all() must be right after 
record__stop_threads() to prevent data loss, but we must deinitialize
ctlfd after out_free_threads as it was initialized in record__open()

record__mmap_read_all() looks independent of evlist__finalize_ctlfd()
but I think any deinitialization in evlist would be safer after
record__mmap_read_all()

Probably adding such notes to this patch will be enough.

Regards,
Alexey

> 
> - Arnaldo
> 
>>  	record__aio_mmap_read_sync(rec);
>>  
>>  	if (rec->session->bytes_transferred && rec->session->bytes_compressed) {
>> @@ -3164,17 +3224,6 @@ int cmd_record(int argc, const char **argv)
>>  
>>  	symbol__init(NULL);
>>  
>> -	if (rec->opts.affinity != PERF_AFFINITY_SYS) {
>> -		rec->affinity_mask.nbits = cpu__max_cpu().cpu;
>> -		rec->affinity_mask.bits = bitmap_zalloc(rec->affinity_mask.nbits);
>> -		if (!rec->affinity_mask.bits) {
>> -			pr_err("Failed to allocate thread mask for %zd cpus\n", rec->affinity_mask.nbits);
>> -			err = -ENOMEM;
>> -			goto out_opts;
>> -		}
>> -		pr_debug2("thread mask[%zd]: empty\n", rec->affinity_mask.nbits);
>> -	}
>> -
>>  	err = record__auxtrace_init(rec);
>>  	if (err)
>>  		goto out;
>> @@ -3323,7 +3372,6 @@ int cmd_record(int argc, const char **argv)
>>  
>>  	err = __cmd_record(&record, argc, argv);
>>  out:
>> -	bitmap_free(rec->affinity_mask.bits);
>>  	evlist__delete(rec->evlist);
>>  	symbol__exit();
>>  	auxtrace_record__free(rec->itr);
>> -- 
>> 2.19.0
>