Re: [PATCH] Perf: bug fix: distinguish between rename and exec

From: Luigi Semenzato
Date: Wed Feb 15 2012 - 12:22:51 EST


On Wed, Feb 15, 2012 at 8:57 AM, David Ahern <dsahern@xxxxxxxxx> wrote:
> On 2/15/12 5:48 AM, Peter Zijlstra wrote:
>>
>> On Mon, 2012-02-13 at 20:56 -0800, Luigi Semenzato wrote:
>>>
>>> This makes it possible for "perf report" to distinguish between an exec
>>> and
>>> a rename (for instance from prctl(PR_SET_NAME)).  Currently a similar
>>> COMM
>>> record is produced for the two events.  Perf report assumes all COMM
>>> records
>>> are execs and discards the old mappings.  Without mappings, perf report
>>> can no longer correlate sampled IPs to the functions containing them,
>>> and collapses all samples into a single bucket.
>>>
>>> This change maintains backward compatibility in both directions, i.e. old
>>> version of perf will continue to work on new perf.data files in the same
>>> way, and new versions of perf on old data files.
>>>
>>> Another solution would be to not send COMM records for renames.  Although
>>> it seems reasonable, it might break existing setups.
>>
>>
>> Uhm, didn't you argue its already broken?
>>
>>> +++ b/fs/exec.c
>>> @@ -1052,7 +1052,7 @@ char *get_task_comm(char *buf, struct task_struct
>>> *tsk)
>>>  }
>>>  EXPORT_SYMBOL_GPL(get_task_comm);
>>>
>>> -void set_task_comm(struct task_struct *tsk, char *buf)
>>> +void set_task_comm(struct task_struct *tsk, char *buf, bool is_rename)
>>>  {
>>>        task_lock(tsk);
>>>
>>> @@ -1068,7 +1068,7 @@ void set_task_comm(struct task_struct *tsk, char
>>> *buf)
>>>        wmb();
>>>        strlcpy(tsk->comm, buf, sizeof(tsk->comm));
>>>        task_unlock(tsk);
>>> -       perf_event_comm(tsk);
>>> +       perf_event_comm(tsk, is_rename);
>>>  }
>>
>>
>> I really dislike changing generic code purely for the purpose of
>> instrumentation like this. Better to pull perf_event_comm() out of here
>> if you want to change semantics.
>>
>> Personally I couldn't care less about renames, I think they're a waste
>> of time, so I'm ok with the simple patch moving the perf_event_comm()
>> into setup_new_exec() and possibly renaming it to perf_event_exec().
>>
>> Acme, do you care about renames?
>>
>
> I'm not Acme, but I do care. We use a lot of processes with named threads
> that give users an idea about the function of a particular thread.
>
> I do not understand the use case targeted by the patch -- what kind of
> processes run for some amount of time and then decide to change task names?

Any process that makes a prctl(PR_SET_NAME) call loses its mappings,
no matter when makes the call. The perf records for that process look
like this:

COMM (for the initial exec)
MMAP (the executable)
MMAP (1st dll)
MMAP (2nd dll)
...
COMM (for the prctl)

The second COMM flushes the old mappings, and all samples from then on
cannot be classified. This is easily reproducible with a small
program, which I would be happy to send.

I found this while trying to use perf with Chrome on Chrome OS.
Chrome forks and execs all the time, and calls prctl() in the thread
library.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/