Message ID | 20200413150116.734047-1-jerinj@marvell.com (mailing list archive) |
---|---|
Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@inbox.dpdk.org Delivered-To: patchwork@inbox.dpdk.org Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E9ED4A0577; Mon, 13 Apr 2020 17:01:39 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2A7CD1B53; Mon, 13 Apr 2020 17:01:39 +0200 (CEST) Received: from mx0b-0016f401.pphosted.com (mx0b-0016f401.pphosted.com [67.231.156.173]) by dpdk.org (Postfix) with ESMTP id D589CF12 for <dev@dpdk.org>; Mon, 13 Apr 2020 17:01:37 +0200 (CEST) Received: from pps.filterd (m0045851.ppops.net [127.0.0.1]) by mx0b-0016f401.pphosted.com (8.16.0.42/8.16.0.42) with SMTP id 03DF1NLD023086; Mon, 13 Apr 2020 08:01:37 -0700 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=marvell.com; h=from : to : cc : subject : date : message-id : in-reply-to : references : mime-version : content-type : content-transfer-encoding; s=pfpt0818; bh=B7lBVf2PlLlp6UFJNOy1b2BxBH39ddhugHv24c/xtf8=; b=tRbBNynXN7jZyw+LBGaajDiiN5z/FZtZ8hqUV4DnavG+osK1dAoqX/hF+y9ZR9ZrAGLc FRfm8uLrhVJl+NYT2lyT1dJ71iyBzkVsORT+zuSZ8fxZcLivB5tU27qboE+soTMyty3E I4AVl3yyqUnwaN4gyJk/l7th4uJY9abpaS6Md1kpaVrJu18ldW5a92P8Nkg5jr/hbbcI jti9OqFMAGpFuUEYJNl2wBCbhmLccMz4NozTCLsguctoEriuM76xf645jiNjuZgXxdcx VJ1FT7IEl3fmtRFyUF0V3XkazrGn+b7a4Jb8L/z4UWrWPghxs4AKpNKsBJ991YVcZaDI kA== Received: from sc-exch01.marvell.com ([199.233.58.181]) by mx0b-0016f401.pphosted.com with ESMTP id 30bddkpf23-2 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-SHA384 bits=256 verify=NOT); Mon, 13 Apr 2020 08:01:37 -0700 Received: from DC5-EXCH01.marvell.com (10.69.176.38) by SC-EXCH01.marvell.com (10.93.176.81) with Microsoft SMTP Server (TLS) id 15.0.1497.2; Mon, 13 Apr 2020 08:01:35 -0700 Received: from maili.marvell.com (10.69.176.80) by DC5-EXCH01.marvell.com (10.69.176.38) with Microsoft SMTP Server id 15.0.1497.2 via Frontend Transport; Mon, 13 Apr 2020 08:01:35 -0700 Received: from jerin-lab.marvell.com (jerin-lab.marvell.com [10.28.34.14]) by maili.marvell.com (Postfix) with ESMTP id ED71A3F70F7; Mon, 13 Apr 2020 08:01:08 -0700 (PDT) From: <jerinj@marvell.com> To: CC: <dev@dpdk.org>, <thomas@monjalon.net>, <bruce.richardson@intel.com>, <david.marchand@redhat.com>, <mattias.ronnblom@ericsson.com>, <skori@marvell.com>, Jerin Jacob <jerinj@marvell.com> Date: Mon, 13 Apr 2020 20:30:43 +0530 Message-ID: <20200413150116.734047-1-jerinj@marvell.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <20200403153709.3703448-1-jerinj@marvell.com> References: <20200403153709.3703448-1-jerinj@marvell.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.138, 18.0.676 definitions=2020-04-13_07:2020-04-13, 2020-04-13 signatures=0 Subject: [dpdk-dev] [PATCH v5 00/33] DPDK Trace support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://mails.dpdk.org/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://mails.dpdk.org/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://mails.dpdk.org/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Series |
DPDK Trace support
|
|
Message
Jerin Jacob Kollanukkaran
April 13, 2020, 3 p.m. UTC
From: Jerin Jacob <jerinj@marvell.com>
This patch depends on http://patches.dpdk.org/patch/68119/
Depends-on:series-9295
v5
~~
Following API rework based David and Thomas feedback.
1) Rename - "Shell pattern" to "Globbing pattern"
2) Remove the log "level" notion from trace library.
3) Remove rte_trace_global_[level/mode]* API from trace library.
4) EAL command line arg to --trace=regex/globing from --trace-level=regex/globing:level
5) Remove "@b EXPERIMENTAL: this API may change without prior notice" from each functions.
6) Use FP instead of DP for fastpath reference.
7) Updated documentation to reflect the above rework.
8) Updated UT to to reflect the above rework.
8) Updated UT for a FP trace point emit.
9) Updated performance test case for a FP trace point emission.
10) Updated git commit comments to reflect the above rework.
v4:
~~
1) Rebased to master.
2) Adapted to latest EAL directory structure change.
3) Fix possible build issue with out of tree application wherein
it does not define -DALLOW_EXPERIMENTAL_API. Fixed by making
fast path trace functions as NOP as it was getting included
in the inline functions of ethdev,mempool, cryptodev, etc(David)
4) Removed DALLOW_EXPERIMENTAL_API definition from individual driver files.
Now it set as global using http://patches.dpdk.org/patch/67758/ patch (David)
5) Added new meson option(-Denable_trace_dp=true)for enabling the datapath trace point (David, Bruce)
6) Changed the authorship and Rewrote the programmer's guide based on the below feedback(Thomas)
http://patches.dpdk.org/patch/67352/
v3:
~~
1) Fix the following build issues reported by CI
http://mails.dpdk.org/archives/test-report/2020-March/122060.html
a) clang + i686 meson build issue(Fixed in the the patch meson: add libatomic as a global dependency for i686 clang)
b) fixed build issue with FreeBSD with top of tree change.
c) fixed missing experimental API for iavf and ice with meson build in avx512 files.
v2:
~~
Addressed the following review comments from Mattias Rönnblom:
1) Changed
from:
typedef uint64_t* rte_trace_t;
to
typedef uint64_t rte_trace_t;
Initially thought to make the handle as
struct rte_trace {
uint64_t val;
}
but changed to uint64_t for the following reasons
a) It is opaque to the application and it will fix the compile-time type
check as well.
b) The handle has an index that will point to an internal slow-path
structure so no ABI change required in the future.
c) RTE_TRACE_POINT_DEFINE need to expose trace object. So it is better
to keep as uint64_t and avoid one more indirection for no use.
2)
Changed:
from:
enum rte_trace_mode_e {
to:
enum rte_trace_mode {
3) removed [out] "found" param from rte_trace_pattern() and
rte_trace_regexp()
4) Changed rte_trace_from_name to rte_trace_by_name
5) rte_trace_is_dp_enabled() return bool now
6) in __rte_trace_point_register() the argument fn change to register_fn
7) removed !! from rte_trace_is_enabled()
8) Remove uninitialized "rc warning" from rte_trace_pattern() and
rte_trace_regexp()
9) fixup bool return type for trace_entry_compare()
10) fixup calloc casting in trace_mkdir()
11) check fclose() return in trace_meta_save() and trace_mem_save()
12) rte_trace_ctf_* macro cleanup
13) added release notes
14) fix build issues reported by CI
http://mails.dpdk.org/archives/test-report/2020-March/121235.html
This patch set contains
~~~~~~~~~~~~~~~~~~~~~~~~
# The native implementation of common trace format(CTF)[1] based tracer
# Public API to create the trace points.
# Add tracepoints to eal, ethdev, mempool, eventdev and cryptodev
library for tracing support
# A unit test case
# Performance test case to measure the trace overhead. (See eal/trace:
# add trace performance test cases, patch)
# Programmers guide for Trace support(See doc: add trace library guide,
# patch)
# Tested OS:
~~~~~~~~~~~
- Linux
- FreeBSD
# Tested open source CTF trace viewers
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
- Babeltrace
- Tracecompass
# Trace overhead comparison with LTTng
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
trace overhead data on x86:[2]
# 236 cycles with LTTng(>100ns)
# 18 cycles(7ns) with Native DPDK CTF emitter.(See eal/trace: add trace
# performance test cases patch)
trace overhead data on arm64:
# 312 cycles to 1100 cycles with LTTng based on the class of arm64
# CPU.
# 11 cycles to 13 cycles with Native DPDK CTF emitter based on the
class of arm64 CPU.
18 cycles(on x86) vs 11 cycles(on arm64) is due to rdtsc() overhead in
x86. It seems rdtsc takes around 15cycles in x86.
More details:
~~~~~~~~~~~~~
# The Native DPDK CTF trace support does not have any dependency on
third-party library.
The generated output file is compatible with LTTng as both are using
CTF trace format.
The performance gain comes from:
1) exploit dpdk worker thread usage model to avoid atomics and use per
core variables
2) use hugepage,
3) avoid a lot function pointers in fast-path etc
4) avoid unaligned store for arm64 etc
Features:
~~~~~~~~~
- No specific limit on the events. A string-based event like rte_log
for pattern matching
- Dynamic enable/disable support.
- Instructmention overhead is ~1 cycle. i.e cost of adding the code
wth out using trace feature.
- Timestamp support for all the events using DPDK rte_rtdsc
- No dependency on another library. Clean room native implementation of
CTF.
Functional test case:
a) echo "trace_autotest" | sudo ./build/app/test/dpdk-test -c 0x3 --trace=.*
The above command emits the following trace events
<code>
uint8_t i;
rte_trace_lib_eal_generic_void();
rte_trace_lib_eal_generic_u64(0x10000000000000);
rte_trace_lib_eal_generic_u32(0x10000000);
rte_trace_lib_eal_generic_u16(0xffee);
rte_trace_lib_eal_generic_u8(0xc);
rte_trace_lib_eal_generic_i64(-1234);
rte_trace_lib_eal_generic_i32(-1234567);
rte_trace_lib_eal_generic_i16(12);
rte_trace_lib_eal_generic_i8(-3);
rte_trace_lib_eal_generic_string("my string");
rte_trace_lib_eal_generic_function(__func__);
</code>
Install babeltrace package in Linux and point the generated trace file
to babel trace. By default trace file created under
<user>/dpdk-traces/time_stamp/
example:
# babeltrace /root/dpdk-traces/rte-2020-02-15-PM-02-56-51 | more
[13:27:36.138468807] (+?.?????????) lib.eal.generic.void: { cpu_id =0, name = "dpdk-test" }, { }
[13:27:36.138468851] (+0.000000044) lib.eal.generic.u64: { cpu_id = 0, name = "dpdk-test" }, { in = 4503599627370496 }
[13:27:36.138468860] (+0.000000009) lib.eal.generic.u32: { cpu_id = 0, name = "dpdk-test" }, { in = 268435456 }
[13:27:36.138468934] (+0.000000074) lib.eal.generic.u16: { cpu_id = 0, name = "dpdk-test" }, { in = 65518 }
[13:27:36.138468949] (+0.000000015) lib.eal.generic.u8: { cpu_id = 0, name = "dpdk-test" }, { in = 12 }
[13:27:36.138468956] (+0.000000007) lib.eal.generic.i64: { cpu_id = 0, name = "dpdk-test" }, { in = -1234 }
[13:27:36.138468963] (+0.000000007) lib.eal.generic.i32: { cpu_id = 0, name = "dpdk-test" }, { in = -1234567 }
[13:27:36.138469024] (+0.000000061) lib.eal.generic.i16: { cpu_id = 0, name = "dpdk-test" }, { in = 12 }
[13:27:36.138469044] (+0.000000020) lib.eal.generic.i8: { cpu_id = 0, name = "dpdk-test" }, { in = -3 }
[13:27:36.138469051] (+0.000000007) lib.eal.generic.string: { cpu_id = 0, name = "dpdk-test" }, { str = "my string" }
[13:27:36.138469203] (+0.000000152) lib.eal.generic.func: { cpu_id = 0, name = "dpdk-test" }, { func = "test_trace_points" }
# There is a GUI based trace viewer available in Windows, Linux and Mac.
It is called as tracecompass.(https://www.eclipse.org/tracecompass/)
The example screenshot and Histogram of above DPDK trace using
Tracecompass.
https://github.com/jerinjacobk/share/blob/master/dpdk_trace.JPG
File walk through:
~~~~~~~~~~~~~~~~~~
lib/librte_eal/common/include/rte_trace.h - Public API for Trace
provider and Trace control
lib/librte_eal/common/eal_common_trace.c - main trace implementation
lib/librte_eal/common/eal_common_trace_ctf.c - CTF metadata spec
implementation
lib/librte_eal/common/eal_common_trace_utils.c - command line utils
and filesystem operations.
lib/librte_eal/common/eal_common_trace_points.c - trace points for EAL
library
lib/librte_eal/common/include/rte_trace_eal.h - EAL tracepoint public
API.
lib/librte_eal/common/eal_trace.h - Private trace header file.
[1] https://diamon.org/ctf/
[2] The above test is ported to LTTng for finding the LTTng trace
overhead. It available at
https://github.com/jerinjacobk/lttng-overhead
https://github.com/jerinjacobk/lttng-overhead/blob/master/README
Jerin Jacob (22):
eal: introduce API for getting thread name
eal/trace: define the public API for trace support
eal/trace: implement trace register API
eal/trace: implement trace operation APIs
eal/trace: add internal trace init and fini interface
eal/trace: get bootup timestamp for trace
eal/trace: create CTF TDSL metadata in memory
eal/trace: implement trace memory allocation
eal/trace: implement debug dump function
eal/trace: implement trace save
eal/trace: implement registration payload
eal/trace: implement provider payload
eal/trace: hook internal trace APIs to Linux
eal/trace: hook internal trace APIs to FreeBSD
eal/trace: add generic tracepoints
eal/trace: add alarm tracepoints
eal/trace: add memory tracepoints
eal/trace: add memzone tracepoints
eal/trace: add thread tracepoints
eal/trace: add interrupt tracepoints
eal/trace: add trace performance test cases
doc: add trace library guide
Pavan Nikhilesh (1):
meson: add libatomic as a global dependency for i686 clang
Sunil Kumar Kori (10):
eal/trace: handle CTF keyword collision
eal/trace: add trace configuration parameter
eal/trace: add trace dir configuration parameter
eal/trace: add trace bufsize configuration parameter
eal/trace: add trace mode configuration parameter
eal/trace: add unit test cases
ethdev: add tracepoints
eventdev: add tracepoints
cryptodev: add tracepoints
mempool: add tracepoints
MAINTAINERS | 8 +
app/test/Makefile | 4 +-
app/test/meson.build | 3 +
app/test/test_trace.c | 258 +++++++++
app/test/test_trace.h | 15 +
app/test/test_trace_perf.c | 183 +++++++
app/test/test_trace_register.c | 19 +
config/common_base | 1 +
config/meson.build | 9 +
doc/api/doxy-api-index.md | 3 +-
doc/guides/linux_gsg/eal_args.include.rst | 52 ++
doc/guides/prog_guide/build-sdk-meson.rst | 5 +
doc/guides/prog_guide/index.rst | 1 +
doc/guides/prog_guide/trace_lib.rst | 334 +++++++++++
doc/guides/rel_notes/release_20_05.rst | 9 +
drivers/event/octeontx/meson.build | 5 -
drivers/event/octeontx2/meson.build | 5 -
drivers/event/opdl/meson.build | 5 -
examples/cmdline/Makefile | 1 +
examples/cmdline/meson.build | 1 +
examples/distributor/Makefile | 1 +
examples/distributor/meson.build | 1 +
examples/ethtool/ethtool-app/Makefile | 1 +
examples/eventdev_pipeline/meson.build | 1 +
examples/flow_filtering/Makefile | 1 +
examples/flow_filtering/meson.build | 1 +
examples/helloworld/Makefile | 1 +
examples/helloworld/meson.build | 1 +
examples/ioat/Makefile | 1 +
examples/ioat/meson.build | 1 +
examples/ip_fragmentation/Makefile | 2 +
examples/ip_fragmentation/meson.build | 1 +
examples/ip_reassembly/Makefile | 1 +
examples/ip_reassembly/meson.build | 1 +
examples/ipv4_multicast/Makefile | 1 +
examples/ipv4_multicast/meson.build | 1 +
examples/l2fwd-cat/Makefile | 1 +
examples/l2fwd-cat/meson.build | 1 +
examples/l2fwd-event/Makefile | 1 +
examples/l2fwd-event/meson.build | 6 +-
examples/l2fwd-jobstats/Makefile | 1 +
examples/l2fwd-jobstats/meson.build | 1 +
examples/l2fwd-keepalive/Makefile | 1 +
examples/l2fwd-keepalive/ka-agent/Makefile | 1 +
examples/l2fwd-keepalive/meson.build | 1 +
examples/l3fwd-acl/Makefile | 1 +
examples/l3fwd-acl/meson.build | 1 +
examples/l3fwd/Makefile | 1 +
examples/l3fwd/meson.build | 1 +
examples/link_status_interrupt/Makefile | 1 +
examples/link_status_interrupt/meson.build | 1 +
.../client_server_mp/mp_client/Makefile | 1 +
.../client_server_mp/mp_client/meson.build | 1 +
.../client_server_mp/mp_server/meson.build | 1 +
examples/multi_process/hotplug_mp/Makefile | 1 +
examples/multi_process/hotplug_mp/meson.build | 1 +
examples/multi_process/simple_mp/Makefile | 1 +
examples/multi_process/simple_mp/meson.build | 1 +
examples/multi_process/symmetric_mp/Makefile | 1 +
.../multi_process/symmetric_mp/meson.build | 1 +
examples/ntb/Makefile | 1 +
examples/ntb/meson.build | 1 +
examples/packet_ordering/Makefile | 1 +
examples/packet_ordering/meson.build | 1 +
.../performance-thread/l3fwd-thread/Makefile | 1 +
.../l3fwd-thread/meson.build | 1 +
.../performance-thread/pthread_shim/Makefile | 1 +
.../pthread_shim/meson.build | 1 +
examples/ptpclient/Makefile | 1 +
examples/ptpclient/meson.build | 1 +
examples/qos_meter/Makefile | 1 +
examples/qos_meter/meson.build | 1 +
examples/qos_sched/Makefile | 1 +
examples/qos_sched/meson.build | 1 +
examples/server_node_efd/node/Makefile | 1 +
examples/server_node_efd/node/meson.build | 1 +
examples/server_node_efd/server/Makefile | 1 +
examples/server_node_efd/server/meson.build | 1 +
examples/service_cores/Makefile | 1 +
examples/service_cores/meson.build | 1 +
examples/skeleton/Makefile | 1 +
examples/skeleton/meson.build | 1 +
examples/timer/Makefile | 1 +
examples/timer/meson.build | 1 +
examples/vm_power_manager/Makefile | 1 +
examples/vm_power_manager/meson.build | 1 +
examples/vmdq/Makefile | 1 +
examples/vmdq/meson.build | 1 +
examples/vmdq_dcb/Makefile | 1 +
examples/vmdq_dcb/meson.build | 1 +
lib/librte_cryptodev/Makefile | 4 +-
lib/librte_cryptodev/cryptodev_trace_points.c | 70 +++
lib/librte_cryptodev/meson.build | 6 +-
lib/librte_cryptodev/rte_cryptodev.c | 18 +
lib/librte_cryptodev/rte_cryptodev.h | 6 +
.../rte_cryptodev_version.map | 18 +
lib/librte_cryptodev/rte_trace_cryptodev.h | 133 +++++
lib/librte_cryptodev/rte_trace_cryptodev_fp.h | 34 ++
lib/librte_distributor/meson.build | 5 -
lib/librte_eal/common/eal_common_memzone.c | 9 +
lib/librte_eal/common/eal_common_options.c | 65 +++
lib/librte_eal/common/eal_common_thread.c | 3 +-
lib/librte_eal/common/eal_common_trace.c | 518 ++++++++++++++++++
lib/librte_eal/common/eal_common_trace_ctf.c | 488 +++++++++++++++++
.../common/eal_common_trace_points.c | 115 ++++
.../common/eal_common_trace_utils.c | 478 ++++++++++++++++
lib/librte_eal/common/eal_options.h | 8 +
lib/librte_eal/common/eal_private.h | 5 +
lib/librte_eal/common/eal_trace.h | 121 ++++
lib/librte_eal/common/meson.build | 4 +
lib/librte_eal/common/rte_malloc.c | 60 +-
lib/librte_eal/freebsd/Makefile | 4 +
lib/librte_eal/freebsd/eal.c | 10 +
lib/librte_eal/freebsd/eal_alarm.c | 3 +
lib/librte_eal/freebsd/eal_interrupts.c | 54 +-
lib/librte_eal/freebsd/eal_thread.c | 21 +-
lib/librte_eal/include/meson.build | 4 +
lib/librte_eal/include/rte_lcore.h | 17 +
lib/librte_eal/include/rte_trace.h | 424 ++++++++++++++
lib/librte_eal/include/rte_trace_eal.h | 247 +++++++++
lib/librte_eal/include/rte_trace_provider.h | 157 ++++++
lib/librte_eal/include/rte_trace_register.h | 52 ++
lib/librte_eal/linux/Makefile | 4 +
lib/librte_eal/linux/eal.c | 9 +
lib/librte_eal/linux/eal_alarm.c | 4 +
lib/librte_eal/linux/eal_interrupts.c | 84 +--
lib/librte_eal/linux/eal_thread.c | 27 +-
lib/librte_eal/rte_eal_version.map | 53 ++
lib/librte_ethdev/Makefile | 3 +
lib/librte_ethdev/ethdev_trace_points.c | 43 ++
lib/librte_ethdev/meson.build | 5 +-
lib/librte_ethdev/rte_ethdev.c | 12 +
lib/librte_ethdev/rte_ethdev.h | 5 +
lib/librte_ethdev/rte_ethdev_version.map | 10 +
lib/librte_ethdev/rte_trace_ethdev.h | 90 +++
lib/librte_ethdev/rte_trace_ethdev_fp.h | 40 ++
lib/librte_eventdev/Makefile | 3 +
lib/librte_eventdev/eventdev_trace_points.c | 173 ++++++
lib/librte_eventdev/meson.build | 3 +
.../rte_event_crypto_adapter.c | 10 +
.../rte_event_eth_rx_adapter.c | 11 +
.../rte_event_eth_tx_adapter.c | 13 +-
.../rte_event_eth_tx_adapter.h | 2 +
lib/librte_eventdev/rte_event_timer_adapter.c | 8 +-
lib/librte_eventdev/rte_event_timer_adapter.h | 8 +
lib/librte_eventdev/rte_eventdev.c | 9 +
lib/librte_eventdev/rte_eventdev.h | 5 +-
lib/librte_eventdev/rte_eventdev_version.map | 42 ++
lib/librte_eventdev/rte_trace_eventdev.h | 278 ++++++++++
lib/librte_eventdev/rte_trace_eventdev_fp.h | 75 +++
lib/librte_mempool/Makefile | 3 +
lib/librte_mempool/mempool_trace_points.c | 108 ++++
lib/librte_mempool/meson.build | 5 +-
lib/librte_mempool/rte_mempool.c | 16 +
lib/librte_mempool/rte_mempool.h | 13 +
lib/librte_mempool/rte_mempool_ops.c | 7 +
lib/librte_mempool/rte_mempool_version.map | 26 +
lib/librte_mempool/rte_trace_mempool.h | 148 +++++
lib/librte_mempool/rte_trace_mempool_fp.h | 102 ++++
lib/librte_rcu/meson.build | 5 -
meson_options.txt | 2 +
161 files changed, 5517 insertions(+), 105 deletions(-)
create mode 100644 app/test/test_trace.c
create mode 100644 app/test/test_trace.h
create mode 100644 app/test/test_trace_perf.c
create mode 100644 app/test/test_trace_register.c
create mode 100644 doc/guides/prog_guide/trace_lib.rst
create mode 100644 lib/librte_cryptodev/cryptodev_trace_points.c
create mode 100644 lib/librte_cryptodev/rte_trace_cryptodev.h
create mode 100644 lib/librte_cryptodev/rte_trace_cryptodev_fp.h
create mode 100644 lib/librte_eal/common/eal_common_trace.c
create mode 100644 lib/librte_eal/common/eal_common_trace_ctf.c
create mode 100644 lib/librte_eal/common/eal_common_trace_points.c
create mode 100644 lib/librte_eal/common/eal_common_trace_utils.c
create mode 100644 lib/librte_eal/common/eal_trace.h
create mode 100644 lib/librte_eal/include/rte_trace.h
create mode 100644 lib/librte_eal/include/rte_trace_eal.h
create mode 100644 lib/librte_eal/include/rte_trace_provider.h
create mode 100644 lib/librte_eal/include/rte_trace_register.h
create mode 100644 lib/librte_ethdev/ethdev_trace_points.c
create mode 100644 lib/librte_ethdev/rte_trace_ethdev.h
create mode 100644 lib/librte_ethdev/rte_trace_ethdev_fp.h
create mode 100644 lib/librte_eventdev/eventdev_trace_points.c
create mode 100644 lib/librte_eventdev/rte_trace_eventdev.h
create mode 100644 lib/librte_eventdev/rte_trace_eventdev_fp.h
create mode 100644 lib/librte_mempool/mempool_trace_points.c
create mode 100644 lib/librte_mempool/rte_trace_mempool.h
create mode 100644 lib/librte_mempool/rte_trace_mempool_fp.h
Comments
Thanks Jerin for this new version. New round of comments. - What do you think of splitting the API in two headers, thinking about who will use them? * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for users of the trace framework that want to * get the status of the whole trace subsystem, * enable/disable tracepoints by pattern/regexp, * dump the current events, * rte_tracepoint.h (rte_tracepoint_ prefix for all functions/macros/types) for developers that want to add tracepoints to their code - Having functions "is_disabled" has little value when a "is_enabled" counterpart exists. - What is the value of having a _public_ rte_trace_is_invalid() ? A final user would need to lookup by name to get a trace descriptor and we should hope that such a lookup returns a valid descriptor :-). A developer would already have the descriptor hook point in his own code: by construction, if the tracepoint is registered, then the descriptor is valid, else, it is unknown. - I did not get why we put the trace descriptors in a specific elf section, can you explain the benefits? - I can see no protection on the tracepoint list. Could we have issues with control/application threads that dpdk does not control, dynamic loading of libraries.. ? - Following comment on v4 and the removal of the mode per tracepoint api, don't we need to put the current select mode in each tracepoint descriptor when registering a trace point ?
15/04/2020 15:26, David Marchand: > - What do you think of splitting the API in two headers, thinking > about who will use them? > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > users of the trace framework that want to > * get the status of the whole trace subsystem, > * enable/disable tracepoints by pattern/regexp, > * dump the current events, > * rte_tracepoint.h (rte_tracepoint_ prefix for all > functions/macros/types) for developers that want to add tracepoints to > their code I like this idea. > - Having functions "is_disabled" has little value when a "is_enabled" > counterpart exists. Yes, which one do we choose? is_enabled? > - What is the value of having a _public_ rte_trace_is_invalid() ? > A final user would need to lookup by name to get a trace descriptor > and we should hope that such a lookup returns a valid descriptor :-). > A developer would already have the descriptor hook point in his own > code: by construction, if the tracepoint is registered, then the > descriptor is valid, else, it is unknown. > > > - I did not get why we put the trace descriptors in a specific elf > section, can you explain the benefits? > > > - I can see no protection on the tracepoint list. Could we have issues > with control/application threads that dpdk does not control, dynamic > loading of libraries.. ? > > > - Following comment on v4 and the removal of the mode per tracepoint > api, don't we need to put the current select mode in each tracepoint > descriptor when registering a trace point ?
On Wed, Apr 15, 2020 at 6:56 PM David Marchand <david.marchand@redhat.com> wrote: > > Thanks Jerin for this new version. > New round of comments. Thanks for the review. > > - What do you think of splitting the API in two headers, thinking > about who will use them? > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > users of the trace framework that want to > * get the status of the whole trace subsystem, > * enable/disable tracepoints by pattern/regexp, > * dump the current events, > * rte_tracepoint.h (rte_tracepoint_ prefix for all > functions/macros/types) for developers that want to add tracepoints to > their code # Initially, I thought of doing the same. Later realized that some of the definitions such as following 1) /** The trace object. The trace APIs are based on this opaque object. */ typedef uint64_t rte_trace_t; 2) rte_trace_fp_is_enabled() Used in both the header file. i.e rte_tracepoint.h needed to include rte_trace.h. If that's is the case, I don't see any value. # Regarding the API change the following to rte_tracepoint_* #define rte_trace_ctf_u64(val) #define rte_trace_ctf_i64(val) #define rte_trace_ctf_u32(val) #define rte_trace_ctf_i32(val) #define rte_trace_ctf_u16(val) #define rte_trace_ctf_i16(val) #define rte_trace_ctf_u8(val) #define rte_trace_ctf_i8(val) #define rte_trace_ctf_int(val) #define rte_trace_ctf_long(val) #define rte_trace_ctf_float(val) #define rte_trace_ctf_double(val) #define rte_trace_ctf_ptr(val) #define rte_trace_ctf_string(val) It could be done. Just concerned the length of API will be more. like rte_trace_point_ctf_u64 If you have a strong opinion on this then I can change it. > > > - Having functions "is_disabled" has little value when a "is_enabled" > counterpart exists. I thought so. I like to have "is_enabled". But in the code, "is_disabled" has been used more. Then finally added both in API. Please share your preference, I will do the same in v6. > > > - What is the value of having a _public_ rte_trace_is_invalid() ? > A final user would need to lookup by name to get a trace descriptor > and we should hope that such a lookup returns a valid descriptor :-). > A developer would already have the descriptor hook point in his own > code: by construction, if the tracepoint is registered, then the > descriptor is valid, else, it is unknown. Yep. I will make rte_trace_is_invalid as internal trace_is_invalid() API. > > > - I did not get why we put the trace descriptors in a specific elf > section, can you explain the benefits? Those are global variables of size 8B. Since it is used in fastpath. I just want all of them to same area for 1) It is a mostly a read-only area. Not mix with other "write" global variables 2) Chances that same subsystem FP variables comes same fastpath cache line. In short, We will be more predictable performance number from build to build i.e not depended on other global variables from build to build. > > - I can see no protection on the tracepoint list. Could we have issues > with control/application threads that dpdk does not control, dynamic > loading of libraries.. ? Based on my understanding, all constructors in the .so file should be called in eal_plugins_init() one by one. So there is no synchronization issue. eal_trace_init() which is called after eal_plugins_init(). So all the .so files with constructor should be called in eal_plugins_init() be it DPDK shared lib or non DPDK shared lib. rte_log_register() does the similar thing. > > > - Following comment on v4 and the removal of the mode per tracepoint > api, don't we need to put the current select mode in each tracepoint > descriptor when registering a trace point ? RTE_TRACE_POINT_REGISTER has only trace and name. RTE_TRACE_POINT_REGISTER(trace, name) Not sure why we need to add "mode" in the trace register. Reason for adding in the descriptor we discussed last time. --------------------------------------------------------------- > In fastpath, I tried to avoid reading multiple control variables to > improve performance. > i.e the tracepoint(uint64_t) has all control information. So it has > given a feature to control mode per tracepoint. > I thought it may be useful. No strong option here. > > If you think, it is not usefull and creates more problems, I will > change to the following: > > 1) Remove int rte_trace_mode_set(rte_trace_t trace, enum rte_trace_mode_e mode); > 2) Change > From: > void rte_trace_global_mode_set(enum rte_trace_mode_e mode); > to: > void rte_trace_mode_set(enum rte_trace_mode_e mode); Yes, you can keep the information in the tracepoint descriptor and expose a single api that impacts all tracepoints. +1 for me. -------------------------------------------------------------------- > > > -- > David Marchand >
On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > > - What do you think of splitting the API in two headers, thinking > > about who will use them? > > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > > users of the trace framework that want to > > * get the status of the whole trace subsystem, > > * enable/disable tracepoints by pattern/regexp, > > * dump the current events, > > * rte_tracepoint.h (rte_tracepoint_ prefix for all > > functions/macros/types) for developers that want to add tracepoints to > > their code > > # Initially, I thought of doing the same. > Later realized that some of the definitions such as following > > 1) > /** The trace object. The trace APIs are based on this opaque object. */ > typedef uint64_t rte_trace_t; As a user, I would ask the trace framework to enable tracepoints by calling rte_trace_pattern()/rte_trace_regexp(). This does not require the tracepoint descriptor to be exposed in rte_trace.h. If some application wants to store/manipulate the descriptors, then it will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object and API are: - rte_tracepoint_lookup (currently named rte_trace_by_name) - rte_tracepoint_enable - rte_tracepoint_disable - rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid, should be private, as discussed) - rte_tracepoint_is_enabled - RTE_TRACEPOINT/_FP macros - rte_tracepoint_register etc... > > 2) rte_trace_fp_is_enabled() As a user, what information would this give me? "Some fastpath tracepoints are not available" Moving to rte_tracepoint.h is enough to me. > # Regarding the API change the following to rte_tracepoint_* > > #define rte_trace_ctf_u64(val) > #define rte_trace_ctf_i64(val) > #define rte_trace_ctf_u32(val) > #define rte_trace_ctf_i32(val) > #define rte_trace_ctf_u16(val) > #define rte_trace_ctf_i16(val) > #define rte_trace_ctf_u8(val) > #define rte_trace_ctf_i8(val) > #define rte_trace_ctf_int(val) > #define rte_trace_ctf_long(val) > #define rte_trace_ctf_float(val) > #define rte_trace_ctf_double(val) > #define rte_trace_ctf_ptr(val) > #define rte_trace_ctf_string(val) > It could be done. Just concerned the length of API will be more. like > rte_trace_point_ctf_u64 > If you have a strong opinion on this then I can change it. I don't like mentioning ctf here. I went with a git grep -l rte_trace_ctf |xargs sed -i -e 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. If we keep one rte_tracepoint_emit_ per line in tracepoint declarations, the length is not an issue by looking at how they are used. Example: RTE_TRACEPOINT( rte_trace_lib_eal_intr_disable, RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), rte_tracepoint_emit_int(rc); rte_tracepoint_emit_int(handle->vfio_dev_fd); rte_tracepoint_emit_int(handle->fd); rte_tracepoint_emit_int(handle->type); rte_tracepoint_emit_u32(handle->max_intr); rte_tracepoint_emit_u32(handle->nb_efd); ) Besides, we don't need to define all those rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in rte_tracepoint_provider.h and rte_tracepoint_register.h. If we define a helper rte_tracepoint_emit_data(type, in) in rte_tracepoint.h, then the "provider" and "register" headers must only define how to emit a header (generic and fp cases), then rte_tracepoint_emit_data and rte_tracepoint_emit_string. > > - Having functions "is_disabled" has little value when a "is_enabled" > > counterpart exists. > > I thought so. I like to have "is_enabled". But in the code, > "is_disabled" has been used more. > Then finally added both in API. Please share your preference, I will > do the same in v6. is_enabled is enough. > > - I did not get why we put the trace descriptors in a specific elf > > section, can you explain the benefits? > > Those are global variables of size 8B. Since it is used in fastpath. > I just want all of them to same area for > 1) It is a mostly a read-only area. Not mix with other "write" global variables > 2) Chances that same subsystem FP variables comes same fastpath cache line. > In short, We will be more predictable performance number from build to build i.e > not depended on other global variables from build to build. Ok, thanks for the explanation. It is worth a few words in the associated commitlog for posterity. > > - I can see no protection on the tracepoint list. Could we have issues > > with control/application threads that dpdk does not control, dynamic > > loading of libraries.. ? > > Based on my understanding, all constructors in the .so file should be > called in eal_plugins_init() > one by one. So there is no synchronization issue. eal_trace_init() > which is called after > eal_plugins_init(). So all the .so files with constructor should be > called in eal_plugins_init() > be it DPDK shared lib or non DPDK shared lib. > rte_log_register() does the similar thing. As long as we register in constructor, this is ok. Time will tell us if users want to use this in a different way. > > - Following comment on v4 and the removal of the mode per tracepoint > > api, don't we need to put the current select mode in each tracepoint > > descriptor when registering a trace point ? > > RTE_TRACE_POINT_REGISTER has only trace and name. > RTE_TRACE_POINT_REGISTER(trace, name) > > Not sure why we need to add "mode" in the trace register. I thought of registers happening out of a constructor. But as long as we are in constructors and thanks to the eal_trace_init() after plugin load, the mode is set in all descriptors. So we are fine.
On Thu, Apr 16, 2020 at 7:09 PM David Marchand <david.marchand@redhat.com> wrote: > > On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > > > - What do you think of splitting the API in two headers, thinking > > > about who will use them? > > > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > > > users of the trace framework that want to > > > * get the status of the whole trace subsystem, > > > * enable/disable tracepoints by pattern/regexp, > > > * dump the current events, > > > * rte_tracepoint.h (rte_tracepoint_ prefix for all > > > functions/macros/types) for developers that want to add tracepoints to > > > their code > > > > # Initially, I thought of doing the same. > > Later realized that some of the definitions such as following > > > > 1) > > /** The trace object. The trace APIs are based on this opaque object. */ > > typedef uint64_t rte_trace_t; > > As a user, I would ask the trace framework to enable tracepoints by > calling rte_trace_pattern()/rte_trace_regexp(). > This does not require the tracepoint descriptor to be exposed in rte_trace.h. > > > If some application wants to store/manipulate the descriptors, then it > will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object > and API are: > - rte_tracepoint_lookup (currently named rte_trace_by_name) > - rte_tracepoint_enable > - rte_tracepoint_disable > - rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid, > should be private, as discussed) > - rte_tracepoint_is_enabled > - RTE_TRACEPOINT/_FP macros > - rte_tracepoint_register etc... From the prototype onwards, Myself shuffled abound multiple times on the API name to satisfying names. If you would like to classify based on the tracepoint object dependency to a new header file, it is fine. Let's go the last round for API naming details. I think, trace being the domain, IMO, it better to call the trace point API with rte_trace_point_* and trace point object to rte_trace_point_t (vs rte_tracepoint_t) I will summarise the public API and file name details. Let's finalize. # rte_trace.h will have rte_trace_global_is_enabled rte_trace_mode_set rte_trace_mode_get rte_trace_pattern rte_trace_regexp rte_trace_save rte_trace_metadata_dump rte_trace_dump # rte_trace_point.h will have all operation related to rte_trace_point_t object # rte_trace_provider.h renamed rte_trace_point_provider.h # rte_trace_register.h renamed to rte_trace_point_register.h # rte_trace_eal.h renamed to rte_trace_point_eal.h > > > > > > 2) rte_trace_fp_is_enabled() > > As a user, what information would this give me? > "Some fastpath tracepoints are not available" > > Moving to rte_tracepoint.h is enough to me. IMO, semantically not correct as we are splitting based on some definition. How about, 1) Not expose this API OR 2) rte_trace_point.h includes the rte_trace.h > > > > # Regarding the API change the following to rte_tracepoint_* > > > > #define rte_trace_ctf_u64(val) > > #define rte_trace_ctf_i64(val) > > #define rte_trace_ctf_u32(val) > > #define rte_trace_ctf_i32(val) > > #define rte_trace_ctf_u16(val) > > #define rte_trace_ctf_i16(val) > > #define rte_trace_ctf_u8(val) > > #define rte_trace_ctf_i8(val) > > #define rte_trace_ctf_int(val) > > #define rte_trace_ctf_long(val) > > #define rte_trace_ctf_float(val) > > #define rte_trace_ctf_double(val) > > #define rte_trace_ctf_ptr(val) > > #define rte_trace_ctf_string(val) > > It could be done. Just concerned the length of API will be more. like > > rte_trace_point_ctf_u64 > > If you have a strong opinion on this then I can change it. > > I don't like mentioning ctf here. > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > If we keep one rte_tracepoint_emit_ per line in tracepoint > declarations, the length is not an issue by looking at how they are > used. OK to remove ctf to make it as rte_trace_point_emit_*. OK? > > Example: > RTE_TRACEPOINT( > rte_trace_lib_eal_intr_disable, > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > rte_tracepoint_emit_int(rc); > rte_tracepoint_emit_int(handle->vfio_dev_fd); > rte_tracepoint_emit_int(handle->fd); > rte_tracepoint_emit_int(handle->type); > rte_tracepoint_emit_u32(handle->max_intr); > rte_tracepoint_emit_u32(handle->nb_efd); > ) > > > Besides, we don't need to define all those > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > rte_tracepoint_provider.h and rte_tracepoint_register.h. > If we define a helper rte_tracepoint_emit_data(type, in) in > rte_tracepoint.h, then the "provider" and "register" headers must only > define how to emit a header (generic and fp cases), then > rte_tracepoint_emit_data and rte_tracepoint_emit_string. The reason for rte_tracepoint_emit_(u|i)(8|16|32|64) to get compile to check to correct time type used. See: rte_trace_point_emit_u32 defintion has RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(uint32_t))); > > > > > - Having functions "is_disabled" has little value when a "is_enabled" > > > counterpart exists. > > > > I thought so. I like to have "is_enabled". But in the code, > > "is_disabled" has been used more. > > Then finally added both in API. Please share your preference, I will > > do the same in v6. > > is_enabled is enough. OK. I will remove is_disabled() > > > > > - I did not get why we put the trace descriptors in a specific elf > > > section, can you explain the benefits? > > > > Those are global variables of size 8B. Since it is used in fastpath. > > I just want all of them to same area for > > 1) It is a mostly a read-only area. Not mix with other "write" global variables > > 2) Chances that same subsystem FP variables comes same fastpath cache line. > > In short, We will be more predictable performance number from build to build i.e > > not depended on other global variables from build to build. > > Ok, thanks for the explanation. > It is worth a few words in the associated commitlog for posterity. OK. I add it in git commit. >
16/04/2020 18:08, Jerin Jacob: > On Thu, Apr 16, 2020 at 7:09 PM David Marchand > <david.marchand@redhat.com> wrote: > > > > On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > > > > - What do you think of splitting the API in two headers, thinking > > > > about who will use them? > > > > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > > > > users of the trace framework that want to > > > > * get the status of the whole trace subsystem, > > > > * enable/disable tracepoints by pattern/regexp, > > > > * dump the current events, > > > > * rte_tracepoint.h (rte_tracepoint_ prefix for all > > > > functions/macros/types) for developers that want to add tracepoints to > > > > their code > > > > > > # Initially, I thought of doing the same. > > > Later realized that some of the definitions such as following > > > > > > 1) > > > /** The trace object. The trace APIs are based on this opaque object. */ > > > typedef uint64_t rte_trace_t; > > > > As a user, I would ask the trace framework to enable tracepoints by > > calling rte_trace_pattern()/rte_trace_regexp(). > > This does not require the tracepoint descriptor to be exposed in rte_trace.h. > > > > > > If some application wants to store/manipulate the descriptors, then it > > will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object > > and API are: > > - rte_tracepoint_lookup (currently named rte_trace_by_name) > > - rte_tracepoint_enable > > - rte_tracepoint_disable > > - rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid, > > should be private, as discussed) > > - rte_tracepoint_is_enabled > > - RTE_TRACEPOINT/_FP macros > > - rte_tracepoint_register etc... > > From the prototype onwards, Myself shuffled abound multiple times on > the API name to satisfying > names. > > If you would like to classify based on the tracepoint object > dependency to a new header file, it is fine. > Let's go the last round for API naming details. > > I think, trace being the domain, IMO, it better to call the trace > point API with rte_trace_point_* > and trace point object to rte_trace_point_t (vs rte_tracepoint_t) > > I will summarise the public API and file name details. Let's finalize. > > # rte_trace.h will have > > rte_trace_global_is_enabled > rte_trace_mode_set > rte_trace_mode_get > rte_trace_pattern > rte_trace_regexp > rte_trace_save > rte_trace_metadata_dump > rte_trace_dump > > # rte_trace_point.h will have all operation related to rte_trace_point_t object > > # rte_trace_provider.h renamed rte_trace_point_provider.h > # rte_trace_register.h renamed to rte_trace_point_register.h > # rte_trace_eal.h renamed to rte_trace_point_eal.h > > > > > > > > > > > > 2) rte_trace_fp_is_enabled() > > > > As a user, what information would this give me? > > "Some fastpath tracepoints are not available" > > > > Moving to rte_tracepoint.h is enough to me. > > IMO, semantically not correct as we are splitting based on some definition. Semantically, rte_trace.h must be the API for simple users enabling traces, while rte_trace_point.h would be used by those adding traces. > How about, > 1) Not expose this API > OR > 2) rte_trace_point.h includes the rte_trace.h > > > > > > > > > # Regarding the API change the following to rte_tracepoint_* > > > > > > #define rte_trace_ctf_u64(val) > > > #define rte_trace_ctf_i64(val) > > > #define rte_trace_ctf_u32(val) > > > #define rte_trace_ctf_i32(val) > > > #define rte_trace_ctf_u16(val) > > > #define rte_trace_ctf_i16(val) > > > #define rte_trace_ctf_u8(val) > > > #define rte_trace_ctf_i8(val) > > > #define rte_trace_ctf_int(val) > > > #define rte_trace_ctf_long(val) > > > #define rte_trace_ctf_float(val) > > > #define rte_trace_ctf_double(val) > > > #define rte_trace_ctf_ptr(val) > > > #define rte_trace_ctf_string(val) > > > It could be done. Just concerned the length of API will be more. like > > > rte_trace_point_ctf_u64 > > > If you have a strong opinion on this then I can change it. > > > > I don't like mentioning ctf here. > > > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > > If we keep one rte_tracepoint_emit_ per line in tracepoint > > declarations, the length is not an issue by looking at how they are > > used. > > OK to remove ctf to make it as rte_trace_point_emit_*. OK? > > > > > Example: > > RTE_TRACEPOINT( > > rte_trace_lib_eal_intr_disable, > > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > > rte_tracepoint_emit_int(rc); > > rte_tracepoint_emit_int(handle->vfio_dev_fd); > > rte_tracepoint_emit_int(handle->fd); > > rte_tracepoint_emit_int(handle->type); > > rte_tracepoint_emit_u32(handle->max_intr); > > rte_tracepoint_emit_u32(handle->nb_efd); > > ) > > > > > > Besides, we don't need to define all those > > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > > rte_tracepoint_provider.h and rte_tracepoint_register.h. > > If we define a helper rte_tracepoint_emit_data(type, in) in > > rte_tracepoint.h, then the "provider" and "register" headers must only > > define how to emit a header (generic and fp cases), then > > rte_tracepoint_emit_data and rte_tracepoint_emit_string. > > The reason for rte_tracepoint_emit_(u|i)(8|16|32|64) to get compile > to check to correct time type used. > See: > rte_trace_point_emit_u32 defintion has > RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(uint32_t))); Is it possible to implement it with a common helper as David suggests?
On Thu, Apr 16, 2020 at 9:53 PM Thomas Monjalon <thomas@monjalon.net> wrote: > > 16/04/2020 18:08, Jerin Jacob: > > On Thu, Apr 16, 2020 at 7:09 PM David Marchand > > <david.marchand@redhat.com> wrote: > > > > > > On Wed, Apr 15, 2020 at 4:40 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > > > > > - What do you think of splitting the API in two headers, thinking > > > > > about who will use them? > > > > > * rte_trace.h (rte_trace_ prefix for all functions/macros/types) for > > > > > users of the trace framework that want to > > > > > * get the status of the whole trace subsystem, > > > > > * enable/disable tracepoints by pattern/regexp, > > > > > * dump the current events, > > > > > * rte_tracepoint.h (rte_tracepoint_ prefix for all > > > > > functions/macros/types) for developers that want to add tracepoints to > > > > > their code > > > > > > > > # Initially, I thought of doing the same. > > > > Later realized that some of the definitions such as following > > > > > > > > 1) > > > > /** The trace object. The trace APIs are based on this opaque object. */ > > > > typedef uint64_t rte_trace_t; > > > > > > As a user, I would ask the trace framework to enable tracepoints by > > > calling rte_trace_pattern()/rte_trace_regexp(). > > > This does not require the tracepoint descriptor to be exposed in rte_trace.h. > > > > > > > > > If some application wants to store/manipulate the descriptors, then it > > > will rely on rte_tracepoint.h where the rte_tracepoint_t opaque object > > > and API are: > > > - rte_tracepoint_lookup (currently named rte_trace_by_name) > > > - rte_tracepoint_enable > > > - rte_tracepoint_disable > > > - rte_tracepoint_is_invalid (currently named rte_trace_id_is_invalid, > > > should be private, as discussed) > > > - rte_tracepoint_is_enabled > > > - RTE_TRACEPOINT/_FP macros > > > - rte_tracepoint_register etc... > > > > From the prototype onwards, Myself shuffled abound multiple times on > > the API name to satisfying > > names. > > > > If you would like to classify based on the tracepoint object > > dependency to a new header file, it is fine. > > Let's go the last round for API naming details. > > > > I think, trace being the domain, IMO, it better to call the trace > > point API with rte_trace_point_* > > and trace point object to rte_trace_point_t (vs rte_tracepoint_t) > > > > I will summarise the public API and file name details. Let's finalize. > > > > # rte_trace.h will have > > > > rte_trace_global_is_enabled > > rte_trace_mode_set > > rte_trace_mode_get > > rte_trace_pattern > > rte_trace_regexp > > rte_trace_save > > rte_trace_metadata_dump > > rte_trace_dump > > > > # rte_trace_point.h will have all operation related to rte_trace_point_t object > > > > # rte_trace_provider.h renamed rte_trace_point_provider.h > > # rte_trace_register.h renamed to rte_trace_point_register.h > > # rte_trace_eal.h renamed to rte_trace_point_eal.h > > > > > > > > > > > > > > > > > > 2) rte_trace_fp_is_enabled() > > > > > > As a user, what information would this give me? > > > "Some fastpath tracepoints are not available" > > > > > > Moving to rte_tracepoint.h is enough to me. > > > > IMO, semantically not correct as we are splitting based on some definition. > > Semantically, rte_trace.h must be the API for simple users enabling traces, > while rte_trace_point.h would be used by those adding traces. Yes. The problem is where will be the definition of rte_trace_fp_is_enabled() come? It is used for both needs. > > > > How about, > > 1) Not expose this API > > OR > > 2) rte_trace_point.h includes the rte_trace.h > > > > > > > > > > > > > > # Regarding the API change the following to rte_tracepoint_* > > > > > > > > #define rte_trace_ctf_u64(val) > > > > #define rte_trace_ctf_i64(val) > > > > #define rte_trace_ctf_u32(val) > > > > #define rte_trace_ctf_i32(val) > > > > #define rte_trace_ctf_u16(val) > > > > #define rte_trace_ctf_i16(val) > > > > #define rte_trace_ctf_u8(val) > > > > #define rte_trace_ctf_i8(val) > > > > #define rte_trace_ctf_int(val) > > > > #define rte_trace_ctf_long(val) > > > > #define rte_trace_ctf_float(val) > > > > #define rte_trace_ctf_double(val) > > > > #define rte_trace_ctf_ptr(val) > > > > #define rte_trace_ctf_string(val) > > > > It could be done. Just concerned the length of API will be more. like > > > > rte_trace_point_ctf_u64 > > > > If you have a strong opinion on this then I can change it. > > > > > > I don't like mentioning ctf here. > > > > > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > > > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > > > If we keep one rte_tracepoint_emit_ per line in tracepoint > > > declarations, the length is not an issue by looking at how they are > > > used. > > > > OK to remove ctf to make it as rte_trace_point_emit_*. OK? > > > > > > > > Example: > > > RTE_TRACEPOINT( > > > rte_trace_lib_eal_intr_disable, > > > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > > > rte_tracepoint_emit_int(rc); > > > rte_tracepoint_emit_int(handle->vfio_dev_fd); > > > rte_tracepoint_emit_int(handle->fd); > > > rte_tracepoint_emit_int(handle->type); > > > rte_tracepoint_emit_u32(handle->max_intr); > > > rte_tracepoint_emit_u32(handle->nb_efd); > > > ) > > > > > > > > > Besides, we don't need to define all those > > > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > > > rte_tracepoint_provider.h and rte_tracepoint_register.h. > > > If we define a helper rte_tracepoint_emit_data(type, in) in > > > rte_tracepoint.h, then the "provider" and "register" headers must only > > > define how to emit a header (generic and fp cases), then > > > rte_tracepoint_emit_data and rte_tracepoint_emit_string. > > > > The reason for rte_tracepoint_emit_(u|i)(8|16|32|64) to get compile > > to check to correct time type used. > > See: > > rte_trace_point_emit_u32 defintion has > > RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(uint32_t))); > > Is it possible to implement it with a common helper as David suggests? It is already present. See. #define __rte_trace_emit_datatype(in, type)\ do {\ RTE_BUILD_BUG_ON(sizeof(type) != sizeof(typeof(in)));\ __rte_trace_emit_ctf_field(sizeof(type), RTE_STR(in), RTE_STR(type));\ } while (0) #define rte_trace_ctf_u64(in) __rte_trace_emit_datatype(in, uint64_t) #define rte_trace_ctf_i64(in) __rte_trace_emit_datatype(in, int64_t) #define rte_trace_ctf_u32(in) __rte_trace_emit_datatype(in, uint32_t) #define rte_trace_ctf_i32(in) __rte_trace_emit_datatype(in, int32_t) #define rte_trace_ctf_u16(in) __rte_trace_emit_datatype(in, uint16_t) #define rte_trace_ctf_i16(in) __rte_trace_emit_datatype(in, int16_t) #define rte_trace_ctf_u8(in) __rte_trace_emit_datatype(in, uint8_t) #define rte_trace_ctf_i8(in) __rte_trace_emit_datatype(in, int8_t) #define rte_trace_ctf_int(in) __rte_trace_emit_datatype(in, int32_t) #define rte_trace_ctf_long(in) __rte_trace_emit_datatype(in, long) #define rte_trace_ctf_float(in) __rte_trace_emit_datatype(in, float) #define rte_trace_ctf_double(in) __rte_trace_emit_datatype(in, double) #define rte_trace_ctf_ptr(in) __rte_trace_emit_datatype(in, uintptr_t) > > >
Hello Jerin, On Thu, Apr 16, 2020 at 6:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > From the prototype onwards, Myself shuffled abound multiple times on > the API name to satisfying > names. > > If you would like to classify based on the tracepoint object > dependency to a new header file, it is fine. > Let's go the last round for API naming details. > > I think, trace being the domain, IMO, it better to call the trace > point API with rte_trace_point_* > and trace point object to rte_trace_point_t (vs rte_tracepoint_t) Ok, let's go with rte_trace_point_. > I will summarise the public API and file name details. Let's finalize. > > # rte_trace.h will have > > rte_trace_global_is_enabled global_ does not make sense anymore. > rte_trace_mode_set > rte_trace_mode_get > rte_trace_pattern > rte_trace_regexp > rte_trace_save > rte_trace_metadata_dump > rte_trace_dump > > # rte_trace_point.h will have all operation related to rte_trace_point_t object > > # rte_trace_provider.h renamed rte_trace_point_provider.h > # rte_trace_register.h renamed to rte_trace_point_register.h > # rte_trace_eal.h renamed to rte_trace_point_eal.h Ok. > > > 2) rte_trace_fp_is_enabled() > > > > As a user, what information would this give me? > > "Some fastpath tracepoints are not available" > > > > Moving to rte_tracepoint.h is enough to me. > > IMO, semantically not correct as we are splitting based on some definition. What is rte_trace_fp_is_enabled() supposed to do? "Test if the trace datapath compile-time option is enabled." One implication is that dpdk must be compiled with RTE_ENABLE_TRACE_FP enabled for users to make use of fp tracepoints later. It would impose restrictions to final users when they did not compile the dpdk package themselves. > > How about, > 1) Not expose this API > OR > 2) rte_trace_point.h includes the rte_trace.h > > My vote is 1). On how to do without it, this should be enough in rte_trace_point_provider.h: #ifdef RTE_ENABLE_TRACE_FP #define __rte_trace_emit_header_fp(t) \ do { \ RTE_SET_USED(t); \ return; \ } while (0) #else #define __rte_trace_emit_header_fp(t) \ __rte_trace_emit_header(t) \ #endif This way, the user can choose where he enables fp tracepoints in his code by placing a #undef/#define RTE_ENABLE_TRACE_FP before including the tracepoints headers. > > > # Regarding the API change the following to rte_tracepoint_* > > > > > > #define rte_trace_ctf_u64(val) > > > #define rte_trace_ctf_i64(val) > > > #define rte_trace_ctf_u32(val) > > > #define rte_trace_ctf_i32(val) > > > #define rte_trace_ctf_u16(val) > > > #define rte_trace_ctf_i16(val) > > > #define rte_trace_ctf_u8(val) > > > #define rte_trace_ctf_i8(val) > > > #define rte_trace_ctf_int(val) > > > #define rte_trace_ctf_long(val) > > > #define rte_trace_ctf_float(val) > > > #define rte_trace_ctf_double(val) > > > #define rte_trace_ctf_ptr(val) > > > #define rte_trace_ctf_string(val) > > > It could be done. Just concerned the length of API will be more. like > > > rte_trace_point_ctf_u64 > > > If you have a strong opinion on this then I can change it. > > > > I don't like mentioning ctf here. > > > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > > If we keep one rte_tracepoint_emit_ per line in tracepoint > > declarations, the length is not an issue by looking at how they are > > used. > > OK to remove ctf to make it as rte_trace_point_emit_*. OK? Ok. > > > > > Example: > > RTE_TRACEPOINT( > > rte_trace_lib_eal_intr_disable, > > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > > rte_tracepoint_emit_int(rc); > > rte_tracepoint_emit_int(handle->vfio_dev_fd); > > rte_tracepoint_emit_int(handle->fd); > > rte_tracepoint_emit_int(handle->type); > > rte_tracepoint_emit_u32(handle->max_intr); > > rte_tracepoint_emit_u32(handle->nb_efd); > > ) > > > > Reading again what I wrote. > > Besides, we don't need to define all those > > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > > rte_tracepoint_provider.h and rte_tracepoint_register.h. This part still stands. > > If we define a helper rte_tracepoint_emit_data(type, in) in > > rte_tracepoint.h, then the "provider" and "register" headers must only > > define how to emit a header (generic and fp cases), then > > rte_tracepoint_emit_data and rte_tracepoint_emit_string. But this part is inconsistent, I will blame my son (EINTR/EAGAIN). I meant: we can have all per-type helpers in rte_trace_point.h. rte_trace_point_register.h and rte_trace_point_provider.h both define a macro (with a common signature) rte_trace_point_emit_data(type, in). This way, the rte_trace_point_emit_data implementation in rte_trace_point_register.h can do the type checking. rte_trace_point_provider.h macro just ignores the type argument. If you don't like it, let's drop this last part. Thanks.
On Fri, Apr 17, 2020 at 1:57 PM David Marchand <david.marchand@redhat.com> wrote: > > Hello Jerin, Hello David, > > On Thu, Apr 16, 2020 at 6:08 PM Jerin Jacob <jerinjacobk@gmail.com> wrote: > > From the prototype onwards, Myself shuffled abound multiple times on > > the API name to satisfying > > names. > > > > If you would like to classify based on the tracepoint object > > dependency to a new header file, it is fine. > > Let's go the last round for API naming details. > > > > I think, trace being the domain, IMO, it better to call the trace > > point API with rte_trace_point_* > > and trace point object to rte_trace_point_t (vs rte_tracepoint_t) > > Ok, let's go with rte_trace_point_. Ack. > > > > I will summarise the public API and file name details. Let's finalize. > > > > # rte_trace.h will have > > > > rte_trace_global_is_enabled > > global_ does not make sense anymore. Ack. I will change to rte_trace_is_enabled(). > > > > rte_trace_mode_set > > rte_trace_mode_get > > rte_trace_pattern > > rte_trace_regexp > > rte_trace_save > > rte_trace_metadata_dump > > rte_trace_dump > > > > # rte_trace_point.h will have all operation related to rte_trace_point_t object > > > > # rte_trace_provider.h renamed rte_trace_point_provider.h > > # rte_trace_register.h renamed to rte_trace_point_register.h > > # rte_trace_eal.h renamed to rte_trace_point_eal.h > > Ok. > > > > > > 2) rte_trace_fp_is_enabled() > > > > > > As a user, what information would this give me? > > > "Some fastpath tracepoints are not available" > > > > > > Moving to rte_tracepoint.h is enough to me. > > > > IMO, semantically not correct as we are splitting based on some definition. > > What is rte_trace_fp_is_enabled() supposed to do? > "Test if the trace datapath compile-time option is enabled." > > One implication is that dpdk must be compiled with RTE_ENABLE_TRACE_FP > enabled for users to make use of fp tracepoints later. > It would impose restrictions to final users when they did not compile > the dpdk package themselves. > > > > > > How about, > > 1) Not expose this API > > OR > > 2) rte_trace_point.h includes the rte_trace.h > > > > > > My vote is 1). OK. +1 > > On how to do without it, this should be enough in rte_trace_point_provider.h: > > #ifdef RTE_ENABLE_TRACE_FP > #define __rte_trace_emit_header_fp(t) \ > do { \ > RTE_SET_USED(t); \ > return; \ > } while (0) > #else > #define __rte_trace_emit_header_fp(t) \ > __rte_trace_emit_header(t) \ > #endif > > This way, the user can choose where he enables fp tracepoints in his > code by placing a #undef/#define RTE_ENABLE_TRACE_FP before including > the tracepoints headers. I will make the internal. I prefer to have if (0) c style scheme so that both sections of the code goes through the compilation phase. aka No compilation issue when RTE_ENABLE_TRACE_FP enabled. We have a separate macro declaration for RTE_TRACE_POINT and RTE_TRACE_POINT_FP for Tracepoint selection. > > > > > > # Regarding the API change the following to rte_tracepoint_* > > > > > > > > #define rte_trace_ctf_u64(val) > > > > #define rte_trace_ctf_i64(val) > > > > #define rte_trace_ctf_u32(val) > > > > #define rte_trace_ctf_i32(val) > > > > #define rte_trace_ctf_u16(val) > > > > #define rte_trace_ctf_i16(val) > > > > #define rte_trace_ctf_u8(val) > > > > #define rte_trace_ctf_i8(val) > > > > #define rte_trace_ctf_int(val) > > > > #define rte_trace_ctf_long(val) > > > > #define rte_trace_ctf_float(val) > > > > #define rte_trace_ctf_double(val) > > > > #define rte_trace_ctf_ptr(val) > > > > #define rte_trace_ctf_string(val) > > > > It could be done. Just concerned the length of API will be more. like > > > > rte_trace_point_ctf_u64 > > > > If you have a strong opinion on this then I can change it. > > > > > > I don't like mentioning ctf here. > > > > > > I went with a git grep -l rte_trace_ctf |xargs sed -i -e > > > 's/rte_trace_ctf_/rte_tracepoint_emit_/g'. > > > If we keep one rte_tracepoint_emit_ per line in tracepoint > > > declarations, the length is not an issue by looking at how they are > > > used. > > > > OK to remove ctf to make it as rte_trace_point_emit_*. OK? > > Ok. > > > > > > > > > > Example: > > > RTE_TRACEPOINT( > > > rte_trace_lib_eal_intr_disable, > > > RTE_TRACEPOINT_ARGS(const struct rte_intr_handle *handle, int rc), > > > rte_tracepoint_emit_int(rc); > > > rte_tracepoint_emit_int(handle->vfio_dev_fd); > > > rte_tracepoint_emit_int(handle->fd); > > > rte_tracepoint_emit_int(handle->type); > > > rte_tracepoint_emit_u32(handle->max_intr); > > > rte_tracepoint_emit_u32(handle->nb_efd); > > > ) > > > > > > > > Reading again what I wrote. > > > > Besides, we don't need to define all those > > > rte_tracepoint_emit_(u|i)(8|16|32|64) helpers in > > > rte_tracepoint_provider.h and rte_tracepoint_register.h. > > This part still stands. > > > > If we define a helper rte_tracepoint_emit_data(type, in) in > > > rte_tracepoint.h, then the "provider" and "register" headers must only > > > define how to emit a header (generic and fp cases), then > > > rte_tracepoint_emit_data and rte_tracepoint_emit_string. > > But this part is inconsistent, I will blame my son (EINTR/EAGAIN). > > I meant: we can have all per-type helpers in rte_trace_point.h. > > rte_trace_point_register.h and rte_trace_point_provider.h both define > a macro (with a common signature) rte_trace_point_emit_data(type, in). > > This way, the rte_trace_point_emit_data implementation in > rte_trace_point_register.h can do the type checking. > rte_trace_point_provider.h macro just ignores the type argument. Yes, if we have rte_trace_point.h as a separate file. I will work on this. Let me see if I can get it right. > > If you don't like it, let's drop this last part. > Thanks. Thanks for the review. > > > -- > David Marchand >