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;
From:   Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>
Date:   Thu, 31 Oct 2019 14:59:02 -0300
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>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@redhat.com>,
        Namhyung Kim <namhyung@kernel.org>,
        Stephane Eranian <eranian@google.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] perf inject --jit: Remove //anon mmap events
Message-ID: <20191031175902.GB15593@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1572483912-111747-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

Em Wed, Oct 30, 2019 at 06:05:12PM -0700, Steve MacLean escreveu:
> From: Steve MacLean <Steve.MacLean@Microsoft.com>
> @@ -749,6 +750,34 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	return 0;
>  }
>  
> +static void jit_add_pid(struct machine *machine, pid_t pid)
> +{
> +	struct thread *thread = machine__findnew_thread(machine, pid, pid);
> +
> +	if (!thread)
> +	{
> +		pr_err("jit_add_pid() thread not found\n");
> +
> +		return;
> +	}


Can you please follow the coding style used in tools/? Its mostly the
same as for the kernel sources.

I.e. above you need to have it as:

	if (!thread) {
		pr_err("jit_add_pid() thread not found\n");
		return;
	}

There also consider using __funct__ and showing the pid that wasn't
found.

> +
> +	thread->priv = (void *) 1;

No space after the cast.

> +}
> +
> +static bool jit_has_pid(struct machine *machine, pid_t pid)
> +{
> +	struct thread *thread = machine__findnew_thread(machine, pid, pid);
> +
> +	if (!thread)
> +	{
> +		pr_err("jit_has_pid() thread not found\n");
> +
> +		return 0;
> +	}
> +
> +	return (bool) thread->priv;

Same style problems and the only way for machine__findnew_thread() to
fail is if the system is under severe memory exhaustion, what you
probably want to use here is machine__find_thread(), that will fail if
the pid isn't found, the findnew methods, in the machine class or
elsewhere will return an existing thread _or_ create and insert it, then
return the newly added/inserted object.

> +}
> +
>  int
>  jit_process(struct perf_session *session,
>  	    struct perf_data *output,
> @@ -765,7 +794,15 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  	 * first, detect marker mmap (i.e., the jitdump mmap)
>  	 */
>  	if (jit_detect(filename, pid))
> +	{

if () {

> +		/*
> +		 * Strip //anon* mmaps if we processed a jitdump for this pid
> +		 */

// can be used, not strictly required tho, the way you used is
acceptable.

> +		if (jit_has_pid(machine, pid) && (strncmp(filename, "//anon", 6) == 0))
> +			return 1;
> +
>  		return 0;
> +	}
>  
>  	memset(&jd, 0, sizeof(jd));
>  
> @@ -784,6 +821,7 @@ static int jit_repipe_debug_info(struct jit_buf_desc *jd, union jr_entry *jr)
>  
>  	ret = jit_inject(&jd, filename);
>  	if (!ret) {
> +		jit_add_pid(machine, pid);
>  		*nbytes = jd.bytes_written;
>  		ret = 1;
>  	}