Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 3 Oct 2019 12:57:16 +0200
From:   Jiri Olsa <jolsa@redhat.com>
To:     Steve MacLean <steve.maclean@linux.microsoft.com>
Cc:     Steve MacLean <Steve.MacLean@microsoft.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Stephane Eranian <eranian@google.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 3/4] perf inject --jit: Remove //anon mmap events
Message-ID: <20191003105716.GB23291@krava>
References: <1570049901-115628-1-git-send-email-steve.maclean@linux.microsoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1570049901-115628-1-git-send-email-steve.maclean@linux.microsoft.com>
User-Agent: Mutt/1.12.1 (2019-06-15)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Oct 02, 2019 at 01:58:21PM -0700, Steve MacLean wrote:
> From: Steve MacLean <Steve.MacLean@Microsoft.com>
> 
> While a JIT is jitting code it will eventually need to commit more pages and
> change these pages to executable permissions.
> 
> Typically the JIT will want these colocated to minimize branch displacements.
> 
> The kernel will coalesce these anonymous mapping with identical permissions
> before sending an MMAP event for the new pages. This means the mmap event for
> the new pages will include the older pages.
> 
> These anonymous mmap events will obscure the jitdump injected pseudo events.
> This means that the jitdump generated symbols, machine code, debugging info,
> and unwind info will no longer be used.
> 
> Observations:
> 
> When a process emits a jit dump marker and a jitdump file, the perf-xxx.map
> file represents inferior information which has been superseded by the
> jitdump jit-xxx.dump file.
> 
> Further the '//anon*' mmap events are only required for the legacy
> perf-xxx.map mapping.

it's been some time I saw the code, Stephane?

I added some comments below

> 
> Summary:
> 
> Add rbtree to track which pids have successfully injected a jitdump file.
> 
> During "perf inject --jit", discard "//anon*" mmap events for any pid which
> has successfully processed a jitdump file.
> 
> Committer testing:
> 
> // jitdump case
> perf record <app with jitdump>
> perf inject --jit --input perf.data --output perfjit.data
> 
> // verify mmap "//anon" events present initially
> perf script --input perf.data --show-mmap-events | grep '//anon'
> // verify mmap "//anon" events removed
> perf script --input perfjit.data --show-mmap-events | grep '//anon'
> 
> // no jitdump case
> perf record <app without jitdump>
> perf inject --jit --input perf.data --output perfjit.data
> 
> // verify mmap "//anon" events present initially
> perf script --input perf.data --show-mmap-events | grep '//anon'
> // verify mmap "//anon" events not removed
> perf script --input perfjit.data --show-mmap-events | grep '//anon'
> 
> Repro:
> 
> This issue was discovered while testing the initial CoreCLR jitdump
> implementation. https://github.com/dotnet/coreclr/pull/26897.
> 
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Steve MacLean <Steve.MacLean@Microsoft.com>
> ---
>  tools/perf/builtin-inject.c |  4 +--
>  tools/perf/util/jitdump.c   | 63 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index c14f40b8..4c921e0 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -261,7 +261,7 @@ static int perf_event__jit_repipe_mmap(struct perf_tool *tool,
>  	 * if jit marker, then inject jit mmaps and generate ELF images
>  	 */
>  	ret = jit_process(inject->session, &inject->output, machine,
> -			  event->mmap.filename, sample->pid, &n);
> +			  event->mmap.filename, event->mmap.pid, &n);
>  	if (ret < 0)
>  		return ret;
>  	if (ret) {
> @@ -299,7 +299,7 @@ static int perf_event__jit_repipe_mmap2(struct perf_tool *tool,
>  	 * if jit marker, then inject jit mmaps and generate ELF images
>  	 */
>  	ret = jit_process(inject->session, &inject->output, machine,
> -			  event->mmap2.filename, sample->pid, &n);
> +			  event->mmap2.filename, event->mmap2.pid, &n);
>  	if (ret < 0)
>  		return ret;
>  	if (ret) {
> diff --git a/tools/perf/util/jitdump.c b/tools/perf/util/jitdump.c
> index 22d09c4..6a1563f 100644
> --- a/tools/perf/util/jitdump.c
> +++ b/tools/perf/util/jitdump.c
> @@ -751,6 +751,59 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	return 0;
>  }
>  
> +struct pid_rbtree
> +{
> +	struct rb_node node;
> +	pid_t pid;
> +};
> +
> +static void jit_add_pid(struct rb_root *root, pid_t pid)
> +{
> +	struct rb_node **new = &(root->rb_node), *parent = NULL;

we don't use the parenthesis like that

> +	struct pid_rbtree* data = NULL;
> +
> +	/* Figure out where to put new node */
> +	while (*new) {
> +		struct pid_rbtree *this = container_of(*new, struct pid_rbtree, node);

there's rb_entry macro for this

> +		pid_t nodePid = this->pid;
> +
> +		parent = *new;
> +		if (pid < nodePid)
> +			new = &((*new)->rb_left);
> +		else if (pid > nodePid)
> +			new = &((*new)->rb_right);
> +		else
> +			return;
> +	}
> +
> +	data = malloc(sizeof(struct pid_rbtree));

plz check every allocation

anyway, I wonder if you could just use thread::priv flag for that, like:

  thread = machine__findnew_thread(machine, pid, pid);
  if (!thread)
    bad

  (int) thread->priv = 1;

and check on thread->priv when ruling the pid out, should be faster
then maintain rb tree

thanks,
jirka