Re: [PATCH v2 3/3] ftrace: add tracepoint for itimer

Thomas Gleixner
2009-07-06T19:17:44Z

On Mon, 6 Jul 2009, Xiao Guangrong wrote:
> +TRACE_EVENT(itimer_state,
> +
> + TP_PROTO(int which, struct itimerval *value, cputime_t expires),
> +
> + TP_ARGS(which, value, expires),
> +
> + TP_STRUCT__entry(
> + __field( int, which )
> + __field( void *, timer )
> + __field( int, state )
> + __field( cputime_t, expires )
> + __field( long, value_sec )
> + __field( long, value_usec )
> + __field( long, interval_sec )
> + __field( long, interval_usec )
> + __string( comm, current->comm )
> + ),
> +
> + TP_fast_assign(
> + __entry->which = which;
> + __entry->timer = which == ITIMER_REAL ?
> + &current->signal->real_timer : NULL;

Why do we need this information at all ? There is no value in the
address of real_timer AFAICT.

> + __entry->state = (value->it_value.tv_sec != 0 ||
> + value->it_value.tv_usec != 0) ? 1 : 0;

Redundant information. You store the values anyway.

> + __entry->expires = which == ITIMER_REAL ? 0 : expires;

Why not initializing "nval" in do_setitimer() to 0 and store it
unconditionally in the trace.

> + __entry->value_sec = value->it_value.tv_sec;
> + __entry->value_usec = value->it_value.tv_usec;
> + __entry->interval_sec = value->it_interval.tv_sec;
> + __entry->interval_usec = value->it_interval.tv_usec;
> + __assign_str(comm, current->comm);

The tracer is recording current->comm already, so where is the value
of another instance ?

> +TRACE_EVENT(itimer_expire,
> +
> + TP_PROTO(int which, struct task_struct *task, struct pid *pid, cputime_t now),
> +
> + TP_ARGS(which, task, pid, now),
> +
> + TP_STRUCT__entry(
> + __field( int , which )
> + __field( cputime_t, now )
> + __array( char, comm, TASK_COMM_LEN )
> + ),
> +
> + TP_fast_assign(
> + __entry->which = which;
> + __entry->now = now;
> + if (pid)
> + strcpy(__entry->comm, pid_task(pid, PIDTYPE_PID)->comm);
> + else
> + strcpy(__entry->comm, task->comm);

It's sufficient to store task->pid.

Thanks,

tglx