tools/runqslower.py: Option for --pid or --tid #2635

bodgergely · 2019-12-05T19:35:06Z

Closes: #2607

Option for --pid or --tid tracing.
Enable group leader level tracing.

--pid :
will trace every task that has as group leader.
Meaning it will trace the parent and its direct children.
--tid :
will trace the given thread

Update example file.

yonghong-song · 2019-12-06T06:23:29Z

[buildbot, test this please]

yonghong-song · 2019-12-06T06:23:40Z

[buildbot, ok to test]

yonghong-song

Thanks for the patch. Just added some comments. Overall the patch is in the right direction.

tools/runqslower.py

tools/runqslower_example.txt

tools/runqslower.py

bodgergely · 2019-12-06T12:37:02Z

For now I just made changes based on all your comments except the

could you change all tgid => pid and pid => tid in both python and C code? This will make code cleaner, less confusion and easy to follow.

yonghong-song

I do not mind you use tgid/pid in the bpf code to represent pid/tid in user space as long as the implementation is correct. Now the output we emit something like

TIME     COMM             PID           LAT(us)
10:29:01 kworker/11:2     177355            265
10:29:02 ksoftirqd/24     133              1068
10:29:02 dnsmond          561899            119

The PID here is actually tid. Do you mind to change to TID in python code and examples?

tools/runqslower.py

bodgergely · 2019-12-20T17:17:49Z

@yonghong-song Sorry for not resolving your comments yet... Will do so, today or over the weekend.

Closes: iovisor#2607 Option for --pid or --tid tracing. Enable group leader level tracing. * --pid <pid>: will trace every task that has <pid> as group leader. Meaning it will trace the parent and its direct children. * --tid <tid>: will trace the given thread * Reporting TID instead of PID in the report column. * Update the example file to reflect the TID/PID change

bodgergely · 2019-12-29T15:01:09Z

@yonghong-song Thanks for the valuable feedback! I made the changes. Can you take a look
and see if the changes look good?
Unfortunately I already squashed the commits so it will be a bit more difficult for you to see the updates
I did...sorry about that.

yonghong-song · 2020-01-02T07:38:54Z

[buildbot, test this please]

yonghong-song · 2020-01-02T07:39:12Z

@bodgergely no problem. will take a look soon.

tools/runqslower.py

yonghong-song · 2020-01-02T16:42:01Z

tools/runqslower.py

@@ -240,7 +260,7 @@ def print_event(cpu, data, size):
 b.attach_kprobe(event="finish_task_switch", fn_name="trace_run")

 print("Tracing run queue latency higher than %d us" % min_us)
-print("%-8s %-16s %-6s %14s" % ("TIME", "COMM", "PID", "LAT(us)"))
+print("%-8s %-16s %-6s %14s" % ("TIME", "COMM", "TID", "LAT(us)"))


This does not sound right. You cannot use "TID" all the time. If user specify '-p ', you want to show "PID".

We might get threads showing up in case of '--pid' flag too. So using PID is also not correct in that case either.
Anyways my 2 cents are on just to simply stay with showing PID as a column value even for threads. Htop, top does the same. Showing PID/TID is bringing quite a lot possible confusion here.
Or what do you vote for?
If you prefer I can make it show PID if '--pid' and TID if '--tid'.

Think about this again. For kernel run queue, actually TID indeed makes sense. So, yes, we can just do TID here. If user filtered through pid, showing TID should be okay too, redundant PID information may not be needed. If user did not do any filterring, TID is the right choice too.

In summary, we can just use "TID" as you changed the above. Please a comment though. Please address other comments and resubmit. Thanks!

Thanks, just to make it clear: :)

In case --pid cmd line arg -> show PID in the report column

In case --tid cmd line arg specified -> show TID in the report column

Is the above what you think would make the most sense?

tools/runqslower_example.txt

yonghong-song · 2020-01-02T16:43:33Z

tools/runqslower_example.txt

+Note: Showing TID (thread id) in the report column. The smallest
+execution unit becomes a TID when using the --pid flag as
+in that case the tool reports not only the parent pid but
+its children threads as well.


Please do make sure when "-p" is specified, the above contents are presented.

Do you mean to emit (the below caveat for example) when running the tool with --pid the text:

Note: when using the --pid flag the tool reports both the parent pid and any of its children threads as well.

bodgergely · 2020-01-03T00:54:21Z

Updating the man file too. If we agree on the reporting column (TID and PID) then I will make more updates to it.

yonghong-song

Overall, looks good. Showing TID even when the filtering based on pid is fine since fundamentally run queue is operating on threads. Thanks! Just one minor comment.

yonghong-song · 2020-01-03T01:28:55Z

tools/runqslower.py

