add tcp congestion status duration statistic tool

[jackygam2001]

add tcp congestion contol status duration statistic tool, and it can be used to evalute the networking and congestion algorithm performace.

[iamkafai]

The use case makes sense to me. The implementation is not straightforward though because of the kernel missing a tracepoint in tcp_set_ca_state(), so it has to resort to kprobe and kretprobe on all functions calling tcp_set_ca_state().

".set_state" of "struct tcp_congestion_ops" is implemented for some popular cc like cubic, bbr, and dctcp. That will be a more stable kprobe points.

If you are interested, a tracepoint in tcp_set_ca_state() may be a good add to the kernel considering it is not the hot path. This future kernel work does not need to derail the current bcc pull request.

[jackygam2001]

The use case makes sense to me. The implementation is not straightforward though because of the kernel missing a tracepoint in tcp_set_ca_state(), so it has to resort to kprobe and kretprobe on all functions calling tcp_set_ca_state().

".set_state" of "struct tcp_congestion_ops" is implemented for some popular cc like cubic, bbr, and dctcp. That will be a more stable kprobe points.

If you are interested, a tracepoint in tcp_set_ca_state() may be a good add to the kernel considering it is not the hot path. This future kernel work does not need to derail the current bcc pull request.

yes, currently the upstream kernel only has inline function tcp_set_ca_state(), which is optimized by compiler when building and can not be probed. So in my implementation i have to kprobe and kretprobe on all functions calling it.

[brendangregg]

Thanks; Several comments:

• Name is too long; should be tcpcong or something.

• It has 20 k{ret}probes plus a lot of struct digging, which would make it by far the most brittle tool in all of bcc. Most kprobe tools use between 1 and 6 kprobes. The prior biggest is zfsdist/zfsslower, which has 16, but only attaches to 6 at a time depending on the software version, and it also does no struct digging. (While BTF can help solve struct members moving, it can't solve logic changes.) Also, looking at these tcp functions:

grep attach_k.*probe tcpcongestdura.py 
b.attach_kprobe(event="tcp_try_keep_open",
b.attach_kretprobe(event="tcp_try_keep_open",
b.attach_kprobe(event="tcp_enter_cwr", fn_name="trace_entry_tcp_enter_cwr")
b.attach_kretprobe(event="tcp_enter_cwr",
b.attach_kprobe(event="tcp_process_tlp_ack",
b.attach_kretprobe(event="tcp_process_tlp_ack",
b.attach_kprobe(event="tcp_enter_recovery",
b.attach_kretprobe(event="tcp_enter_recovery",
b.attach_kprobe(event="tcp_enter_loss", fn_name="trace_entry_tcp_enter_loss")
b.attach_kretprobe(event="tcp_enter_loss",
b.attach_kprobe(event="tcp_simple_retransmit",
b.attach_kretprobe(event="tcp_simple_retransmit",
b.attach_kprobe(event="tcp_try_undo_recovery",
b.attach_kretprobe(event="tcp_try_undo_recovery",
b.attach_kprobe(event="tcp_try_undo_loss",
b.attach_kretprobe(event="tcp_try_undo_loss",
b.attach_kprobe(event="tcp_fastretrans_alert",
b.attach_kretprobe(event="tcp_fastretrans_alert",
b.attach_kprobe(event="tcp_disconnect", fn_name="trace_entry_tcp_enter_open")
b.attach_kretprobe(event="tcp_disconnect",

I guess this is only really tracing 10 functions as it's doing both kprobe and kretprobe.

Things like tcp_disconnect() I'm not too worried about as it's in tcp_prot, and isn't likely going to change, and tcp_enter_loss() is in tcp.h. But others like tcp_try_undo_loss() feel like gritty internals. I'm ok with one or two such functions in a tool, but if it ends up being several it's a pain to maintain.

So my two questions are: How many of these are unstable internal functions, and does it need to trace them all or can you pop up a stack frame and trace some more-stable parent function to do the same job? I'm not saying this is a blocker, I just want to be thoughtful about the future maintenance.

• How does it compare to tcp_probe and other TCP congestion analysis tools? (Other people will no doubt ask.)

• Output columns look misaligned by one.

• Default output is > 80 chars, but fortunately only by 5. This does make it hard (and sometimes impossible) to include the output in things like published books and printed articles, but I don't see an easy fix.

• Columns are in "us", but the numbers are so big it's confusing. Would it make sense to make them "ms" by default? Have a switch to output "us".

• I think the opening example in the examples file is missing a sentence or more to explain why the output matters. Which of these columns is evidence of a problem, such that clients are blocked waiting? Help a newcomer interpret what they are seeing.

• These numbers are great, but how would I double check that they are right? Have you already got some fields in nstat/netstat or something to compare them to, at least as a sanity check? (They won't have latency totals, but they may have counts.) Just looking for a way to sanity check the output.

[jackygam2001]

@brendangregg ,
thanks for your comments!
For comment 2, you are right. I have checked kernel code, most of cc status change functions are called by 5 functions(tcp_fastretrans_alert, tcp_enter_cwr, tcp_enter_loss, tcp_enter_recovery, tcp_process_tlp_ack), so i will update the code in next pull request. I think @iamkafai 's advice is very good, i will try to add tracepoint for tcp_set_ca_state to upstream kernel.
For comment 3, i think the tcp_probe is for trace the connection's congestion window update, but this tool is for tracing the congestion control status change and duration. you know the cwnd's update may not cause status's change, so they are two different tools.
For comment 8, i don't use the nstat/netstat to validate the tool, as those tools has only summary for all tcp sockets. Actually i use packetdrill script which was provided by google on github to check the output of my tool.
For other comments, i will update the code in next pull request. thanks very much!

[yonghong-song]

A lot of inlining COULD happen for the probe functions used in this tool. So I strongly suggest you to implement the tracepoint as Martin suggested. The tool can still proceed. But with tracepoint in the kernel, we can have an alternative better implementation for future kernels.

[jackygam2001]

@yonghong-song ,
thanks for your comments, yes, i will prepare the tracepoint for tcp_set_ca_state patch to upstream kernel. And will update my code for other comments in next commit. thanks very much!

[yonghong-song]

Most of okay. A few minor comments.