Re: [PATCH 2/6] tracing: Replace memcpy() with __get_task_comm()

[Steven Rostedt <rostedt@xxxxxxxxxxx>]

On Sun,  2 Jun 2024 10:37:50 +0800
Yafang Shao <laoar.shao@xxxxxxxxx> wrote:

> Using __get_task_comm() to read the task comm ensures that the name is
> always NUL-terminated, regardless of the source string. This approach also
> facilitates future extensions to the task comm.
> 
> Signed-off-by: Yafang Shao <laoar.shao@xxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Masami Hiramatsu <mhiramat@xxxxxxxxxx>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> ---
>  include/linux/tracepoint.h     |  4 ++--
>  include/trace/events/block.h   | 10 +++++-----
>  include/trace/events/oom.h     |  2 +-
>  include/trace/events/osnoise.h |  2 +-
>  include/trace/events/sched.h   | 27 ++++++++++++++-------------
>  include/trace/events/signal.h  |  2 +-
>  include/trace/events/task.h    |  4 ++--
>  7 files changed, 26 insertions(+), 25 deletions(-)
> 
[..]

> diff --git a/include/trace/events/sched.h b/include/trace/events/sched.h
> index 68973f650c26..2a9d7c62c58a 100644
> --- a/include/trace/events/sched.h
> +++ b/include/trace/events/sched.h
> @@ -9,6 +9,7 @@
>  #include <linux/sched/numa_balancing.h>
>  #include <linux/tracepoint.h>
>  #include <linux/binfmts.h>
> +#include <linux/sched.h>
>  
>  /*
>   * Tracepoint for calling kthread_stop, performed to end a kthread:
> @@ -25,7 +26,7 @@ TRACE_EVENT(sched_kthread_stop,
>  	),
>  
>  	TP_fast_assign(
> -		memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
> +		__get_task_comm(__entry->comm, TASK_COMM_LEN, t);
>  		__entry->pid	= t->pid;
>  	),
>  
> @@ -152,7 +153,7 @@ DECLARE_EVENT_CLASS(sched_wakeup_template,
>  	),
>  
>  	TP_fast_assign(
> -		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
> +		__get_task_comm(__entry->comm, TASK_COMM_LEN, p);
>  		__entry->pid		= p->pid;
>  		__entry->prio		= p->prio; /* XXX SCHED_DEADLINE */
>  		__entry->target_cpu	= task_cpu(p);


> @@ -239,11 +240,11 @@ TRACE_EVENT(sched_switch,
>  	),
>  
>  	TP_fast_assign(
> -		memcpy(__entry->next_comm, next->comm, TASK_COMM_LEN);
> +		__get_task_comm(__entry->next_comm, TASK_COMM_LEN, next);
>  		__entry->prev_pid	= prev->pid;
>  		__entry->prev_prio	= prev->prio;
>  		__entry->prev_state	= __trace_sched_switch_state(preempt, prev_state, prev);
> -		memcpy(__entry->prev_comm, prev->comm, TASK_COMM_LEN);
> +		__get_task_comm(__entry->prev_comm, TASK_COMM_LEN, prev);
>  		__entry->next_pid	= next->pid;
>  		__entry->next_prio	= next->prio;
>  		/* XXX SCHED_DEADLINE */

sched_switch is special so we could probably hold off, but the rest should
be converted to the normal way strings are processed in trace events. That
is, to use __string(), __assign_str() and __get_str() for task->comm. I've
been wanting to do that for a while, but thought that memcpy() was a bit
faster than the need for strlen(). But this now needs to test the length of
comm. This method will also allow comms to be recorded that are larger than
16 bytes (if we extend comm).

TRACE_EVENT(sched_migrate_task,

	TP_PROTO(struct task_struct *p, int dest_cpu),

	TP_ARGS(p, dest_cpu),

	TP_STRUCT__entry(
-		__array(	char,	comm,	TASK_COMM_LEN	)
+		__string(	comm,	strlen(comm)		)
		__field(	pid_t,	pid			)
		__field(	int,	prio			)
		__field(	int,	orig_cpu		)
		__field(	int,	dest_cpu		)
	),

	TP_fast_assign(
-		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
+		__assign_str(comm);
		__entry->pid		= p->pid;
		__entry->prio		= p->prio; /* XXX SCHED_DEADLINE */
		__entry->orig_cpu	= task_cpu(p);
		__entry->dest_cpu	= dest_cpu;
	),

	TP_printk("comm=%s pid=%d prio=%d orig_cpu=%d dest_cpu=%d",
-		  __entry->comm, __entry->pid, __entry->prio,
+		  __get_str(comm), __entry->pid, __entry->prio,
		  __entry->orig_cpu, __entry->dest_cpu)
);

-- Steve


> @@ -286,7 +287,7 @@ TRACE_EVENT(sched_migrate_task,
>  	),
>  
>  	TP_fast_assign(
> -		memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
> +		__get_task_comm(__entry->comm, TASK_COMM_LEN, p);
>  		__entry->pid		= p->pid;
>  		__entry->prio		= p->prio; /* XXX SCHED_DEADLINE */
>  		__entry->orig_cpu	= task_cpu(p);