softirqs: focus CPU as disector #1130

cherusk · 2017-04-23T12:57:34Z

With having in mind that softirq processing is happeing in
ksoftirqd/ context, which is associated with a specific cpu over
the whole dynamic life time of a system, focussing on CPus as the
disector appears more sensical.

Quite helpful is this alternative angle of view on the softirqs
processing especially for surveilling the effectiveness of net stack
tunings, as this is highly dynamic depending on the actual net stack
configuration, e.g. scaling or steering.

Signed-off-by: Matthias Tafelmeier matthias.tafelmeier@gmx.net

brendangregg · 2017-04-25T16:55:05Z

Do you think it makes sense to update tools/softirq.py to key on the CPU id as well?

Note for others: to test tools/old/softirq.py on newer kernels, I had to delete the blk_iopoll_softirq kprobe.

So the output is now:

# ./softirqs.py.1
warning: unknown warning option '-Wno-pragma-once-outside-header'; did you mean '-Wno-private-header'? [-Wunknown-warning-option]
1 warning generated.
Tracing soft irq event time... Hit Ctrl-C to end.
^C
SOFTIRQ                            CPU TOTAL_usecs
net_tx_action                        7          69
run_rebalance_domains                3         235
run_rebalance_domains                2         248
run_rebalance_domains                4         257
run_rebalance_domains                0         271
run_rebalance_domains                6         274
run_rebalance_domains                1         342
run_rebalance_domains                7         355
run_rebalance_domains                5         366
rcu_process_callbacks                5         804
rcu_process_callbacks                3         820
rcu_process_callbacks                2         822
rcu_process_callbacks                0         828
rcu_process_callbacks                1         912
rcu_process_callbacks                6         930
rcu_process_callbacks                4        1319
rcu_process_callbacks                7        1404
net_rx_action                        7        1451
run_timer_softirq                    6        2536
run_timer_softirq                    2        2554
run_timer_softirq                    3        2599
run_timer_softirq                    0        2642
run_timer_softirq                    1        2893
run_timer_softirq                    4        2901
run_timer_softirq                    7        2951
run_timer_softirq                    5        3269

I'd make this per-CPU breakdown an option, like -C. I do want the per-softirq summaries by default, especially as my larger systems have 64 CPUs and the output is going to get long.

An example of an optional breakdown is biolatency's -D.

I haven't encountered the word "dissector" for this feature, we usually use "breakdown", but I can see that dissector is more specific, so I think I like it. :)

cherusk · 2017-04-25T19:53:40Z

Do you think it makes sense to update tools/softirq.py to key on the CPU id as well?

Theopractically, it'd. Was a little selfish, since I was running on a kernel < 4.7. Wanted to sound opinions first.

|| I'd make this per-CPU breakdown an option, like -C.

Right, no objections.

I do want the per-softirq summaries by default, especially as my larger systems have 64 CPUs and the output is going to get long.

Am handling equally equipped systems. On top, well, it's rather meant as a "grepable" backend mechanism. Thought of NUMA aware agglomeration for manual wielding. However, we can leave the status quo in place.

[...] "breakdown", but I can see that dissector is more specific, so I think I like it. :)

No preferences, merely was the first word appearing to when commencing on it.

…

-- Besten Gruß Matthias Tafelmeier

cherusk · 2017-04-26T19:30:59Z

Right, seems legit now ...

goldshtn

I have a couple of comments below, and also a bigger question: why does this PR only update tools/old/softirqs.py, and not the new tool that uses tracepoints? That's the one most people will be using by default (on 4.7+ anyway).

goldshtn · 2017-04-27T04:49:21Z

tools/old/softirqs.py

@@ -33,6 +33,8 @@
    help="output in nanoseconds")
 parser.add_argument("-d", "--dist", action="store_true",
    help="show distributions as histograms")
