-
Notifications
You must be signed in to change notification settings - Fork 798
providers: add support for USDT tracing using libbpf/usdt #1663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| #define rdma_tracepoint(arg...) | ||
| #include <util/usdt.h> | ||
|
|
||
| #define rdma_tracepoint(arg...) USDT(arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also be enabled by a cmake flag and not as fallback to LTTng.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mrgolin thanks for reviewing! What would be the rationale though? It would be most excellent to have some form of tracing always available, and USDT inserts only a noop instruction at the instrumentation points (so no overhead when not in use).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- AFAIK there are still instructions being added in addition to the mentioned NOP
- Notice that in some cases there are additional function calls inside
rdma_tracepointmacro use that are not meant to be called when building without tracing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For point 1 - in the non-semaphore case (which is used here), it really is just the single NOP instruction that gets executed. We can verify this via the __usdt_probe() assembly code in the usdt.h header. There is a small amount of on-disk space added in a 'note' section that describes the parameters, but that's not used when a probe is not enabled.
And for point 2 - I had a look at those earlier and they are simple string formatters (so things like a switch statement and some buffer setup for string reporting) - its negligible AFAICS. I don't know the history but it's more likely they are conditionally compiled previously because they were unused without LTTng than due to any measurable overhead. In some ways it simplifies the code (removes the need to have ifdefs sprinked around) by making them always available.
Having said that, if its genuinely concerning people I can certainly put together some cmake additions to this PR that make it conditionally compiled. I'd prefer to have a good reason to do so though, I'm not really convinced its needed so far.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding "history", we are very sensitive in any additions in datapath. This is why everything is compiled-out by default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rleon makes sense. Are there any trace points on the datapath?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an example:
2278 int efa_post_send(struct ibv_qp *ibvqp, struct ibv_send_wr *wr,
2279 struct ibv_send_wr **bad)
2280 {
...
2342 }
2343 rdma_tracepoint(rdma_core_efa, post_send, qp->dev->name, wr->wr_id,
2344 EFA_IO_SEND, ibvqp->qp_num, meta_desc->dest_qp_num,
2345 ah->efa_ah, efa_get_wqe_length(&tx_wqe));
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, thanks @rleon - yeah that's definitely a little more than just the no-op esp. with the efa_get_wqe_length call there. I'll post a revised patch.
Any thoughts on the checkpatch usdt.h issues from CI? I'll report that to the libbpf/usdt folk, but it may take a while - can we filter that header from checkpatch somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, the libbpf/usdt folk think the checkpatch warnings here are "bogus": libbpf/usdt#14 (comment)
bb8f04b to
7790db4
Compare
| # Enable LTTng tracing. | ||
| # -DENABLE_USDT (default, no USDT tracing support) | ||
| # Enable USDT tracing. | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- You can ignore checkpatch warning.
- Can both LTTNG and USDT be enabled together?
If not, probably you should change -DENABLE_LTTNG to be:
-DTRACING=LTTNG, -DTRACING=USDT and -DTRACING=None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rleon Having a general cmake flag does look better in my opinion (that's also what we've suggested in the original PR) but changing it now might break existing users. I think we can compromise having a separate flag for each tracing tool but explicitly verifying that they are not enabled at same time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rleon Having a general cmake flag does look better in my opinion (that's also what we've suggested in the original PR) but changing it now might break existing users. I think we can compromise having a separate flag for each tracing tool but explicitly verifying that they are not enabled at same time.
I don't see compilation flags as ABI contract. We can change them whatever we want. Everyone who is compiling this library by himself is capable enough to update cmake flag too. Others are getting this library already precompiled from their disto, which won't be affected by this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can both LTTNG and USDT be enabled together?
Its theoretically possible. The current PR doesn't implement this though.
|
@mrgolin are you ok with this PR? Thanks |
| #ifndef __EFA_TRACE_H__ | ||
| #define __EFA_TRACE_H__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep this in the #else case too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
| #endif /* __EFA_TRACE_H__*/ | ||
|
|
||
| #endif /* defined(LTTNG_ENABLED) */ | ||
| #endif /* defined(LTTNG_ENABLED) || defined(USDT_ENABLED) */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@natoscott Maybe we can have an additional TRACING_ENABLED define to avoid repeating this?
|
|
||
| #include <util/usdt.h> | ||
|
|
||
| #define rdma_tracepoint(arg...) USDT(arg) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Could be nice if this is defined only once.
Left few more comments, thanks. |
Provide lightweight USDT tracing as an alternative to LTTng. This piggybacks on the existing tracing code added for LTTng for a minimal set of changes. > $ sudo bpftrace -l usdt:build/lib/lib*.so:* > usdt:build/lib/libefa-rdmav59.so:rdma_core_efa:post_recv > usdt:build/lib/libefa-rdmav59.so:rdma_core_efa:post_send > usdt:build/lib/libefa-rdmav59.so:rdma_core_efa:process_completion > usdt:build/lib/libefa.so:rdma_core_efa:post_recv > usdt:build/lib/libefa.so:rdma_core_efa:post_send > usdt:build/lib/libefa.so:rdma_core_efa:process_completion > usdt:build/lib/libhns-rdmav59.so:rdma_core_hns:poll_cq > usdt:build/lib/libhns-rdmav59.so:rdma_core_hns:post_recv > usdt:build/lib/libhns-rdmav59.so:rdma_core_hns:post_send > usdt:build/lib/libhns.so:rdma_core_hns:poll_cq > usdt:build/lib/libhns.so:rdma_core_hns:post_recv > usdt:build/lib/libhns.so:rdma_core_hns:post_send > usdt:build/lib/libmlx5-rdmav59.so:rdma_core_mlx5:post_send > usdt:build/lib/libmlx5.so:rdma_core_mlx5:post_send > usdt:build/lib/librxe-rdmav59.so:rdma_core_rxe:post_send The USDT header used here is from the libbpf/usdt project at https://github.com/libbpf/usdt.git Further background discussion for this commit is included in linux-rdma#1621 Signed-off-by: Nathan Scott <[email protected]>
7790db4 to
b393389
Compare
Default to providing lightweight USDT trace points when LTTng is unavailable. This piggybacks on the existing tracing code added for LTTng for a minimal set of changes.
The USDT header used here is from the libbpf/usdt project at https://github.com/libbpf/usdt.git
Further background discussion for this commit is included in #1621