[v5,00/33] DPDK Trace support

Message ID	20200413150116.734047-1-jerinj@marvell.com (mailing list archive)
Headers	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> 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 Subject: [dpdk-dev] [PATCH v5 00/33] DPDK Trace support Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>
Series	DPDK Trace support \| [v5,00/33] DPDK Trace support [v5,01/33] meson: add libatomic as a global dependency for i686 clang [v5,02/33] eal: introduce API for getting thread name [v5,03/33] eal/trace: define the public API for trace support [v5,04/33] eal/trace: implement trace register API [v5,05/33] eal/trace: implement trace operation APIs [v5,06/33] eal/trace: add internal trace init and fini interface [v5,07/33] eal/trace: get bootup timestamp for trace [v5,08/33] eal/trace: create CTF TDSL metadata in memory [v5,09/33] eal/trace: handle CTF keyword collision [v5,10/33] eal/trace: implement trace memory allocation [v5,11/33] eal/trace: implement debug dump function [v5,12/33] eal/trace: implement trace save [v5,13/33] eal/trace: implement registration payload [v5,14/33] eal/trace: implement provider payload [v5,15/33] eal/trace: hook internal trace APIs to Linux [v5,16/33] eal/trace: hook internal trace APIs to FreeBSD [v5,17/33] eal/trace: add generic tracepoints [v5,18/33] eal/trace: add alarm tracepoints [v5,19/33] eal/trace: add memory tracepoints [v5,20/33] eal/trace: add memzone tracepoints [v5,21/33] eal/trace: add thread tracepoints [v5,22/33] eal/trace: add interrupt tracepoints [v5,23/33] eal/trace: add trace configuration parameter [v5,24/33] eal/trace: add trace dir configuration parameter [v5,25/33] eal/trace: add trace bufsize configuration parameter [v5,26/33] eal/trace: add trace mode configuration parameter [v5,27/33] eal/trace: add unit test cases [v5,28/33] eal/trace: add trace performance test cases [v5,29/33] ethdev: add tracepoints [v5,30/33] eventdev: add tracepoints [v5,31/33] cryptodev: add tracepoints [v5,32/33] mempool: add tracepoints [v5,33/33] doc: add trace library guide

Message ID

20200413150116.734047-1-jerinj@marvell.com (mailing list archive)

Headers

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>
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
Subject: [dpdk-dev]  [PATCH v5 00/33] DPDK Trace support
Precedence: list
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

David Marchand April 15, 2020, 1:26 p.m. UTC | #1

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 ?

Thomas Monjalon April 15, 2020, 1:43 p.m. UTC | #2

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 ?

Jerin Jacob April 15, 2020, 2:39 p.m. UTC | #3

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
>

David Marchand April 16, 2020, 1:39 p.m. UTC | #4

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.

Jerin Jacob April 16, 2020, 4:08 p.m. UTC | #5

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.


>

Thomas Monjalon April 16, 2020, 4:23 p.m. UTC | #6

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?

Jerin Jacob April 16, 2020, 4:26 p.m. UTC | #7

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)

>
>
>

David Marchand April 17, 2020, 8:27 a.m. UTC | #8

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.

Jerin Jacob April 17, 2020, 8:57 a.m. UTC | #9

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
>