Skip to content

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+634 −18
Open

providers: add support for USDT tracing using libbpf/usdt #1663

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
# Use the historical search path for providers, in the standard system library.
# -DNO_COMPAT_SYMS=1 (default disabled)
# Do not generate backwards compatibility symbols in the shared
# libraries. This may is necessary if using a dynmic linker that does
# libraries. This may be necessary if using a dynamic linker that does
# not support symbol versions, such as uclibc.
# -DIOCTL_MODE=write (default both)
# Disable new kABI ioctl() support and support only the legacy write
Expand Down Expand Up @@ -52,8 +52,10 @@
# Disable man pages. Allows rdma-core to be built and installed
# (without man pages) when neither pandoc/rst2man nor the pandoc-prebuilt
# directory are available.
# -DENABLE_LTTNG (default, no tracing support)
# -DENABLE_LTTNG (default, no LTTng tracing support)
# Enable LTTng tracing.
# -DENABLE_USDT (default, no USDT tracing support)
# Enable USDT tracing.

Copy link
Member

@rleon rleon Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. You can ignore checkpatch warning.
  2. 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

Copy link
Contributor

@mrgolin mrgolin Nov 23, 2025

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.

Copy link
Member

@rleon rleon Nov 23, 2025

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.

Copy link
Author

@natoscott natoscott Nov 24, 2025

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.

Copy link
Member

@rleon rleon Nov 24, 2025

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.

You need to throw an error in such case and refuse to compile.
if ( enable_lttng AND enable_usdt)
panic("not implemented yet ....")

Copy link
Author

@natoscott natoscott Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - I've added header checks for this condition now.

if (${CMAKE_VERSION} VERSION_LESS "3.18.1")
# Centos 7 support
Expand Down Expand Up @@ -573,6 +575,14 @@ if (DEFINED ENABLE_LTTNG)
add_definitions(-DLTTNG_ENABLED)
endif()

# User Static-Defined Tracing (USDT) support
if (NOT DEFINED ENABLE_USDT)
set(ENABLE_USDT "OFF" CACHE BOOL "Enable USDT tracing support")
endif()
if (ENABLE_USDT)
add_definitions(-DUSDT_ENABLED)
endif()

#-------------------------
# Apply fixups

Expand Down
2 changes: 1 addition & 1 deletion Documentation/contributing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ that are designed to weed out bugs.
Serious errors will result in a red X in the PR and will need to be corrected.
Less serious errors, including checkpatch related, will show up with a green
check but it is necessary to check the details to see that everything is
appropriate. checkpatch is an informative too, not all of its feedback is
appropriate. checkpatch is an informative tool, not all of its feedback is
appropriate to fix.

A build similar to AZP can be run locally using docker and the
Expand Down
21 changes: 18 additions & 3 deletions providers/efa/efa_trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* Copyright 2023-2025 Amazon.com, Inc. or its affiliates. All rights reserved.
*/

#if defined(LTTNG_ENABLED)
#if defined(LTTNG_ENABLED) && defined(USDT_ENABLED)

#error "Only one of LTTNG_ENABLED and USDT_ENABLED allowed"
Copy link
Member

@rleon rleon Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check needs to be in global CmakeList.txt and not in private header.

Thanks

Copy link
Author

@natoscott natoscott Nov 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe both? I'm out of time to work on this further currently as I'll be traveling over the next 6 weeks - if someone else could progress it in the meantime, that'd be awesome. Otherwise I'll take a look again in the new year.

Copy link
Member

@rleon rleon Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe both? I'm out of time to work on this further currently as I'll be traveling over the next 6 weeks - if someone else could progress it in the meantime, that'd be awesome. Otherwise I'll take a look again in the new year.

Let's see how it evolves. Have a nice trip.


#elif defined(LTTNG_ENABLED)

#undef LTTNG_UST_TRACEPOINT_PROVIDER
#define LTTNG_UST_TRACEPOINT_PROVIDER rdma_core_efa
Expand Down Expand Up @@ -109,7 +113,18 @@ LTTNG_UST_TRACEPOINT_EVENT(

#include <lttng/tracepoint-event.h>

#else
#elif defined (USDT_ENABLED)

#ifndef __EFA_TRACE_H__
#define __EFA_TRACE_H__

#include <util/usdt.h>

#define rdma_tracepoint(arg...) USDT(arg)
Copy link
Contributor

@mrgolin mrgolin Nov 13, 2025

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.

Copy link
Author

@natoscott natoscott Nov 13, 2025

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).

