This is the 3rd attempt to fix the report task state issue in sched
tracepint, you can check out previous discussions here:
v1: https://lore.kernel.org/linux-trace-kernel/20230725072254.32045-1-zegao@tencent.com
v2: https://lore.kernel.org/linux-trace-kernel/20230726121618.19198-1-zegao@tencent.com
FYI, this series are designed not to break anything now and still do the
1-1 correspondence int-char mapping for each distinct task state we want to
report, and thus will not lose any details intended for debug purposes. Of
course, this might be compromised because of bugs introduced due to my
stupidity. So your sage comments are very important and appreciated!
diff from v2:
1. reorder to condense sched_switch record structure
2. fallback to older method to maintain backward compatibility
for perf/libtraceevent
3. split housekeeping work into separate ones for readability
--
In the status quo, we should see three different outcomes of the reported
sched-out task state from perf-script, perf-sched-timehist, and Tp_printk
of tracepoint sched_switch. And it's not hard to figure out that the
former two are built upon the third one, and the reason why we see this
inconsistency is that the former two does not catch up with the internal
change of reported task state definitions as the kernel evolves.
IMHO, exporting internal representations of task state in the tracepoint
sched_switch is not a good practice and not encouraged at all, which can
easily break userspace tools that relies on it. Especially when tracepoints
are massively used in many observability tools nowadays due to its stable
nature, which makes them no longer used for debug only purpose and we
should be careful to decide what ought to be reported to userspace and what
ought not.
Therefore, to fix the issues mentioned above for good, I proposed to add
a new variable to report task state in sched_switch with a symbolic char
along with the old hardcoded value, and save the further processing of
userspace tools and spare them from knowing implementation details in the
kernel.
After this patch seires, we report 'RSDTtXZPI' the same as in procfs, plus
a 'p' which denotes PREEMP_ACTIVE and is used for sched_switch tracepoint
only.
Reviews welcome!
Regards,
Ze
Ze Gao (5):
perf sched: sync state char array with the kernel
perf sched: reorganize sched-out task state report code
sched, tracing: add to report task state in symbolic chars
sched, tracing: reorganize fields of switch event struct
perf sched: prefer to use prev_state_char introduced in sched_switch
include/trace/events/sched.h | 68 +++++++++++++++++-------------
tools/perf/builtin-sched.c | 82 ++++++++++++++++--------------------
2 files changed, 76 insertions(+), 74 deletions(-)
Ze Gao (1):
libtraceevent: prefer to use prev_state_char introduced in
sched_switch
plugins/plugin_sched_switch.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
--
2.40.1
update state char array and then remove unused and stale
macros, which are kernel internal representations and not
encouraged to use anymore.
this fix is for old kernels, and we will steer to newly
added symbolic char to report task state but also maintain
compatibility to not break anything.
Signed-off-by: Ze Gao <redacted>
---
tools/perf/builtin-sched.c | 13 +------------
1 file changed, 1 insertion(+), 12 deletions(-)
this patch mainly does housekeeping work and not
introduce any functional change.
Signed-off-by: Ze Gao <redacted>
---
tools/perf/builtin-sched.c | 57 ++++++++++++++++----------------------
1 file changed, 24 insertions(+), 33 deletions(-)
Internal representations of task state are likely to be changed
or ordered, and reporting them to userspace without exporting
them as part of API is basically wrong, which can easily break
a userspace observability tool as kernel evolves. For example,
perf suffers from this and still reports wrong states as of this
writing.
OTOH, some masqueraded states like TASK_REPORT_IDLE and
TASK_REPORT_MAX are also reported inadvertently, which confuses
things even more and most userspace tools do not even take them
into consideration.
So add a new variable in company with the old raw value to
report task state in symbolic chars, which are self-explaining
and no further translation is needed. Of course this does not
break any userspace tool.
Note for PREEMPT_ACTIVE, we introduce 'p' to report it and use
the old conventions for the rest.
Signed-off-by: Ze Gao <redacted>
Reviewed-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>
Acked-by: Ian Rogers <irogers@google.com>
---
include/trace/events/sched.h | 54 +++++++++++++++++++++---------------
1 file changed, 32 insertions(+), 22 deletions(-)
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Ze Gao <redacted>
---
include/trace/events/sched.h | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
Since the sched_switch tracepoint introduces a new variable to
report sched-out task state in symbolic char, we prefer to use
it to spare from knowing internal implementations in kernel.
Also we keep the old parsing logic intact but sync the state char
array with the latest kernel.
Signed-off-by: Ze Gao <redacted>
---
tools/perf/builtin-sched.c | 18 +++++++++++++++---
1 file changed, 15 insertions(+), 3 deletions(-)
Since the sched_switch tracepoint introduces a new variable to
report sched-out task state in symbolic char, we prefer to use
it to spare from knowing internal implementations in kernel.
Also we keep the old parsing logic intact but sync the state char
array with the latest kernel.
Signed-off-by: Ze Gao <redacted>
---
plugins/plugin_sched_switch.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -99,7 +99,12 @@ static int sched_switch_handler(struct trace_seq *s,if(tep_get_field_val(s,event,"prev_prio",record,&val,1)==0)trace_seq_printf(s,"[%d] ",(int)val);-if(tep_get_field_val(s,event,"prev_state",record,&val,1)==0)+//find if has prev_state_char, otherwise fallback to prev_state+if(tep_find_field(event,"prev_state_char")){+if(tep_get_field_val(s,event,"prev_state_char",record,&val,1)==0)+trace_seq_putc(s,(char)val);+}+elseif(tep_get_field_val(s,event,"prev_state",record,&val,1)==0)write_state(s,val);trace_seq_puts(s," ==> ");
From: Peter Zijlstra <peterz@infradead.org> Date: 2023-08-01 11:34:38
On Tue, Aug 01, 2023 at 05:01:21PM +0800, Ze Gao wrote:
Internal representations of task state are likely to be changed
or ordered, and reporting them to userspace without exporting
them as part of API is basically wrong, which can easily break
a userspace observability tool as kernel evolves. For example,
perf suffers from this and still reports wrong states as of this
writing.
OTOH, some masqueraded states like TASK_REPORT_IDLE and
TASK_REPORT_MAX are also reported inadvertently, which confuses
things even more and most userspace tools do not even take them
into consideration.
So add a new variable in company with the old raw value to
report task state in symbolic chars, which are self-explaining
and no further translation is needed. Of course this does not
break any userspace tool.
Note for PREEMPT_ACTIVE, we introduce 'p' to report it and use
the old conventions for the rest.
*sigh*... just because userspace if daft, we need to change the kernel?
Why do we need this character anyway, why not just print the state in
hex and leave it at that? These single character state things are a
relic, please just let them die.
From: Peter Zijlstra <peterz@infradead.org> Date: 2023-08-01 11:47:22
On Tue, Aug 01, 2023 at 05:01:22PM +0800, Ze Gao wrote:
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
I don't see a single line describing the effort you've done to audit
consumers of this tracepoint.
*IF* you're wanting to break this tracepoint ABI, because seriously
that's what it is, then you get to invest the time and effort to audit
the users.
On Tue, Aug 1, 2023 at 7:34 PM Peter Zijlstra [off-list ref] wrote:
On Tue, Aug 01, 2023 at 05:01:21PM +0800, Ze Gao wrote:
quoted
Internal representations of task state are likely to be changed
or ordered, and reporting them to userspace without exporting
them as part of API is basically wrong, which can easily break
a userspace observability tool as kernel evolves. For example,
perf suffers from this and still reports wrong states as of this
writing.
OTOH, some masqueraded states like TASK_REPORT_IDLE and
TASK_REPORT_MAX are also reported inadvertently, which confuses
things even more and most userspace tools do not even take them
into consideration.
So add a new variable in company with the old raw value to
report task state in symbolic chars, which are self-explaining
and no further translation is needed. Of course this does not
break any userspace tool.
Note for PREEMPT_ACTIVE, we introduce 'p' to report it and use
the old conventions for the rest.
*sigh*... just because userspace if daft, we need to change the kernel?
Hi Peter,
Sorry that I don't quite agree with you on this one.
It's just the design that exporting internal details is fundamentally wrong.
And even worse, I did not see any userspace tool is aware of masqueraded
states like TASK_REPORT_IDLE and TASK_REPORT_MAX and let alone
parse it correctly. This confused me a lot when I decided to write my own bpf
version of sched-latency analysis tool and only after I figured out everything
underneath, I started to make things right here.
Again, I mean it's not me that deliberately "breaks" ABI here and I am
never meant
to upset anyone. My confusion is why did people forget to update in-tree perf
the very last time they decide to rearrange the task state mapping
since we all agree
this is important "ABI" here. I don't think it's the tool's fault.
And that's my initiative
to request this RFC.
Why do we need this character anyway, why not just print the state in
hex and leave it at that? These single character state things are a
relic, please just let them die.
I believe hex is ok only after having the reported task state mapping
appear in the
uapi headers, otherwise it's still useless to userspace especially for
value like
TASK_REPORT_IDLE and TASK_REPORT_MAX, which need to dig into the
kernel to see what the hell is going on here.
Thoughts?
Regards,
Ze
Sorry that I don't get this one, did you mean kernel subsystems like
bpf or third party modules? Honestly I don't know how it works here
for userspace to consume the raw tracepoint without looking at
tracefs.
Regards,
Ze
On Tue, Aug 1, 2023 at 7:46 PM Peter Zijlstra [off-list ref] wrote:
On Tue, Aug 01, 2023 at 05:01:21PM +0800, Ze Gao wrote:
Oops, I thought sending this series for RFC is the "effort" you mean
to audit the users :/
Correct me if I'm making stupid moves here and enlighten me what
I should do furthermore to audit the users.
Thanks,
Ze
On Tue, Aug 1, 2023 at 7:47 PM Peter Zijlstra [off-list ref] wrote:
On Tue, Aug 01, 2023 at 05:01:22PM +0800, Ze Gao wrote:
quoted
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
I don't see a single line describing the effort you've done to audit
consumers of this tracepoint.
*IF* you're wanting to break this tracepoint ABI, because seriously
that's what it is, then you get to invest the time and effort to audit
the users.
From: Peter Zijlstra <peterz@infradead.org> Date: 2023-08-01 13:42:22
On Tue, Aug 01, 2023 at 09:03:51PM +0800, Ze Gao wrote:
It's just the design that exporting internal details is fundamentally wrong.
This is tracing... it wasn't supposed to be ABI (although it somehow
ended up being one). But even then, things like PF_foo get exposed in
procfs but even that we change.
The whole point of tracing is to see internals in order to figure out
wth is going wrong.
And even worse, I did not see any userspace tool is aware of masqueraded
states like TASK_REPORT_IDLE and TASK_REPORT_MAX and let alone
parse it correctly.
That's probably because I never use tools, I just look at the raw trace
output -- sometimes an impromptu awk script. I'm pretty sure I ran with
something like the below when I did the freezer rewrite -- or perhaps I
just stuck in trace_printk() and didn't even bother with the
tracepoints, I can't remember.
quoted
Why do we need this character anyway, why not just print the state in
hex and leave it at that? These single character state things are a
relic, please just let them die.
I believe hex is ok only after having the reported task state mapping
appear in the uapi headers, otherwise it's still useless to userspace
especially for value like TASK_REPORT_IDLE and TASK_REPORT_MAX, which
need to dig into the kernel to see what the hell is going on here.
Thoughts?
If you're tracing the kernel, you had better know what the kernel is
doing, otherwise you get to keep the pieces.
Anyway, if you're doing BPF then why do you care about the trace event
at all, just attach to the raw tracepoint and consume @preemt, @prev,
@next and @prev_state.
---
From: Steven Rostedt <rostedt@goodmis.org> Date: 2023-08-01 14:16:22
On Tue, 1 Aug 2023 17:01:22 +0800
Ze Gao [off-list ref] wrote:
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Ze Gao <redacted>
I'd swap this patch with patch 3. That is, make the field changes first.
I'd like this to get in regardless of if the state_char is accepted. We may
want to get this in first to see if there's any regressions before we add a
state_char.
-- Steve
From: Steven Rostedt <rostedt@goodmis.org> Date: 2023-08-01 14:19:18
On Tue, 1 Aug 2023 17:01:24 +0800
Ze Gao [off-list ref] wrote:
Since the sched_switch tracepoint introduces a new variable to
report sched-out task state in symbolic char, we prefer to use
it to spare from knowing internal implementations in kernel.
Also we keep the old parsing logic intact but sync the state char
array with the latest kernel.
This should be two patches. First sync the state char array and then add
your state_char change. The two changes are agnostic to each other, and
should be separate commits. Same goes for the perf changes.
-- Steve
@@ -99,7 +99,12 @@ static int sched_switch_handler(struct trace_seq *s,if(tep_get_field_val(s,event,"prev_prio",record,&val,1)==0)trace_seq_printf(s,"[%d] ",(int)val);-if(tep_get_field_val(s,event,"prev_state",record,&val,1)==0)+//find if has prev_state_char, otherwise fallback to prev_state+if(tep_find_field(event,"prev_state_char")){+if(tep_get_field_val(s,event,"prev_state_char",record,&val,1)==0)+trace_seq_putc(s,(char)val);+}+elseif(tep_get_field_val(s,event,"prev_state",record,&val,1)==0)write_state(s,val);trace_seq_puts(s," ==> ");
And this again will wreck everybody that consumes the raw tracepoint
without looking at tracefs.
Nobody does that anymore, as the events change constantly, and are
different on different kernels. Powertop (the tool that caused us pain
before by using raw values) had to break down and use libtraceevent,
because it would break if there was a 32 bit version running on a 64 bit
kernel.
I've changed the offsets of raw events a few times and nobody has
complained since.
-- Steve
From: Steven Rostedt <rostedt@goodmis.org> Date: 2023-08-01 14:33:51
On Tue, 1 Aug 2023 13:46:50 +0200
Peter Zijlstra [off-list ref] wrote:
On Tue, Aug 01, 2023 at 05:01:22PM +0800, Ze Gao wrote:
quoted
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
I don't see a single line describing the effort you've done to audit
consumers of this tracepoint.
*IF* you're wanting to break this tracepoint ABI, because seriously
that's what it is, then you get to invest the time and effort to audit
the users.
The known major users that I am aware of is raesdaemon,
powertop/latencytop, perf, trace-cmd and some bpf tools. The bpf tooling is
known to update per kernel. The others all use libtraceevent that can
handle this change.
What other tools are there? There's Perfetto, but it also looks at tracefs
to examine where the values are. There's LTTng, but I believe it uses the
raw tracepoint directly and doesn't look at the layout of the ftrace/perf
buffers.
All other tooling I am slightly aware of uses libtracefs and libtraceveent,
as I've been giving many talks on how to use those libraries.
-- Steve
On Tue, Aug 1, 2023 at 9:42 PM Peter Zijlstra [off-list ref] wrote:
On Tue, Aug 01, 2023 at 09:03:51PM +0800, Ze Gao wrote:
quoted
It's just the design that exporting internal details is fundamentally wrong.
This is tracing... it wasn't supposed to be ABI (although it somehow
Sorry, I'm confused. And it sounds contradicting here
because you said this change is abi break before.
ended up being one). But even then, things like PF_foo get exposed in
procfs but even that we change.
The whole point of tracing is to see internals in order to figure out
wth is going wrong.
Fair point, but I think tracepoints are somewhat different from
kprobes/raw_tracepoints due to their stable nature. And I think
at least more and more observability tools use them in this way.
With all due respect, If it was for kernel developers only, what's the
point of leaking this since now we have raw tracepoints support?
Does that mean all tracepoints are useless now? Apparently the
answer is no. So I'm not convinced by this "for internal inspecting"
defense to not ignore what the problem is.
Honestly, I would've never thought to change this if I I got the correct
meaning for values I captured like 0x80/0x100 when I tried to look it up
in include/linux/sched.h the first time. But it really annoyed me to
figure out what it is only after I dived into the kernel and collected all
the pieces. And you know what, when I turned to the famous in-tree perf
for possible help, only to find out it's horribly broken and still uses an
outdated state char array to interpret this weird raw value.
Anyway, we have to accept the fact that prev_state leaves a huge burden
on its users to make things right. And I'm open and glad to see any
solutions (possibly better than this one) or efforts or suggestions to improve
this.
Regards,
Ze
quoted hunk
quoted
And even worse, I did not see any userspace tool is aware of masqueraded
states like TASK_REPORT_IDLE and TASK_REPORT_MAX and let alone
parse it correctly.
That's probably because I never use tools, I just look at the raw trace
output -- sometimes an impromptu awk script. I'm pretty sure I ran with
something like the below when I did the freezer rewrite -- or perhaps I
just stuck in trace_printk() and didn't even bother with the
tracepoints, I can't remember.
quoted
quoted
Why do we need this character anyway, why not just print the state in
hex and leave it at that? These single character state things are a
relic, please just let them die.
I believe hex is ok only after having the reported task state mapping
appear in the uapi headers, otherwise it's still useless to userspace
especially for value like TASK_REPORT_IDLE and TASK_REPORT_MAX, which
need to dig into the kernel to see what the hell is going on here.
Thoughts?
If you're tracing the kernel, you had better know what the kernel is
doing, otherwise you get to keep the pieces.
Anyway, if you're doing BPF then why do you care about the trace event
at all, just attach to the raw tracepoint and consume @preemt, @prev,
@next and @prev_state.
---
Thanks for clarifying this ! Steven. This is really helpful.
Regards,
Ze
On Tue, Aug 1, 2023 at 10:33 PM Steven Rostedt [off-list ref] wrote:
On Tue, 1 Aug 2023 13:46:50 +0200
Peter Zijlstra [off-list ref] wrote:
quoted
On Tue, Aug 01, 2023 at 05:01:22PM +0800, Ze Gao wrote:
quoted
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
I don't see a single line describing the effort you've done to audit
consumers of this tracepoint.
*IF* you're wanting to break this tracepoint ABI, because seriously
that's what it is, then you get to invest the time and effort to audit
the users.
The known major users that I am aware of is raesdaemon,
powertop/latencytop, perf, trace-cmd and some bpf tools. The bpf tooling is
known to update per kernel. The others all use libtraceevent that can
handle this change.
What other tools are there? There's Perfetto, but it also looks at tracefs
to examine where the values are. There's LTTng, but I believe it uses the
raw tracepoint directly and doesn't look at the layout of the ftrace/perf
buffers.
All other tooling I am slightly aware of uses libtracefs and libtraceveent,
as I've been giving many talks on how to use those libraries.
-- Steve
Fair point, will do it in v4 as well.
Thanks,
Ze
On Tue, Aug 1, 2023 at 10:16 PM Steven Rostedt [off-list ref] wrote:
On Tue, 1 Aug 2023 17:01:22 +0800
Ze Gao [off-list ref] wrote:
quoted
Report priorities in 'short' and prev_state in 'int' to save
some buffer space. And also reorder the fields so that we take
struct alignment into consideration to make the record compact.
Suggested-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Ze Gao <redacted>
I'd swap this patch with patch 3. That is, make the field changes first.
I'd like this to get in regardless of if the state_char is accepted. We may
want to get this in first to see if there's any regressions before we add a
state_char.
-- Steve
Fair enough! Already did this in perf fixes. Will push a v4
to do this.
Thanks,
Ze
On Tue, Aug 1, 2023 at 10:19 PM Steven Rostedt [off-list ref] wrote:
On Tue, 1 Aug 2023 17:01:24 +0800
Ze Gao [off-list ref] wrote:
quoted
Since the sched_switch tracepoint introduces a new variable to
report sched-out task state in symbolic char, we prefer to use
it to spare from knowing internal implementations in kernel.
Also we keep the old parsing logic intact but sync the state char
array with the latest kernel.
This should be two patches. First sync the state char array and then add
your state_char change. The two changes are agnostic to each other, and
should be separate commits. Same goes for the perf changes.
-- Steve
@@ -99,7 +99,12 @@ static int sched_switch_handler(struct trace_seq *s,if(tep_get_field_val(s,event,"prev_prio",record,&val,1)==0)trace_seq_printf(s,"[%d] ",(int)val);-if(tep_get_field_val(s,event,"prev_state",record,&val,1)==0)+//find if has prev_state_char, otherwise fallback to prev_state+if(tep_find_field(event,"prev_state_char")){+if(tep_get_field_val(s,event,"prev_state_char",record,&val,1)==0)+trace_seq_putc(s,(char)val);+}+elseif(tep_get_field_val(s,event,"prev_state",record,&val,1)==0)write_state(s,val);trace_seq_puts(s," ==> ");