Hi,
The problems of task state report in both libtraceevent
and perf sched has been reported in [1]. In short, they
parsed the wrong state due to relying on the outdated
hardcoded state string to interpret the raw bitmask
from the record, which left the messes to maintain the
backward compatibilities for both tools.
[1] has not managed to make itself into the kernel, the
problems and the solutions are well studied though.
Luckily, as suggested by Steven, perf/libtraceevent
records the print format, especially the __print_flags()
part of the in-kernel tracepoint sched_switch in its
metadata, and we have a chance to build the state str
on the fly by parsing it.
Now that libtraceevent has landed this solution in [2],
we now apply the same idea to perf as well.
Regards,
-- Ze
[1]: https://lore.kernel.org/lkml/[email protected]/
[2]: https://lore.kernel.org/linux-trace-devel/[email protected]/
Ze Gao (4):
perf sched: Sync state char array with the kernel
perf util: Add helpers to parse task state string from libtraceevent
perf util: Add evsel__taskstate() to parse the task state info instead
perf sched: Commit to evsel__taskstate() to parse task state info
tools/perf/builtin-sched.c | 57 +++------------
tools/perf/util/evsel.c | 146 +++++++++++++++++++++++++++++++++++++
tools/perf/util/evsel.h | 1 +
3 files changed, 157 insertions(+), 47 deletions(-)
--
2.41.0
Update state char array to match the latest kernel definitions and
remove unused state mapping macros.
Note this is the preparing patch for get rid of the way to parse
process state from raw bitmask value. Instead we are going to
parse it from the recorded tracepoint print format, and this change
marks why we're doing it.
Signed-off-by: Ze Gao <[email protected]>
---
tools/perf/builtin-sched.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index dd6065afbbaf..ced6fffe8110 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -92,23 +92,12 @@ struct sched_atom {
struct task_desc *wakee;
};
-#define TASK_STATE_TO_CHAR_STR "RSDTtZXxKWP"
+#define TASK_STATE_TO_CHAR_STR "RSDTtXZPI"
/* task state bitmask, copied from include/linux/sched.h */
#define TASK_RUNNING 0
#define TASK_INTERRUPTIBLE 1
#define TASK_UNINTERRUPTIBLE 2
-#define __TASK_STOPPED 4
-#define __TASK_TRACED 8
-/* in tsk->exit_state */
-#define EXIT_DEAD 16
-#define EXIT_ZOMBIE 32
-#define EXIT_TRACE (EXIT_ZOMBIE | EXIT_DEAD)
-/* in tsk->state again */
-#define TASK_DEAD 64
-#define TASK_WAKEKILL 128
-#define TASK_WAKING 256
-#define TASK_PARKED 512
enum thread_state {
THREAD_SLEEPING = 0,
--
2.41.0
Perf uses a hard coded string "RSDTtXZPI" to index the sched_switch
prev_state field raw bitmask value. This works well except for when
the kernel changes this string, in which case this will break again.
Instead we add a new way to parse task state string from tracepoint
print format already recorded by perf, which eliminates the further
dependencies with this hardcode and unmaintainable macro, and this
is exactly what libtraceevent[1] does for now.
So we borrow the print flags parsing logic from libtraceevent[1].
And in get_states(), we walk the print arguments until the
__print_flags() for the target state field is found, and use that to
build the states string for future parsing.
[1]: https://lore.kernel.org/linux-trace-devel/[email protected]/
Cc: Steven Rostedt (Google) <[email protected]>
Signed-off-by: Steven Rostedt (Google) <[email protected]>
Signed-off-by: Ze Gao <[email protected]>
---
tools/perf/util/evsel.c | 112 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 112 insertions(+)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 72a5dfc38d38..98edf42fcf80 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2818,6 +2818,118 @@ u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const
return field ? format_field__intval(field, sample, evsel->needs_swap) : 0;
}
+/*
+ * prev_state is of size long, which is 32 bits on 32 bit architectures.
+ * As it needs to have the same bits for both 32 bit and 64 bit architectures
+ * we can just assume that the flags we care about will all be within
+ * the 32 bits.
+ */
+#define MAX_STATE_BITS 32
+
+static const char *convert_sym(struct tep_print_flag_sym *sym)
+{
+ static char save_states[MAX_STATE_BITS + 1];
+
+ memset(save_states, 0, sizeof(save_states));
+
+ /* This is the flags for the prev_state_field, now make them into a string */
+ for (; sym; sym = sym->next) {
+ long bitmask = strtoul(sym->value, NULL, 0);
+ int i;
+
+ for (i = 0; !(bitmask & 1); i++)
+ bitmask >>= 1;
+
+ if (i >= MAX_STATE_BITS)
+ continue;
+
+ save_states[i] = sym->str[0];
+ }
+
+ return save_states;
+}
+
+static struct tep_print_arg_field *
+find_arg_field(struct tep_format_field *prev_state_field, struct tep_print_arg *arg)
+{
+ struct tep_print_arg_field *field;
+
+ if (!arg)
+ return NULL;
+
+ if (arg->type == TEP_PRINT_FIELD)
+ return &arg->field;
+
+ if (arg->type == TEP_PRINT_OP) {
+ field = find_arg_field(prev_state_field, arg->op.left);
+ if (field && field->field == prev_state_field)
+ return field;
+ field = find_arg_field(prev_state_field, arg->op.right);
+ if (field && field->field == prev_state_field)
+ return field;
+ }
+ return NULL;
+}
+
+static struct tep_print_flag_sym *
+test_flags(struct tep_format_field *prev_state_field, struct tep_print_arg *arg)
+{
+ struct tep_print_arg_field *field;
+
+ field = find_arg_field(prev_state_field, arg->flags.field);
+ if (!field)
+ return NULL;
+
+ return arg->flags.flags;
+}
+
+static struct tep_print_flag_sym *
+search_op(struct tep_format_field *prev_state_field, struct tep_print_arg *arg)
+{
+ struct tep_print_flag_sym *sym = NULL;
+
+ if (!arg)
+ return NULL;
+
+ if (arg->type == TEP_PRINT_OP) {
+ sym = search_op(prev_state_field, arg->op.left);
+ if (sym)
+ return sym;
+
+ sym = search_op(prev_state_field, arg->op.right);
+ if (sym)
+ return sym;
+ } else if (arg->type == TEP_PRINT_FLAGS) {
+ sym = test_flags(prev_state_field, arg);
+ }
+
+ return sym;
+}
+
+static __maybe_unused const char *get_states(struct tep_format_field *prev_state_field)
+{
+ struct tep_print_flag_sym *sym;
+ struct tep_print_arg *arg;
+ struct tep_event *event;
+
+ event = prev_state_field->event;
+
+ /*
+ * Look at the event format fields, and search for where
+ * the prev_state is parsed via the format flags.
+ */
+ for (arg = event->print_fmt.args; arg; arg = arg->next) {
+ /*
+ * Currently, the __print_flags() for the prev_state
+ * is embedded in operations, so they too must be
+ * searched.
+ */
+ sym = search_op(prev_state_field, arg);
+ if (sym)
+ return convert_sym(sym);
+ }
+ return NULL;
+}
#endif
bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize)
--
2.41.0
Now that we have the __prinf_flags() parsing routines, we add a new
helper evsel__taskstate() to extract the task state info from the
recorded data.
Signed-off-by: Ze Gao <[email protected]>
---
tools/perf/util/evsel.c | 36 +++++++++++++++++++++++++++++++++++-
tools/perf/util/evsel.h | 1 +
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 98edf42fcf80..bc256bdc58d9 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -2906,7 +2906,7 @@ search_op(struct tep_format_field *prev_state_field, struct tep_print_arg *arg)
return sym;
}
-static __maybe_unused const char *get_states(struct tep_format_field *prev_state_field)
+static const char *get_states(struct tep_format_field *prev_state_field)
{
struct tep_print_flag_sym *sym;
struct tep_print_arg *arg;
@@ -2930,6 +2930,40 @@ static __maybe_unused const char *get_states(struct tep_format_field *prev_state
}
return NULL;
}
+
+char evsel__taskstate(struct evsel *evsel, struct perf_sample *sample, const char *name)
+{
+ static struct tep_format_field *prev_state_field;
+ static const char *states;
+ struct tep_format_field *field;
+ unsigned long long val;
+ unsigned int bit;
+ char state = '?'; /* '?' denotes unknown task state */
+
+ field = evsel__field(evsel, name);
+
+ if (!field)
+ return state;
+
+ if (!states || field != prev_state_field) {
+ states = get_states(field);
+ if (!states)
+ return state;
+ prev_state_field = field;
+ }
+
+ /*
+ * Note since the kernel exposes TASK_REPORT_MAX to userspace
+ * to denote the 'preempted' state, we might as welll report
+ * 'R' for this case, which make senses to users as well.
+ *
+ * We can change this if we have a good reason in the future.
+ */
+ val = evsel__intval(evsel, sample, name);
+ bit = val ? ffs(val) : 0;
+ state = (!bit || bit > strlen(states)) ? 'R' : states[bit-1];
+ return state;
+}
#endif
bool evsel__fallback(struct evsel *evsel, int err, char *msg, size_t msgsize)
diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
index d791316a1792..55a7a6210513 100644
--- a/tools/perf/util/evsel.h
+++ b/tools/perf/util/evsel.h
@@ -338,6 +338,7 @@ struct perf_sample;
void *evsel__rawptr(struct evsel *evsel, struct perf_sample *sample, const char *name);
u64 evsel__intval(struct evsel *evsel, struct perf_sample *sample, const char *name);
u64 evsel__intval_common(struct evsel *evsel, struct perf_sample *sample, const char *name);
+char evsel__taskstate(struct evsel *evsel, struct perf_sample *sample, const char *name);
static inline char *evsel__strval(struct evsel *evsel, struct perf_sample *sample, const char *name)
{
--
2.41.0
Now that we have evsel__taskstate() which no longer relies on the
hardcoded task state string and has good backward compatibility,
we have a good reason to use it.
Note TASK_STATE_TO_CHAR_STR and task bitmasks are useless now so
we remove them for good. And now we pass the state info back and
forth in a symbolic char which explains itself well instead.
Signed-off-by: Ze Gao <[email protected]>
---
tools/perf/builtin-sched.c | 46 +++++++++-----------------------------
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/tools/perf/builtin-sched.c b/tools/perf/builtin-sched.c
index ced6fffe8110..f1d62f6b6cab 100644
--- a/tools/perf/builtin-sched.c
+++ b/tools/perf/builtin-sched.c
@@ -92,13 +92,6 @@ struct sched_atom {
struct task_desc *wakee;
};
-#define TASK_STATE_TO_CHAR_STR "RSDTtXZPI"
-
-/* task state bitmask, copied from include/linux/sched.h */
-#define TASK_RUNNING 0
-#define TASK_INTERRUPTIBLE 1
-#define TASK_UNINTERRUPTIBLE 2
-
enum thread_state {
THREAD_SLEEPING = 0,
THREAD_WAIT_CPU,
@@ -255,7 +248,7 @@ struct thread_runtime {
u64 total_preempt_time;
u64 total_delay_time;
- int last_state;
+ char last_state;
char shortname[3];
bool comm_changed;
@@ -425,7 +418,7 @@ static void add_sched_event_wakeup(struct perf_sched *sched, struct task_desc *t
}
static void add_sched_event_sleep(struct perf_sched *sched, struct task_desc *task,
- u64 timestamp, u64 task_state __maybe_unused)
+ u64 timestamp, const char task_state __maybe_unused)
{
struct sched_atom *event = get_new_event(task, timestamp);
@@ -849,7 +842,7 @@ static int replay_switch_event(struct perf_sched *sched,
*next_comm = evsel__strval(evsel, sample, "next_comm");
const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
next_pid = evsel__intval(evsel, sample, "next_pid");
- const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
+ const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
struct task_desc *prev, __maybe_unused *next;
u64 timestamp0, timestamp = sample->time;
int cpu = sample->cpu;
@@ -1039,13 +1032,6 @@ static int thread_atoms_insert(struct perf_sched *sched, struct thread *thread)
return 0;
}
-static char sched_out_state(u64 prev_state)
-{
- const char *str = TASK_STATE_TO_CHAR_STR;
-
- return str[prev_state];
-}
-
static int
add_sched_out_event(struct work_atoms *atoms,
char run_state,
@@ -1121,7 +1107,7 @@ static int latency_switch_event(struct perf_sched *sched,
{
const u32 prev_pid = evsel__intval(evsel, sample, "prev_pid"),
next_pid = evsel__intval(evsel, sample, "next_pid");
- const u64 prev_state = evsel__intval(evsel, sample, "prev_state");
+ const char prev_state = evsel__taskstate(evsel, sample, "prev_state");
struct work_atoms *out_events, *in_events;
struct thread *sched_out, *sched_in;
u64 timestamp0, timestamp = sample->time;
@@ -1157,7 +1143,7 @@ static int latency_switch_event(struct perf_sched *sched,
goto out_put;
}
}
- if (add_sched_out_event(out_events, sched_out_state(prev_state), timestamp))
+ if (add_sched_out_event(out_events, prev_state, timestamp))
return -1;
in_events = thread_atoms_search(&sched->atom_root, sched_in, &sched->cmp_pid);
@@ -2022,24 +2008,12 @@ static void timehist_header(struct perf_sched *sched)
printf("\n");
}
-static char task_state_char(struct thread *thread, int state)
-{
- static const char state_to_char[] = TASK_STATE_TO_CHAR_STR;
- unsigned bit = state ? ffs(state) : 0;
-
- /* 'I' for idle */
- if (thread__tid(thread) == 0)
- return 'I';
-
- return bit < sizeof(state_to_char) - 1 ? state_to_char[bit] : '?';
-}
-
static void timehist_print_sample(struct perf_sched *sched,
struct evsel *evsel,
struct perf_sample *sample,
struct addr_location *al,
struct thread *thread,
- u64 t, int state)
+ u64 t, const char state)
{
struct thread_runtime *tr = thread__priv(thread);
const char *next_comm = evsel__strval(evsel, sample, "next_comm");
@@ -2080,7 +2054,7 @@ static void timehist_print_sample(struct perf_sched *sched,
print_sched_time(tr->dt_run, 6);
if (sched->show_state)
- printf(" %5c ", task_state_char(thread, state));
+ printf(" %5c ", thread->tid == 0 ? 'I' : state);
if (sched->show_next) {
snprintf(nstr, sizeof(nstr), "next: %s[%d]", next_comm, next_pid);
@@ -2152,9 +2126,9 @@ static void timehist_update_runtime_stats(struct thread_runtime *r,
else if (r->last_time) {
u64 dt_wait = tprev - r->last_time;
- if (r->last_state == TASK_RUNNING)
+ if (r->last_state == 'R')
r->dt_preempt = dt_wait;
- else if (r->last_state == TASK_UNINTERRUPTIBLE)
+ else if (r->last_state == 'D')
r->dt_iowait = dt_wait;
else
r->dt_sleep = dt_wait;
@@ -2579,7 +2553,7 @@ static int timehist_sched_change_event(struct perf_tool *tool,
struct thread_runtime *tr = NULL;
u64 tprev, t = sample->time;
int rc = 0;
- int state = evsel__intval(evsel, sample, "prev_state");
+ const char state = evsel__taskstate(evsel, sample, "prev_state");
addr_location__init(&al);
if (machine__resolve(machine, &al, sample) < 0) {
--
2.41.0
Hello,
On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
>
> Hi,
>
> The problems of task state report in both libtraceevent
> and perf sched has been reported in [1]. In short, they
> parsed the wrong state due to relying on the outdated
> hardcoded state string to interpret the raw bitmask
> from the record, which left the messes to maintain the
> backward compatibilities for both tools.
>
> [1] has not managed to make itself into the kernel, the
> problems and the solutions are well studied though.
>
> Luckily, as suggested by Steven, perf/libtraceevent
> records the print format, especially the __print_flags()
> part of the in-kernel tracepoint sched_switch in its
> metadata, and we have a chance to build the state str
> on the fly by parsing it.
>
> Now that libtraceevent has landed this solution in [2],
> we now apply the same idea to perf as well.
Thanks for your work. But perf links libtraceevent
conditionally so you need to make sure if it works without
that too.
I think all libtraceevent related stuff should be in the
util/trace-event.c which is included only if the library is
available. Maybe util/trace-event-parse.c is a better
place but then you need to tweak the python-ext-sources
and Makefile.perf for the case it's not available.
Thanks,
Namhyung
>
> Regards,
>
> -- Ze
>
> [1]: https://lore.kernel.org/lkml/[email protected]/
> [2]: https://lore.kernel.org/linux-trace-devel/[email protected]/
>
> Ze Gao (4):
> perf sched: Sync state char array with the kernel
> perf util: Add helpers to parse task state string from libtraceevent
> perf util: Add evsel__taskstate() to parse the task state info instead
> perf sched: Commit to evsel__taskstate() to parse task state info
>
> tools/perf/builtin-sched.c | 57 +++------------
> tools/perf/util/evsel.c | 146 +++++++++++++++++++++++++++++++++++++
> tools/perf/util/evsel.h | 1 +
> 3 files changed, 157 insertions(+), 47 deletions(-)
>
> --
> 2.41.0
>
On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
>
> Hello,
>
> On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> >
> > Hi,
> >
> > The problems of task state report in both libtraceevent
> > and perf sched has been reported in [1]. In short, they
> > parsed the wrong state due to relying on the outdated
> > hardcoded state string to interpret the raw bitmask
> > from the record, which left the messes to maintain the
> > backward compatibilities for both tools.
> >
> > [1] has not managed to make itself into the kernel, the
> > problems and the solutions are well studied though.
> >
> > Luckily, as suggested by Steven, perf/libtraceevent
> > records the print format, especially the __print_flags()
> > part of the in-kernel tracepoint sched_switch in its
> > metadata, and we have a chance to build the state str
> > on the fly by parsing it.
> >
> > Now that libtraceevent has landed this solution in [2],
> > we now apply the same idea to perf as well.
>
> Thanks for your work. But perf links libtraceevent
> conditionally so you need to make sure if it works without
> that too.
Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
out perf removes perf sched subcmd without libtraceevent,
which explains why the compiler does not complain no
evsel__intval() defined when !HAVE_LIBTRACEEVENT
given the fact so many references of evsel__intval() in
builtin-sched.c.
Here evsel__taskstate() uses the exact assumption as
evsel__intval(), so I put it next to it for clarity and it works
without a doubt.
> I think all libtraceevent related stuff should be in the
> util/trace-event.c which is included only if the library is
> available. Maybe util/trace-event-parse.c is a better
> place but then you need to tweak the python-ext-sources
> and Makefile.perf for the case it's not available.
Thanks for pointing this out. I will do the hack if you insist
on this move :D. But I think the current version is clear
enough, otherwise we need to move all the parts guarded
by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
What do you think of it?
Thanks,
-- Ze
> Thanks,
> Namhyung
>
> >
> > Regards,
> >
> > -- Ze
> >
> > [1]: https://lore.kernel.org/lkml/[email protected]/
> > [2]: https://lore.kernel.org/linux-trace-devel/[email protected]/
> >
> > Ze Gao (4):
> > perf sched: Sync state char array with the kernel
> > perf util: Add helpers to parse task state string from libtraceevent
> > perf util: Add evsel__taskstate() to parse the task state info instead
> > perf sched: Commit to evsel__taskstate() to parse task state info
> >
> > tools/perf/builtin-sched.c | 57 +++------------
> > tools/perf/util/evsel.c | 146 +++++++++++++++++++++++++++++++++++++
> > tools/perf/util/evsel.h | 1 +
> > 3 files changed, 157 insertions(+), 47 deletions(-)
> >
> > --
> > 2.41.0
> >
On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
>
> On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
> >
> > Hello,
> >
> > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > The problems of task state report in both libtraceevent
> > > and perf sched has been reported in [1]. In short, they
> > > parsed the wrong state due to relying on the outdated
> > > hardcoded state string to interpret the raw bitmask
> > > from the record, which left the messes to maintain the
> > > backward compatibilities for both tools.
> > >
> > > [1] has not managed to make itself into the kernel, the
> > > problems and the solutions are well studied though.
> > >
> > > Luckily, as suggested by Steven, perf/libtraceevent
> > > records the print format, especially the __print_flags()
> > > part of the in-kernel tracepoint sched_switch in its
> > > metadata, and we have a chance to build the state str
> > > on the fly by parsing it.
> > >
> > > Now that libtraceevent has landed this solution in [2],
> > > we now apply the same idea to perf as well.
> >
> > Thanks for your work. But perf links libtraceevent
> > conditionally so you need to make sure if it works without
> > that too.
>
> Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> out perf removes perf sched subcmd without libtraceevent,
FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
from the system") has proved this as well.
Regards,
-- Ze
> which explains why the compiler does not complain no
> evsel__intval() defined when !HAVE_LIBTRACEEVENT
> given the fact so many references of evsel__intval() in
> builtin-sched.c.
> Here evsel__taskstate() uses the exact assumption as
> evsel__intval(), so I put it next to it for clarity and it works
> without a doubt.
>
> > I think all libtraceevent related stuff should be in the
> > util/trace-event.c which is included only if the library is
> > available. Maybe util/trace-event-parse.c is a better
> > place but then you need to tweak the python-ext-sources
> > and Makefile.perf for the case it's not available.
>
> Thanks for pointing this out. I will do the hack if you insist
> on this move :D. But I think the current version is clear
> enough, otherwise we need to move all the parts guarded
> by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> What do you think of it?
>
> Thanks,
> -- Ze
>
> > Thanks,
> > Namhyung
> >
> > >
> > > Regards,
> > >
> > > -- Ze
> > >
> > > [1]: https://lore.kernel.org/lkml/20230803083352.1585-1-zegao@tencentcom/
> > > [2]: https://lore.kernel.org/linux-trace-devel/[email protected]/
> > >
> > > Ze Gao (4):
> > > perf sched: Sync state char array with the kernel
> > > perf util: Add helpers to parse task state string from libtraceevent
> > > perf util: Add evsel__taskstate() to parse the task state info instead
> > > perf sched: Commit to evsel__taskstate() to parse task state info
> > >
> > > tools/perf/builtin-sched.c | 57 +++------------
> > > tools/perf/util/evsel.c | 146 +++++++++++++++++++++++++++++++++++++
> > > tools/perf/util/evsel.h | 1 +
> > > 3 files changed, 157 insertions(+), 47 deletions(-)
> > >
> > > --
> > > 2.41.0
> > >
On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <[email protected]> wrote:
>
> On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
> >
> > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
> > >
> > > Hello,
> > >
> > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > The problems of task state report in both libtraceevent
> > > > and perf sched has been reported in [1]. In short, they
> > > > parsed the wrong state due to relying on the outdated
> > > > hardcoded state string to interpret the raw bitmask
> > > > from the record, which left the messes to maintain the
> > > > backward compatibilities for both tools.
> > > >
> > > > [1] has not managed to make itself into the kernel, the
> > > > problems and the solutions are well studied though.
> > > >
> > > > Luckily, as suggested by Steven, perf/libtraceevent
> > > > records the print format, especially the __print_flags()
> > > > part of the in-kernel tracepoint sched_switch in its
> > > > metadata, and we have a chance to build the state str
> > > > on the fly by parsing it.
> > > >
> > > > Now that libtraceevent has landed this solution in [2],
> > > > we now apply the same idea to perf as well.
> > >
> > > Thanks for your work. But perf links libtraceevent
> > > conditionally so you need to make sure if it works without
> > > that too.
> >
> > Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> > out perf removes perf sched subcmd without libtraceevent,
>
> FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> from the system") has proved this as well.
Right, but I think we can enable perf sched without libtraceevent
for minimal features like record only. But that doesn't belong to
this change set.
>
> > which explains why the compiler does not complain no
> > evsel__intval() defined when !HAVE_LIBTRACEEVENT
> > given the fact so many references of evsel__intval() in
> > builtin-sched.c.
> > Here evsel__taskstate() uses the exact assumption as
> > evsel__intval(), so I put it next to it for clarity and it works
> > without a doubt.
> >
> > > I think all libtraceevent related stuff should be in the
> > > util/trace-event.c which is included only if the library is
> > > available. Maybe util/trace-event-parse.c is a better
> > > place but then you need to tweak the python-ext-sources
> > > and Makefile.perf for the case it's not available.
> >
> > Thanks for pointing this out. I will do the hack if you insist
> > on this move :D. But I think the current version is clear
> > enough, otherwise we need to move all the parts guarded
> > by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> > What do you think of it?
Oh, I realized that all the affected codes are under the #ifdef
properly then maybe it's ok for now. But I prefer moving the
code if you're ok. Maybe I can accept this code as is and you
can work on the refactoring later. Does that work for you?
Thanks,
Namhyung
On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <[email protected]> wrote:
>
> On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <[email protected]> wrote:
> >
> > On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
> > >
> > > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <namhyung@kernelorg> wrote:
> > > >
> > > > Hello,
> > > >
> > > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > The problems of task state report in both libtraceevent
> > > > > and perf sched has been reported in [1]. In short, they
> > > > > parsed the wrong state due to relying on the outdated
> > > > > hardcoded state string to interpret the raw bitmask
> > > > > from the record, which left the messes to maintain the
> > > > > backward compatibilities for both tools.
> > > > >
> > > > > [1] has not managed to make itself into the kernel, the
> > > > > problems and the solutions are well studied though.
> > > > >
> > > > > Luckily, as suggested by Steven, perf/libtraceevent
> > > > > records the print format, especially the __print_flags()
> > > > > part of the in-kernel tracepoint sched_switch in its
> > > > > metadata, and we have a chance to build the state str
> > > > > on the fly by parsing it.
> > > > >
> > > > > Now that libtraceevent has landed this solution in [2],
> > > > > we now apply the same idea to perf as well.
> > > >
> > > > Thanks for your work. But perf links libtraceevent
> > > > conditionally so you need to make sure if it works without
> > > > that too.
> > >
> > > Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> > > out perf removes perf sched subcmd without libtraceevent,
> >
> > FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> > from the system") has proved this as well.
>
> Right, but I think we can enable perf sched without libtraceevent
> for minimal features like record only. But that doesn't belong to
> this change set.
>
> >
> > > which explains why the compiler does not complain no
> > > evsel__intval() defined when !HAVE_LIBTRACEEVENT
> > > given the fact so many references of evsel__intval() in
> > > builtin-sched.c.
> > > Here evsel__taskstate() uses the exact assumption as
> > > evsel__intval(), so I put it next to it for clarity and it works
> > > without a doubt.
> > >
> > > > I think all libtraceevent related stuff should be in the
> > > > util/trace-event.c which is included only if the library is
> > > > available. Maybe util/trace-event-parse.c is a better
> > > > place but then you need to tweak the python-ext-sources
> > > > and Makefile.perf for the case it's not available.
> > >
> > > Thanks for pointing this out. I will do the hack if you insist
> > > on this move :D. But I think the current version is clear
> > > enough, otherwise we need to move all the parts guarded
> > > by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> > > What do you think of it?
>
> Oh, I realized that all the affected codes are under the #ifdef
> properly then maybe it's ok for now. But I prefer moving the
> code if you're ok. Maybe I can accept this code as is and you
Sounds great!
> can work on the refactoring later. Does that work for you?
Absolutely! Will send the following refactoring patches soon. :D
Cheers,
-- Ze
> Thanks,
> Namhyung
On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <[email protected]> wrote:
>
> On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <[email protected]> wrote:
> >
> > On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <[email protected]> wrote:
> > >
> > > On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
> > > >
> > > > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
> > > > >
> > > > > Hello,
> > > > >
> > > > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > The problems of task state report in both libtraceevent
> > > > > > and perf sched has been reported in [1]. In short, they
> > > > > > parsed the wrong state due to relying on the outdated
> > > > > > hardcoded state string to interpret the raw bitmask
> > > > > > from the record, which left the messes to maintain the
> > > > > > backward compatibilities for both tools.
> > > > > >
> > > > > > [1] has not managed to make itself into the kernel, the
> > > > > > problems and the solutions are well studied though.
> > > > > >
> > > > > > Luckily, as suggested by Steven, perf/libtraceevent
> > > > > > records the print format, especially the __print_flags()
> > > > > > part of the in-kernel tracepoint sched_switch in its
> > > > > > metadata, and we have a chance to build the state str
> > > > > > on the fly by parsing it.
> > > > > >
> > > > > > Now that libtraceevent has landed this solution in [2],
> > > > > > we now apply the same idea to perf as well.
> > > > >
> > > > > Thanks for your work. But perf links libtraceevent
> > > > > conditionally so you need to make sure if it works without
> > > > > that too.
> > > >
> > > > Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> > > > out perf removes perf sched subcmd without libtraceevent,
> > >
> > > FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> > > from the system") has proved this as well.
> >
> > Right, but I think we can enable perf sched without libtraceevent
> > for minimal features like record only. But that doesn't belong to
> > this change set.
> >
> > >
> > > > which explains why the compiler does not complain no
> > > > evsel__intval() defined when !HAVE_LIBTRACEEVENT
> > > > given the fact so many references of evsel__intval() in
> > > > builtin-sched.c.
> > > > Here evsel__taskstate() uses the exact assumption as
> > > > evsel__intval(), so I put it next to it for clarity and it works
> > > > without a doubt.
> > > >
> > > > > I think all libtraceevent related stuff should be in the
> > > > > util/trace-event.c which is included only if the library is
> > > > > available. Maybe util/trace-event-parse.c is a better
> > > > > place but then you need to tweak the python-ext-sources
> > > > > and Makefile.perf for the case it's not available.
> > > >
> > > > Thanks for pointing this out. I will do the hack if you insist
> > > > on this move :D. But I think the current version is clear
> > > > enough, otherwise we need to move all the parts guarded
> > > > by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> > > > What do you think of it?
> >
> > Oh, I realized that all the affected codes are under the #ifdef
> > properly then maybe it's ok for now. But I prefer moving the
> > code if you're ok. Maybe I can accept this code as is and you
>
> Sounds great!
>
> > can work on the refactoring later. Does that work for you?
>
> Absolutely! Will send the following refactoring patches soon. :D
Thanks, but your patches don't apply cleanly. Could you please
rebase it onto the current perf-tools-next tree?
Thanks,
Namhyung
On Sat, Jan 20, 2024 at 6:45 AM Namhyung Kim <[email protected]> wrote:
>
> On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <[email protected]> wrote:
> >
> > On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <[email protected]> wrote:
> > >
> > > On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <[email protected]> wrote:
> > > >
> > > > On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
> > > > >
> > > > > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > > The problems of task state report in both libtraceevent
> > > > > > > and perf sched has been reported in [1]. In short, they
> > > > > > > parsed the wrong state due to relying on the outdated
> > > > > > > hardcoded state string to interpret the raw bitmask
> > > > > > > from the record, which left the messes to maintain the
> > > > > > > backward compatibilities for both tools.
> > > > > > >
> > > > > > > [1] has not managed to make itself into the kernel, the
> > > > > > > problems and the solutions are well studied though.
> > > > > > >
> > > > > > > Luckily, as suggested by Steven, perf/libtraceevent
> > > > > > > records the print format, especially the __print_flags()
> > > > > > > part of the in-kernel tracepoint sched_switch in its
> > > > > > > metadata, and we have a chance to build the state str
> > > > > > > on the fly by parsing it.
> > > > > > >
> > > > > > > Now that libtraceevent has landed this solution in [2],
> > > > > > > we now apply the same idea to perf as well.
> > > > > >
> > > > > > Thanks for your work. But perf links libtraceevent
> > > > > > conditionally so you need to make sure if it works without
> > > > > > that too.
> > > > >
> > > > > Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> > > > > out perf removes perf sched subcmd without libtraceevent,
> > > >
> > > > FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> > > > from the system") has proved this as well.
> > >
> > > Right, but I think we can enable perf sched without libtraceevent
> > > for minimal features like record only. But that doesn't belong to
> > > this change set.
> > >
> > > >
> > > > > which explains why the compiler does not complain no
> > > > > evsel__intval() defined when !HAVE_LIBTRACEEVENT
> > > > > given the fact so many references of evsel__intval() in
> > > > > builtin-sched.c.
> > > > > Here evsel__taskstate() uses the exact assumption as
> > > > > evsel__intval(), so I put it next to it for clarity and it works
> > > > > without a doubt.
> > > > >
> > > > > > I think all libtraceevent related stuff should be in the
> > > > > > util/trace-event.c which is included only if the library is
> > > > > > available. Maybe util/trace-event-parse.c is a better
> > > > > > place but then you need to tweak the python-ext-sources
> > > > > > and Makefile.perf for the case it's not available.
> > > > >
> > > > > Thanks for pointing this out. I will do the hack if you insist
> > > > > on this move :D. But I think the current version is clear
> > > > > enough, otherwise we need to move all the parts guarded
> > > > > by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> > > > > What do you think of it?
> > >
> > > Oh, I realized that all the affected codes are under the #ifdef
> > > properly then maybe it's ok for now. But I prefer moving the
> > > code if you're ok. Maybe I can accept this code as is and you
> >
> > Sounds great!
> >
> > > can work on the refactoring later. Does that work for you?
> >
> > Absolutely! Will send the following refactoring patches soon. :D
>
> Thanks, but your patches don't apply cleanly. Could you please
> rebase it onto the current perf-tools-next tree?
Oops, that is kinda weird. I've tested and managed to cherry-picked all 4
patches onto branch perf-tools-next in [1], with no conflicts being
hit. Maybe I used the wrong branch tip?
FWIW: the tip I rebase onto is
d988c9f511af (perf/perf-tools-next) MAINTAINERS: Add Namhyung as
tools/perf/ co-maintainer
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/
Regards,
-- Ze
> Thanks,
> Namhyung
On Mon, Jan 22, 2024 at 11:08 AM Ze Gao <[email protected]> wrote:
>
> On Sat, Jan 20, 2024 at 6:45 AM Namhyung Kim <[email protected]> wrote:
> >
> > On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <[email protected]> wrote:
> > >
> > > On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernelorg> wrote:
> > > >
> > > > On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
> > > > > > >
> > > > > > > Hello,
> > > > > > >
> > > > > > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > The problems of task state report in both libtraceevent
> > > > > > > > and perf sched has been reported in [1]. In short, they
> > > > > > > > parsed the wrong state due to relying on the outdated
> > > > > > > > hardcoded state string to interpret the raw bitmask
> > > > > > > > from the record, which left the messes to maintain the
> > > > > > > > backward compatibilities for both tools.
> > > > > > > >
> > > > > > > > [1] has not managed to make itself into the kernel, the
> > > > > > > > problems and the solutions are well studied though.
> > > > > > > >
> > > > > > > > Luckily, as suggested by Steven, perf/libtraceevent
> > > > > > > > records the print format, especially the __print_flags()
> > > > > > > > part of the in-kernel tracepoint sched_switch in its
> > > > > > > > metadata, and we have a chance to build the state str
> > > > > > > > on the fly by parsing it.
> > > > > > > >
> > > > > > > > Now that libtraceevent has landed this solution in [2],
> > > > > > > > we now apply the same idea to perf as well.
> > > > > > >
> > > > > > > Thanks for your work. But perf links libtraceevent
> > > > > > > conditionally so you need to make sure if it works without
> > > > > > > that too.
> > > > > >
> > > > > > Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> > > > > > out perf removes perf sched subcmd without libtraceevent,
> > > > >
> > > > > FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> > > > > from the system") has proved this as well.
> > > >
> > > > Right, but I think we can enable perf sched without libtraceevent
> > > > for minimal features like record only. But that doesn't belong to
> > > > this change set.
> > > >
> > > > >
> > > > > > which explains why the compiler does not complain no
> > > > > > evsel__intval() defined when !HAVE_LIBTRACEEVENT
> > > > > > given the fact so many references of evsel__intval() in
> > > > > > builtin-sched.c.
> > > > > > Here evsel__taskstate() uses the exact assumption as
> > > > > > evsel__intval(), so I put it next to it for clarity and it works
> > > > > > without a doubt.
> > > > > >
> > > > > > > I think all libtraceevent related stuff should be in the
> > > > > > > util/trace-event.c which is included only if the library is
> > > > > > > available. Maybe util/trace-event-parse.c is a better
> > > > > > > place but then you need to tweak the python-ext-sources
> > > > > > > and Makefile.perf for the case it's not available.
> > > > > >
> > > > > > Thanks for pointing this out. I will do the hack if you insist
> > > > > > on this move :D. But I think the current version is clear
> > > > > > enough, otherwise we need to move all the parts guarded
> > > > > > by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> > > > > > What do you think of it?
> > > >
> > > > Oh, I realized that all the affected codes are under the #ifdef
> > > > properly then maybe it's ok for now. But I prefer moving the
> > > > code if you're ok. Maybe I can accept this code as is and you
> > >
> > > Sounds great!
> > >
> > > > can work on the refactoring later. Does that work for you?
> > >
> > > Absolutely! Will send the following refactoring patches soon. :D
> >
> > Thanks, but your patches don't apply cleanly. Could you please
> > rebase it onto the current perf-tools-next tree?
>
> Oops, that is kinda weird. I've tested and managed to cherry-picked all 4
> patches onto branch perf-tools-next in [1], with no conflicts being
> hit. Maybe I used the wrong branch tip?
For reference, it ends up like:
https://github.com/zegao96/linux/commits/perf-tools-next/
> FWIW: the tip I rebase onto is
>
> d988c9f511af (perf/perf-tools-next) MAINTAINERS: Add Namhyung as
> tools/perf/ co-maintainer
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/
>
> Regards,
> -- Ze
>
> > Thanks,
> > Namhyung
On Mon, Jan 22, 2024 at 11:08 AM Ze Gao <[email protected]> wrote:
>
> On Sat, Jan 20, 2024 at 6:45 AM Namhyung Kim <[email protected]> wrote:
> >
> > On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <[email protected]> wrote:
> > >
> > > On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernelorg> wrote:
> > > >
> > > > On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <[email protected]> wrote:
> > > > >
> > > > > On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <[email protected]> wrote:
> > > > > >
> > > > > > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <[email protected]> wrote:
> > > > > > >
> > > > > > > Hello,
> > > > > > >
> > > > > > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > The problems of task state report in both libtraceevent
> > > > > > > > and perf sched has been reported in [1]. In short, they
> > > > > > > > parsed the wrong state due to relying on the outdated
> > > > > > > > hardcoded state string to interpret the raw bitmask
> > > > > > > > from the record, which left the messes to maintain the
> > > > > > > > backward compatibilities for both tools.
> > > > > > > >
> > > > > > > > [1] has not managed to make itself into the kernel, the
> > > > > > > > problems and the solutions are well studied though.
> > > > > > > >
> > > > > > > > Luckily, as suggested by Steven, perf/libtraceevent
> > > > > > > > records the print format, especially the __print_flags()
> > > > > > > > part of the in-kernel tracepoint sched_switch in its
> > > > > > > > metadata, and we have a chance to build the state str
> > > > > > > > on the fly by parsing it.
> > > > > > > >
> > > > > > > > Now that libtraceevent has landed this solution in [2],
> > > > > > > > we now apply the same idea to perf as well.
> > > > > > >
> > > > > > > Thanks for your work. But perf links libtraceevent
> > > > > > > conditionally so you need to make sure if it works without
> > > > > > > that too.
> > > > > >
> > > > > > Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
> > > > > > out perf removes perf sched subcmd without libtraceevent,
> > > > >
> > > > > FWIW, commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> > > > > from the system") has proved this as well.
> > > >
> > > > Right, but I think we can enable perf sched without libtraceevent
> > > > for minimal features like record only. But that doesn't belong to
> > > > this change set.
> > > >
> > > > >
> > > > > > which explains why the compiler does not complain no
> > > > > > evsel__intval() defined when !HAVE_LIBTRACEEVENT
> > > > > > given the fact so many references of evsel__intval() in
> > > > > > builtin-sched.c.
> > > > > > Here evsel__taskstate() uses the exact assumption as
> > > > > > evsel__intval(), so I put it next to it for clarity and it works
> > > > > > without a doubt.
> > > > > >
> > > > > > > I think all libtraceevent related stuff should be in the
> > > > > > > util/trace-event.c which is included only if the library is
> > > > > > > available. Maybe util/trace-event-parse.c is a better
> > > > > > > place but then you need to tweak the python-ext-sources
> > > > > > > and Makefile.perf for the case it's not available.
> > > > > >
> > > > > > Thanks for pointing this out. I will do the hack if you insist
> > > > > > on this move :D. But I think the current version is clear
> > > > > > enough, otherwise we need to move all the parts guarded
> > > > > > by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
> > > > > > What do you think of it?
> > > >
> > > > Oh, I realized that all the affected codes are under the #ifdef
> > > > properly then maybe it's ok for now. But I prefer moving the
> > > > code if you're ok. Maybe I can accept this code as is and you
> > >
> > > Sounds great!
> > >
> > > > can work on the refactoring later. Does that work for you?
> > >
> > > Absolutely! Will send the following refactoring patches soon. :D
> >
> > Thanks, but your patches don't apply cleanly. Could you please
> > rebase it onto the current perf-tools-next tree?
>
> Oops, that is kinda weird. I've tested and managed to cherry-picked all 4
> patches onto branch perf-tools-next in [1], with no conflicts being
Please forget about this. Looks like git-cherry-pick is smarter to
figure out where to
apply when in the same repo ( or than git-am ?).
Anyway I've resent a v2 series:
https://lore.kernel.org/all/[email protected]/
Tested and verified. Hope this time i don't mess it up :)
Thanks,
-- Ze
> hit. Maybe I used the wrong branch tip?
>
> FWIW: the tip I rebase onto is
>
> d988c9f511af (perf/perf-tools-next) MAINTAINERS: Add Namhyung as
> tools/perf/ co-maintainer
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/
>
> Regards,
> -- Ze
>
> > Thanks,
> > Namhyung