Copy link
Contributor

@mrgolin mrgolin Nov 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. AFAIK there are still instructions being added in addition to the mentioned NOP
  2. Notice that in some cases there are additional function calls inside rdma_tracepoint macro use that are not meant to be called when building without tracing

Copy link
Author

@natoscott natoscott Nov 17, 2025 •
edited
Loading

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.

Copy link
Member

@rleon rleon Nov 17, 2025

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.

Copy link
Author

@natoscott natoscott Nov 17, 2025

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?

Copy link
Member

@rleon rleon Nov 17, 2025

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));
...

Copy link
Author

@natoscott natoscott Nov 17, 2025

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?

Copy link
Author

@natoscott natoscott Nov 18, 2025

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)


#endif /* __EFA_TRACE_H__*/

#else /* !defined(LTTNG_ENABLED) && !defined(USDT_ENABLED) */

#ifndef __EFA_TRACE_H__
#define __EFA_TRACE_H__
Expand All @@ -118,4 +133,4 @@ LTTNG_UST_TRACEPOINT_EVENT(

#endif /* __EFA_TRACE_H__*/

#endif /* defined(LTTNG_ENABLED) */
#endif /* defined(LTTNG_ENABLED) || defined(USDT_ENABLED) */
Copy link
Contributor

@mrgolin mrgolin Nov 23, 2025

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?

2 changes: 1 addition & 1 deletion providers/efa/verbs.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2170,7 +2170,7 @@ static void efa_set_common_ctrl_flags(struct efa_io_tx_meta_desc *desc,
EFA_SET(&desc->ctrl2, EFA_IO_TX_META_DESC_COMP_REQ, 1);
}

#ifdef LTTNG_ENABLED
#if defined(LTTNG_ENABLED) || defined(USDT_ENABLED)
static uint32_t efa_get_wqe_length(struct efa_io_tx_wqe *tx_wqe)
{
enum efa_io_send_op_type op_type;
Expand Down
4 changes: 2 additions & 2 deletions providers/hns/hns_roce_u_hw_v2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -678,7 +678,7 @@ static int parse_cqe_for_cq(struct hns_roce_context *ctx, struct hns_roce_cq *cq
return 0;
}

#ifdef LTTNG_ENABLED
#if defined(LTTNG_ENABLED) || defined(USDT_ENABLED)
static uint8_t read_wc_sl(struct hns_roce_qp *hr_qp,
struct hns_roce_v2_cqe *cqe,
struct ibv_wc *wc)
Expand All @@ -687,6 +687,7 @@ static uint8_t read_wc_sl(struct hns_roce_qp *hr_qp,
hr_reg_read(cqe, CQE_S_R) == CQE_FOR_RQ ?
wc->sl : UINT8_MAX;
}
#endif

static uint32_t read_wc_rqpn(struct hns_roce_qp *hr_qp,
struct hns_roce_v2_cqe *cqe,
Expand Down Expand Up @@ -750,7 +751,6 @@ static uint8_t get_send_wr_tclass(struct ibv_send_wr *wr,
return qp_type == IBV_QPT_UD ?
to_hr_ah(wr->wr.ud.ah)->av.tclass : UINT8_MAX;
}
#endif

static int hns_roce_poll_one(struct hns_roce_context *ctx,
struct hns_roce_qp **cur_qp, struct hns_roce_cq *cq,
Expand Down
21 changes: 18 additions & 3 deletions providers/hns/hns_roce_u_trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* Copyright (c) 2025 Hisilicon Limited.
*/

#if defined(LTTNG_ENABLED)
#if defined(LTTNG_ENABLED) && defined(USDT_ENABLED)

#error "Only one of LTTNG_ENABLED and USDT_ENABLED allowed"

#elif defined(LTTNG_ENABLED)

#undef LTTNG_UST_TRACEPOINT_PROVIDER
#define LTTNG_UST_TRACEPOINT_PROVIDER rdma_core_hns
Expand Down Expand Up @@ -121,7 +125,18 @@ LTTNG_UST_TRACEPOINT_EVENT(

#include <lttng/tracepoint-event.h>

#else
#elif defined(USDT_ENABLED)

#ifndef __HNS_TRACE_H__
#define __HNS_TRACE_H__

#include <util/usdt.h>

#define rdma_tracepoint(arg...) USDT(arg)
Copy link
Contributor

@mrgolin mrgolin Nov 23, 2025

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.

Copy link
Author

@natoscott natoscott Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I followed the existing pattern from LTTNG tracing, where each provider provides self-contained definitions.


#endif /* __HNS_TRACE_H__*/

#else /* !defined(LTTNG_ENABLED) && !defined(USDT_ENABLED) */

#ifndef __HNS_TRACE_H__
#define __HNS_TRACE_H__
Expand All @@ -130,4 +145,4 @@ LTTNG_UST_TRACEPOINT_EVENT(

#endif /* __HNS_TRACE_H__*/

#endif /* defined(LTTNG_ENABLED) */
#endif /* defined(LTTNG_ENABLED) || defined(USDT_ENABLED) */
21 changes: 18 additions & 3 deletions providers/mlx5/mlx5_trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* Copyright 2023 Bytedance.com, Inc. or its affiliates. All rights reserved.
*/

#if defined(LTTNG_ENABLED)
#if defined(LTTNG_ENABLED) && defined(USDT_ENABLED)

#error "Only one of LTTNG_ENABLED and USDT_ENABLED allowed"

#elif defined(LTTNG_ENABLED)

#undef LTTNG_UST_TRACEPOINT_PROVIDER
#define LTTNG_UST_TRACEPOINT_PROVIDER rdma_core_mlx5
Expand Down Expand Up @@ -47,7 +51,18 @@ LTTNG_UST_TRACEPOINT_EVENT(

#include <lttng/tracepoint-event.h>

#else
#elif defined(USDT_ENABLED)

#ifndef __MLX5_TRACE_H__
#define __MLX5_TRACE_H__

#include <util/usdt.h>

#define rdma_tracepoint(arg...) USDT(arg)

#endif /* __MLX5_TRACE_H__*/

#else /* !defined(LTTNG_ENABLED) && !defined(USDT_ENABLED) */

#ifndef __MLX5_TRACE_H__
#define __MLX5_TRACE_H__
Expand All @@ -56,4 +71,4 @@ LTTNG_UST_TRACEPOINT_EVENT(

#endif /* __MLX5_TRACE_H__*/

#endif /* defined(LTTNG_ENABLED) */
#endif /* defined(LTTNG_ENABLED) || defined(USDT_ENABLED) */
21 changes: 18 additions & 3 deletions providers/rxe/rxe_trace.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
* Copyright 2023 Bytedance.com, Inc. or its affiliates. All rights reserved.
*/

#if defined(LTTNG_ENABLED)
#if defined(LTTNG_ENABLED) && defined(USDT_ENABLED)

#error "Only one of LTTNG_ENABLED and USDT_ENABLED allowed"

#elif defined(LTTNG_ENABLED)

#undef LTTNG_UST_TRACEPOINT_PROVIDER
#define LTTNG_UST_TRACEPOINT_PROVIDER rdma_core_rxe
Expand Down Expand Up @@ -47,7 +51,18 @@ LTTNG_UST_TRACEPOINT_EVENT(

#include <lttng/tracepoint-event.h>

#else
#elif defined(USDT_ENABLED)

#ifndef __RXE_TRACE_H__
#define __RXE_TRACE_H__

#include <util/usdt.h>

#define rdma_tracepoint(arg...) USDT(arg)

#endif /* __RXE_TRACE_H__*/

#else /* !defined(LTTNG_ENABLED) && !defined(USDT_ENABLED) */

#ifndef __RXE_TRACE_H__
#define __RXE_TRACE_H__
Expand All @@ -56,4 +71,4 @@ LTTNG_UST_TRACEPOINT_EVENT(

#endif /* __RXE_TRACE_H__*/

#endif /* defined(LTTNG_ENABLED) */
#endif /* defined(LTTNG_ENABLED) || defined(USDT_ENABLED) */
1 change: 1 addition & 0 deletions util/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ publish_internal_headers(util
node_name_map.h
rdma_nl.h
symver.h
usdt.h
util.h
)

Expand Down
Loading
Loading