lkml.org 
[lkml]   [2012]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Perf: bug fix: distinguish between rename and exec
    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?

    David


    \
     
     \ /
      Last update: 2012-02-15 17:59    [W:0.025 / U:1.428 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site