Re: [PATCH v2] perf inject --jit: Remove //anon mmap events

[Arnaldo Carvalho de Melo]

Em Wed, Oct 30, 2019 at 06:05:12PM -0700, Steve MacLean escreveu:
> From: Steve MacLean <Steve.MacLean@xxxxxxxxxxxxx>
> @@ -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;
>  	}