+parser.add_argument("-C", "--CPUidx", action="store_true",


I suggest a somewhat clearer long option name, perhaps --by-cpu?

No preferences, can do so ...

goldshtn · 2017-04-27T04:51:14Z

tools/old/softirqs.py

-    if (tsp == 0 || ipp == 0) {
-        return 0;   // missed start
+bpf_text = ""
+if args.CPUidx:


There is a lot of shared code across the two alternatives, which makes me worried about duplication and potential maintenance problems if we add more "dissectors". What we typically do in other tools is one of two options:

Embed string markers (like REPLACEME) in the C code string and then conditionally replace them with the appropriate code, kind of like you did with COMMON here but in the other direction

Use conditional compilation (#ifdef ...) in the C code and conditionally prepend the relevant macro definition

Well, technically, I agree, though, I was refraining from making the replacement section a mess. So I had a tradeoff between readability and code duplication in mind.

goldshtn · 2017-04-27T04:53:10Z

tools/old/softirqs.py

@@ -115,11 +166,18 @@
        "rcu_process_callbacks", "run_rebalance_domains", "tasklet_action",
        "tasklet_hi_action", "run_timer_softirq", "net_tx_action",
        "net_rx_action"):
-    b.attach_kprobe(event=softirqfunc, fn_name="trace_start")
-    b.attach_kretprobe(event=softirqfunc, fn_name="trace_completion")
+    if args.CPUidx:


Again, there's no need for this duplication if we give the C functions the same names in both cases. I don't know why not.

goldshtn · 2017-04-27T04:54:55Z

[buildbot, add to whitelist]

cherusk · 2017-04-27T18:15:46Z

I have a couple of comments below, and also a bigger question: why does this PR only update tools/old/softirqs.py, and not the new tool that uses tracepoints? That's the one most people will be using by default (on 4.7+ anyway).

Answered that further up.

…

-- Besten Gruß Matthias Tafelmeier

brendangregg · 2017-05-01T17:30:47Z

# ./softirqs.py 
Traceback (most recent call last):
  File "./softirqs.py", line 54, in <module>
    if args.by-cpu:
AttributeError: 'Namespace' object has no attribute 'by'

Seems that my versions of argparse doesn't like that. I found s/by-cpu/bycpu/ fixed it. (How come our test system didn't pick it up?).

-C also doesn't work with -d:

# softirqs.py -C -d
warning: unknown warning option '-Wno-pragma-once-outside-header'; did you mean '-Wno-private-header'? [-Wunknown-warning-option]
1 warning generated.
Tracing soft irq event time... Hit Ctrl-C to end.
^C
Traceback (most recent call last):
  File "./softirqs.py", line 197, in <module>
    dist.print_log2_hist(label, "softirq", section_print_fn=distr_sec_fn)
  File "/usr/lib/python2.7/dist-packages/bcc/table.py", line 311, in print_log2_hist
    vals[slot] = v.value
IndexError: cannot fit 'long' into an index-sized integer

I filed #1146 for updating tools/softirqs.

goldshtn · 2017-05-01T17:47:37Z

@brendangregg Our smoke tests only run the tools in tools/, not in tools/old/.

I generally think of the tools in tools/old/ as unmaintained.

With having in mind that softirq processing is happeing in ksoftirqd/<cpu> context, which is associated with a specific cpu over the whole dynamic life time of a system, focussing on CPus as the dissector appears more sensical. Quite helpful is this alternative angle of view on the softirqs processing especially for surveilling the effectiveness of net stack tunings, as this is highly dynamic depending on the actual net stack configuration, e.g. scaling or steering. Signed-off-by: Matthias Tafelmeier <matthias.tafelmeier@gmx.net>

cherusk · 2017-07-05T18:07:54Z

Amended things, also the general Softirq per CPU breakdown is working now, though, I don't deem this as very helpful. It might need a nested breakdown per CPU-softirq on top - feel free to add that later. Cheers!

4ast · 2017-10-26T03:35:11Z

have been pending for too long. Please resubmit if it's still relevant.

cherusk · 2017-10-31T18:25:57Z

It is ... pls, reopen in order not to splinter the context. I've not procrastinated it.

4ast

rebase needed?

cherusk · 2017-11-01T07:40:44Z

I don't think so., seemingly, it hasn't been touched on master ever since.

yonghong-song · 2017-11-01T16:27:18Z

@cherusk This still only changes tools/old/softirqs.py. Did any situation change in your side so you could make a change in tools/softirqs.py instead? As mentioned in the above, tools/old/... are considered unmaintained and hence will not be tested in bcc testing framework and will not be used by most people.

cherusk · 2017-11-01T17:07:14Z

@yonghong-song I am prepared to align the tools/softirqs.py to the 'old' symmetrically. Though, landscapes on older kernels (enough out there for stability) will use this one, so please merge it in.

yonghong-song · 2017-11-01T18:18:06Z

@4ast could you merge since you requested the change? Note that the code is using kprobe and it will not work on 4.14 kernel as some functions are already gone.

cherusk force-pushed the master branch from d7331fd to 70f7372 Compare April 23, 2017 18:39

cherusk force-pushed the master branch from 70f7372 to 5008eef Compare April 26, 2017 19:30

goldshtn reviewed Apr 27, 2017

View reviewed changes

cherusk force-pushed the master branch from 5008eef to 744e1ae Compare April 27, 2017 18:29

brendangregg mentioned this pull request May 1, 2017

softirqs to use CPU #1146

Open

cherusk force-pushed the master branch from 744e1ae to 4618690 Compare July 5, 2017 18:05

4ast closed this Oct 26, 2017

4ast reopened this Oct 31, 2017

4ast requested changes Oct 31, 2017

View reviewed changes

4ast merged commit 8c0e4b9 into iovisor:master Nov 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

softirqs: focus CPU as disector #1130

softirqs: focus CPU as disector #1130

cherusk commented Apr 23, 2017

brendangregg commented Apr 25, 2017

cherusk commented Apr 25, 2017 via email

cherusk commented Apr 26, 2017

goldshtn left a comment

goldshtn Apr 27, 2017

cherusk Apr 27, 2017

goldshtn Apr 27, 2017

cherusk Apr 27, 2017

goldshtn Apr 27, 2017

goldshtn commented Apr 27, 2017

cherusk commented Apr 27, 2017 via email

brendangregg commented May 1, 2017

goldshtn commented May 1, 2017

cherusk commented Jul 5, 2017

4ast commented Oct 26, 2017

cherusk commented Oct 31, 2017

4ast left a comment

cherusk commented Nov 1, 2017

yonghong-song commented Nov 1, 2017

cherusk commented Nov 1, 2017

yonghong-song commented Nov 1, 2017

softirqs: focus CPU as disector #1130

softirqs: focus CPU as disector #1130

Conversation

cherusk commented Apr 23, 2017

brendangregg commented Apr 25, 2017

cherusk commented Apr 25, 2017 via email

cherusk commented Apr 26, 2017

goldshtn left a comment

Choose a reason for hiding this comment

goldshtn Apr 27, 2017

Choose a reason for hiding this comment

cherusk Apr 27, 2017

Choose a reason for hiding this comment

goldshtn Apr 27, 2017

Choose a reason for hiding this comment

cherusk Apr 27, 2017

Choose a reason for hiding this comment

goldshtn Apr 27, 2017

Choose a reason for hiding this comment

goldshtn commented Apr 27, 2017

cherusk commented Apr 27, 2017 via email

brendangregg commented May 1, 2017

goldshtn commented May 1, 2017

cherusk commented Jul 5, 2017

4ast commented Oct 26, 2017

cherusk commented Oct 31, 2017

4ast left a comment

Choose a reason for hiding this comment

cherusk commented Nov 1, 2017

yonghong-song commented Nov 1, 2017

cherusk commented Nov 1, 2017

yonghong-song commented Nov 1, 2017