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;
Message-ID: <3b297a17f935d2a00bfa74afbbf064b01fe83607.camel@gmail.com>
Subject: Re: [PATCH v6 03/20] perf record: Introduce thread local variable
From:   Riccardo Mancini <rickyman7@gmail.com>
To:     Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
Cc:     Jiri Olsa <jolsa@redhat.com>, Namhyung Kim <namhyung@kernel.org>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        Andi Kleen <ak@linux.intel.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        Alexander Antonov <alexander.antonov@linux.intel.com>,
        Alexei Budankov <abudankov@huawei.com>,
        linux-perf-users@vger.kernel.org, Ian Rogers <irogers@google.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>
Date:   Fri, 04 Jun 2021 00:56:48 +0200
In-Reply-To: <c5a046f8bed989e4ede98f1fcdaa9d0b6bf78cac.1622025774.git.alexey.v.bayduraev@linux.intel.com>
References: <cover.1622025774.git.alexey.v.bayduraev@linux.intel.com>
         <c5a046f8bed989e4ede98f1fcdaa9d0b6bf78cac.1622025774.git.alexey.v.bayduraev@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.40.1 (3.40.1-1.fc34) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

thank you very much for your work for adding threading capabilites to perf
record. 
I did some testing on your entire patchset, especially checking for memory
issues using ASan. This is just the first of a couple of emails to point out
some issues I found.
I will also do additional tests in the future.

On Wed, 2021-05-26 at 13:52 +0300, Alexey Bayduraev wrote:
SNIP
> @@ -2220,18 +2275,20 @@ static int __cmd_record(struct record *rec, int argc,
> const char **argv)
>                 goto out_child;
>         }
>  
> -       if (!quiet)
> -               fprintf(stderr, "[ perf record: Woken up %ld times to write data
> ]\n", waking);
> -
>         if (target__none(&rec->opts.target))
>                 record__synthesize_workload(rec, true);
>  
>  out_child:
> +       record__stop_threads(rec, &waking);
> +out_free_threads:
>         record__free_thread_data(rec);
>         evlist__finalize_ctlfd(rec->evlist);
>         record__mmap_read_all(rec, true);
>         record__aio_mmap_read_sync(rec);

record__mmap_read_all should be moved before record__free_thread_data since it
uses the thread_data that's just been freed.
Furthermore, record__mmap_read_all should also be moved before the
out_free_threads label, since it cannot be called unless record__start_threads
succeeded, otherwise thread would be NULL and will cause a segfault (it happens
if there is an error somewhere else in perf, for example).

In my tests the following order works, but it should be double checked for
possible side-effects of this order change.

out_child:
        record__stop_threads(rec, &waking);
        record__mmap_read_all(rec, true);
out_free_threads:
        record__free_thread_data(rec);
        evlist__finalize_ctlfd(rec->evlist);
        record__aio_mmap_read_sync(rec);

Thanks,
Riccardo

> +       if (!quiet)
> +               fprintf(stderr, "[ perf record: Woken up %ld times to write data
> ]\n", waking);
> +
>         if (rec->session->bytes_transferred && rec->session->bytes_compressed) {
>                 ratio = (float)rec->session->bytes_transferred/(float)rec-
> >session->bytes_compressed;
>                 session->header.env.comp_ratio = ratio + 0.5;
SNIP