Re: [RFC PATCH 1/3] sched, tracing: report task state in symbolic chars instead
From: Steven Rostedt <rostedt@goodmis.org>
Date: 2023-07-25 17:59:47
Also in:
linux-perf-users, lkml
On Tue, 25 Jul 2023 15:22:52 +0800 Ze Gao [off-list ref] wrote:
#ifdef CREATE_TRACE_POINTS -static inline long __trace_sched_switch_state(bool preempt, +static inline const char __trace_sched_switch_state(bool preempt,
Does it really need to be "const"?
quoted hunk ↗ jump to hunk
unsigned int prev_state, struct task_struct *p) {@@ -197,13 +198,6 @@ static inline long __trace_sched_switch_state(bool preempt, BUG_ON(p != current); #endif /* CONFIG_SCHED_DEBUG */ - /* - * Preemption ignores task state, therefore preempted tasks are always - * RUNNING (we will not have dequeued if state != RUNNING). - */ - if (preempt) - return TASK_REPORT_MAX; - /* * task_state_index() uses fls() and returns a value from 0-8 range. * Decrement it by 1 (except TASK_RUNNING state i.e 0) before using@@ -212,7 +206,16 @@ static inline long __trace_sched_switch_state(bool preempt, */ state = __task_state_index(prev_state, p->exit_state); - return state ? (1 << (state - 1)) : state; + /* + * Preemption ignores task state, therefore preempted tasks are always + * RUNNING (we will not have dequeued if state != RUNNING). + * Here, we use 'p' to denote this case and only for this case. + */ + if (preempt) + return 'p'; + + + return task_index_to_char(state); } #endif /* CREATE_TRACE_POINTS */@@ -232,7 +235,7 @@ TRACE_EVENT(sched_switch, __array( char, prev_comm, TASK_COMM_LEN ) __field( pid_t, prev_pid ) __field( int, prev_prio ) - __field( long, prev_state ) + __field( char, prev_state ) __array( char, next_comm, TASK_COMM_LEN ) __field( pid_t, next_pid ) __field( int, next_prio )
This will break userspace. Just because you update libtraceevent doesn't mean that it will get to all the users of it. There's still tools that have the old method hard coded and doesn't use the library. Now, because the old tools still do the parsing of this format, we can add a new field called prev_state_char that will give you this. Now to save space, we should change prev_state to int (can't make it short as there's that test for "+" which does sometimes happen). I believe we can make prev_prio and next prio into shorts (and possibly chars!). -- Steve
quoted hunk ↗ jump to hunk
@@ -249,22 +252,8 @@ TRACE_EVENT(sched_switch, /* XXX SCHED_DEADLINE */ ), - TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%s%s ==> next_comm=%s next_pid=%d next_prio=%d", - __entry->prev_comm, __entry->prev_pid, __entry->prev_prio, - - (__entry->prev_state & (TASK_REPORT_MAX - 1)) ? - __print_flags(__entry->prev_state & (TASK_REPORT_MAX - 1), "|", - { TASK_INTERRUPTIBLE, "S" }, - { TASK_UNINTERRUPTIBLE, "D" }, - { __TASK_STOPPED, "T" }, - { __TASK_TRACED, "t" }, - { EXIT_DEAD, "X" }, - { EXIT_ZOMBIE, "Z" }, - { TASK_PARKED, "P" }, - { TASK_DEAD, "I" }) : - "R", - - __entry->prev_state & TASK_REPORT_MAX ? "+" : "", + TP_printk("prev_comm=%s prev_pid=%d prev_prio=%d prev_state=%c ==> next_comm=%s next_pid=%d next_prio=%d", + __entry->prev_comm, __entry->prev_pid, __entry->prev_prio, __entry->prev_state, __entry->next_comm, __entry->next_pid, __entry->next_prio)