@@ -207,6 +218,10 @@
 }
 """

+
+if args.pid and args.tid:
+ parser.error("specify only one of -p and -t")


With exclusive group in the above, the above checking should be redundant, python should have warned earlier.

I thought the same! I added this line because tools/offcputime.py had that too at line 102.
https://github.com/iovisor/bcc/blob/master/tools/offcputime.py#L102
Maybe that should be removed then too.
I will do so in mine.

It is already exclusive.

bodgergely · 2020-01-03T01:40:03Z

Let me know if good to go. Also probably I should squash the commits into a single unit before you merge.

yonghong-song · 2020-01-03T01:42:47Z

Also probably I should squash the commits into a single unit before you merge.
No worry. I can squash all commits into one before the merge.

bodgergely requested review from brendangregg and goldshtn as code owners December 5, 2019 19:35

yonghong-song reviewed Dec 6, 2019

View reviewed changes

tools/runqslower.py Outdated Show resolved Hide resolved

tools/runqslower.py Show resolved Hide resolved

tools/runqslower_example.txt Show resolved Hide resolved

tools/runqslower.py Outdated Show resolved Hide resolved

bodgergely force-pushed the bodgergely/runqslower-pid-tip-2607 branch from 3a98afb to 82c37af Compare December 6, 2019 12:35

yonghong-song reviewed Dec 6, 2019

View reviewed changes

tools/runqslower.py Outdated Show resolved Hide resolved

yonghong-song reviewed Dec 6, 2019

View reviewed changes

tools/runqslower.py Outdated Show resolved Hide resolved

bodgergely force-pushed the bodgergely/runqslower-pid-tip-2607 branch from 82c37af to dc60797 Compare December 29, 2019 14:57

bodgergely force-pushed the bodgergely/runqslower-pid-tip-2607 branch from dc60797 to 2a3818b Compare December 29, 2019 15:00

yonghong-song reviewed Jan 2, 2020

View reviewed changes

bodgergely added 3 commits January 2, 2020 21:41

Remove the 'and its children' comments.

1f865b7

tools/runqslower.py: Use add_mutually_exclusive_group()

09f4fd2

man runqslower: show TID option too

08d04cc

yonghong-song reviewed Jan 3, 2020

View reviewed changes

Remove redundant arg check.

43b1f5b

It is already exclusive.

yonghong-song merged commit db82644 into iovisor:master Jan 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools/runqslower.py: Option for --pid or --tid #2635

tools/runqslower.py: Option for --pid or --tid #2635

bodgergely commented Dec 5, 2019

yonghong-song commented Dec 6, 2019

yonghong-song commented Dec 6, 2019

yonghong-song left a comment

bodgergely commented Dec 6, 2019 •

edited

Loading

yonghong-song left a comment

bodgergely commented Dec 20, 2019 •

edited

Loading

bodgergely commented Dec 29, 2019 •

edited

Loading

yonghong-song commented Jan 2, 2020

yonghong-song commented Jan 2, 2020

yonghong-song Jan 2, 2020

bodgergely Jan 2, 2020

yonghong-song Jan 2, 2020

bodgergely Jan 3, 2020 •

edited

Loading

yonghong-song Jan 2, 2020

bodgergely Jan 3, 2020 •

edited

Loading

bodgergely commented Jan 3, 2020

yonghong-song left a comment

yonghong-song Jan 3, 2020

bodgergely Jan 3, 2020

bodgergely commented Jan 3, 2020

yonghong-song commented Jan 3, 2020

tools/runqslower.py: Option for --pid or --tid #2635

tools/runqslower.py: Option for --pid or --tid #2635

Conversation

bodgergely commented Dec 5, 2019

yonghong-song commented Dec 6, 2019

yonghong-song commented Dec 6, 2019

yonghong-song left a comment

Choose a reason for hiding this comment

bodgergely commented Dec 6, 2019 • edited Loading

yonghong-song left a comment

Choose a reason for hiding this comment

bodgergely commented Dec 20, 2019 • edited Loading

bodgergely commented Dec 29, 2019 • edited Loading

yonghong-song commented Jan 2, 2020

yonghong-song commented Jan 2, 2020

yonghong-song Jan 2, 2020

Choose a reason for hiding this comment

bodgergely Jan 2, 2020

Choose a reason for hiding this comment

yonghong-song Jan 2, 2020

Choose a reason for hiding this comment

bodgergely Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

yonghong-song Jan 2, 2020

Choose a reason for hiding this comment

bodgergely Jan 3, 2020 • edited Loading

Choose a reason for hiding this comment

bodgergely commented Jan 3, 2020

yonghong-song left a comment

Choose a reason for hiding this comment

yonghong-song Jan 3, 2020

Choose a reason for hiding this comment

bodgergely Jan 3, 2020

Choose a reason for hiding this comment

bodgergely commented Jan 3, 2020

yonghong-song commented Jan 3, 2020

bodgergely commented Dec 6, 2019 •

edited

Loading

bodgergely commented Dec 20, 2019 •

edited

Loading

bodgergely commented Dec 29, 2019 •

edited

Loading

bodgergely Jan 3, 2020 •

edited

Loading

bodgergely Jan 3, 2020 •

edited

Loading