[v4,00/33] DPDK Trace support

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

Message ID

20200403153709.3703448-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: Fri, 3 Apr 2020 21:06:36 +0530
Message-ID: <20200403153709.3703448-1-jerinj@marvell.com>
In-Reply-To: <20200329144342.1543749-1-jerinj@marvell.com>
References: <20200329144342.1543749-1-jerinj@marvell.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Subject: [dpdk-dev]  [PATCH v4 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 3, 2020, 3:36 p.m. UTC

  From: Jerin Jacob <jerinj@marvell.com>

v4:
~~

This patch depends on http://patches.dpdk.org/patch/67758/

Depends-on:series-9191

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:
~~~~~~~~~
- APIs and Features are similar to rte_log dynamic framework
API(expect log prints on stdout vs it dumps on trace file)
- 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-level=8

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

        for (i = 0; i < 128; i++)
                rte_trace_lib_eal_generic_u8(i);
</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" }
[13:27:36.138469239] (+0.000000036) lib.eal.generic.u8: { cpu_id = 0,
name = "dpdk-test" }, { in = 0 }
[13:27:36.138469246] (+0.000000007) lib.eal.generic.u8: { cpu_id = 0,
name = "dpdk-test" }, { in = 1 }
[13:27:36.138469252] (+0.000000006) lib.eal.generic.u8: { cpu_id = 0,
name = "dpdk-test" }, { in = 2 }
[13:27:36.138469262] (+0.000000010) lib.eal.generic.u8: { cpu_id = 0,
name = "dpdk-test" }, { in = 3 }
[13:27:36.138469269] (+0.000000007) lib.eal.generic.u8: { cpu_id = 0,
name = "dpdk-test" }, { in = 4 }
[13:27:36.138469276] (+0.000000007) lib.eal.generic.u8: { cpu_id = 0,
name = "dpdk-test" }, { in = 5 }

# 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 level 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                         | 618 ++++++++++++++++++
 app/test/test_trace.h                         |  52 ++
 app/test/test_trace_perf.c                    | 179 +++++
 app/test/test_trace_register.c                |  46 ++
 config/common_base                            |   1 +
 config/meson.build                            |   9 +
 doc/api/doxy-api-index.md                     |   3 +-
 doc/guides/linux_gsg/eal_args.include.rst     |  55 ++
 doc/guides/prog_guide/build-sdk-meson.rst     |   5 +
 doc/guides/prog_guide/index.rst               |   1 +
 doc/guides/prog_guide/trace_lib.rst           | 339 ++++++++++
 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_log.c        |   9 +-
 lib/librte_eal/common/eal_common_memzone.c    |   9 +
 lib/librte_eal/common/eal_common_options.c    |  68 +-
 lib/librte_eal/common/eal_common_thread.c     |   3 +-
 lib/librte_eal/common/eal_common_trace.c      | 610 +++++++++++++++++
 lib/librte_eal/common/eal_common_trace_ctf.c  | 488 ++++++++++++++
 .../common/eal_common_trace_points.c          | 115 ++++
 .../common/eal_common_trace_utils.c           | 523 +++++++++++++++
 lib/librte_eal/common/eal_options.h           |   8 +
 lib/librte_eal/common/eal_private.h           |  11 +
 lib/librte_eal/common/eal_trace.h             | 122 ++++
 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            | 584 +++++++++++++++++
 lib/librte_eal/include/rte_trace_eal.h        | 247 +++++++
 lib/librte_eal/include/rte_trace_provider.h   | 160 +++++
 lib/librte_eal/include/rte_trace_register.h   |  53 ++
 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            |  59 ++
 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 +
 162 files changed, 6266 insertions(+), 110 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

Thomas Monjalon April 9, 2020, 2 p.m. UTC | #1

Hi,

03/04/2020 17:36, jerinj@marvell.com:
> Features:
> ~~~~~~~~~
> - APIs and Features are similar to rte_log dynamic framework
> API(expect log prints on stdout vs it dumps on trace file)

A log can print to syslog as well.

As discussed somewhere else, please do not introduce global level
in rte_trace. I think it is useless. If we need to change the level
of all trace types, we can just use a wildcard (globbing or regexp).

And about wording, "pattern" is too vague and should be replaced
with "globbing".


> - No specific limit on the events. A string-based event like rte_log
> for pattern matching

Would it be possible to replace rte_log with rte_trace?


> - Dynamic enable/disable support.

How dynamic it is? Can we start/stop a trace after starting DPDK?
I think we need a control channel for this.
I propose to introduce a general control channel in DPDK.


> - Instructmention overhead is ~1 cycle. i.e cost of adding the code

Nice


> wth out using trace feature.
> - Timestamp support for all the events using DPDK rte_rtdsc

Could we use other timestamp sources, like the mbuf timestamp
for Rx traces?


> - No dependency on another library. Clean room native implementation of
>   CTF.

No benefit of using an external CTF library maintained somewhere else?

Jerin Jacob April 9, 2020, 3:36 p.m. UTC | #2

On Thu, Apr 9, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> Hi,
>
> 03/04/2020 17:36, jerinj@marvell.com:
> > Features:
> > ~~~~~~~~~
> > - APIs and Features are similar to rte_log dynamic framework
> > API(expect log prints on stdout vs it dumps on trace file)
>
> A log can print to syslog as well.
>
> As discussed somewhere else, please do not introduce global level
> in rte_trace. I think it is useless. If we need to change the level
> of all trace types, we can just use a wildcard (globbing or regexp).

Currently, In the log library, when EAL command-line argument
specifies the "--log-level=val" it, will call
the  rte_log_global_level_set(val)

Is the suggestion to make rte_log_global_level_set() as an internal
EAL API or remove that feature?
If we remove, then I dont know, how we can map --log-level or
--trace-level EAL command-line argument.

>
> And about wording, "pattern" is too vague and should be replaced
> with "globbing".

Currently, we are using the word "shell pattern" in log and trace.
According to man 3 fnmatch, both being used.
I can send a patch for changing the "shell pattern" to  "glob" in
rte_log.h and update the trace if needed.
Let me know?. IMO, Both has to use same word.

>
>
> > - No specific limit on the events. A string-based event like rte_log
> > for pattern matching
>
> Would it be possible to replace rte_log with rte_trace?

Yes, I kept public API similar to log to have that use case in mind.

>
>
> > - Dynamic enable/disable support.
>
> How dynamic it is? Can we start/stop a trace after starting DPDK?

Yes, we have fine control enabling and disabling a specific or group of events.
See rte_trace_enable(), rte_trace_disable() and rte_trace_pattern()

> I think we need a control channel for this.
> I propose to introduce a general control channel in DPDK.

Yes. Once we have a general control channel in DPDK. We can hook
rte_trace_enable() and friends
in the message handler.

>
>
> > - Instructmention overhead is ~1 cycle. i.e cost of adding the code
>
> Nice
>
>
> > wth out using trace feature.
> > - Timestamp support for all the events using DPDK rte_rtdsc
>
> Could we use other timestamp sources, like the mbuf timestamp
> for Rx traces?

Trace will be running at the global wall clock. Rx specific tracepoint
can add mbuf->timestamp as FIELD in the future if needed.

>
>
> > - No dependency on another library. Clean room native implementation of
> >   CTF.
>
> No benefit of using an external CTF library maintained somewhere else?

See performance data comparison with LTTng in the cover letter.

We discussed this enough in the mailing list on RFC and decide to use
Faster trace support for Fastpath use case and avoid any external library to EAL
(We need tracepoints in the EAL too for tracing).

>
>
>

Thomas Monjalon April 9, 2020, 4 p.m. UTC | #3

09/04/2020 17:36, Jerin Jacob:
> On Thu, Apr 9, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 03/04/2020 17:36, jerinj@marvell.com:
> > > Features:
> > > ~~~~~~~~~
> > > - APIs and Features are similar to rte_log dynamic framework
> > > API(expect log prints on stdout vs it dumps on trace file)
> >
> > A log can print to syslog as well.
> >
> > As discussed somewhere else, please do not introduce global level
> > in rte_trace. I think it is useless. If we need to change the level
> > of all trace types, we can just use a wildcard (globbing or regexp).
> 
> 
> Currently, In the log library, when EAL command-line argument
> specifies the "--log-level=val" it, will call
> the  rte_log_global_level_set(val)
> 
> Is the suggestion to make rte_log_global_level_set() as an internal
> EAL API or remove that feature?

Completely remove global level.
Why would we need 2 levels of level?

> If we remove, then I dont know, how we can map --log-level or
> --trace-level EAL command-line argument.

They are setting levels with regex or globbing.
--log-level supports 3 syntaxes today:
	- int (global level)
	- globbing:int
	- regex,int
I propose to drop the first syntax because it became useless
now that we have powerful globbing and regex.


> > And about wording, "pattern" is too vague and should be replaced
> > with "globbing".
> 
> Currently, we are using the word "shell pattern" in log and trace.
> According to man 3 fnmatch, both being used.
> I can send a patch for changing the "shell pattern" to  "glob" in
> rte_log.h and update the trace if needed.
> Let me know?. IMO, Both has to use same word.

Yes I think globbing is more accurate than "shell pattern",
because we can use regex pattern in shell as well.


> > > - No specific limit on the events. A string-based event like rte_log
> > > for pattern matching
> >
> > Would it be possible to replace rte_log with rte_trace?
> 
> Yes, I kept public API similar to log to have that use case in mind.

OK I think it would be good to have only one system.


> > > - Dynamic enable/disable support.
> >
> > How dynamic it is? Can we start/stop a trace after starting DPDK?
> 
> Yes, we have fine control enabling and disabling a specific or group of events.
> See rte_trace_enable(), rte_trace_disable() and rte_trace_pattern()
> 
> > I think we need a control channel for this.
> > I propose to introduce a general control channel in DPDK.
> 
> Yes. Once we have a general control channel in DPDK. We can hook
> rte_trace_enable() and friends
> in the message handler.

That would be a nice feature.


> > > - Instructmention overhead is ~1 cycle. i.e cost of adding the code
> >
> > Nice
> >
> >
> > > wth out using trace feature.
> > > - Timestamp support for all the events using DPDK rte_rtdsc
> >
> > Could we use other timestamp sources, like the mbuf timestamp
> > for Rx traces?
> 
> Trace will be running at the global wall clock. Rx specific tracepoint
> can add mbuf->timestamp as FIELD in the future if needed.

OK makes sense.

Note: I prefer rte_get_timer_cycles() because rdtsc is an x86 instruction.


> > > - No dependency on another library. Clean room native implementation of
> > >   CTF.
> >
> > No benefit of using an external CTF library maintained somewhere else?
> 
> See performance data comparison with LTTng in the cover letter.
> 
> We discussed this enough in the mailing list on RFC and decide to use
> Faster trace support for Fastpath use case and avoid any external library to EAL
> (We need tracepoints in the EAL too for tracing).

I can understand the performance reason.
But if another library can provide the same performance,
I don't see any reason not depending on it.
If the dependency is not found, the feature can be disabled.

Jerin Jacob April 9, 2020, 4:34 p.m. UTC | #4

On Thu, Apr 9, 2020 at 9:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 09/04/2020 17:36, Jerin Jacob:
> > On Thu, Apr 9, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > 03/04/2020 17:36, jerinj@marvell.com:
> > > > Features:
> > > > ~~~~~~~~~
> > > > - APIs and Features are similar to rte_log dynamic framework
> > > > API(expect log prints on stdout vs it dumps on trace file)
> > >
> > > A log can print to syslog as well.
> > >
> > > As discussed somewhere else, please do not introduce global level
> > > in rte_trace. I think it is useless. If we need to change the level
> > > of all trace types, we can just use a wildcard (globbing or regexp).
> >
> >
> > Currently, In the log library, when EAL command-line argument
> > specifies the "--log-level=val" it, will call
> > the  rte_log_global_level_set(val)
> >
> > Is the suggestion to make rte_log_global_level_set() as an internal
> > EAL API or remove that feature?
>
> Completely remove global level.
> Why would we need 2 levels of level?
>
> > If we remove, then I dont know, how we can map --log-level or
> > --trace-level EAL command-line argument.
>
> They are setting levels with regex or globbing.
> --log-level supports 3 syntaxes today:
>         - int (global level)
>         - globbing:int
>         - regex,int

Here is my understanding.

IMO, Actual Syntax is
         - int (global level)
         - globbing: int (global level)
         - regex: int (global level)

i.e

1) Each log/trace has a  "level"
2) There is a global level. See [1]
3) The trace will be emitted when
a) When it is enabled(not applicable for log as it is always enabled)
AND
b) it is less than the global level.

The global level will be useful to control what category of trace
"level" needs to enable.
I think, it is useful to trace only DEBUG or CRITICAL events, Any
reason for thinking, it is not.


[1]
/**
 * Set the global log level.
 *
 * After this call, logs with a level lower or equal than the level
 * passed as argument will be displayed.
 *
 * @param level
 *   Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
 */
void rte_log_set_global_level(uint32_t level);


> I propose to drop the first syntax because it became useless
> now that we have powerful globbing and regex.
>
>
> > > And about wording, "pattern" is too vague and should be replaced
> > > with "globbing".
> >
> > Currently, we are using the word "shell pattern" in log and trace.
> > According to man 3 fnmatch, both being used.
> > I can send a patch for changing the "shell pattern" to  "glob" in
> > rte_log.h and update the trace if needed.
> > Let me know?. IMO, Both has to use same word.
>
> Yes I think globbing is more accurate than "shell pattern",
> because we can use regex pattern in shell as well.

I will change to "gloabbing" in trace and submit a patch for the same
for rte_log as well.

>
>
> > > > - No specific limit on the events. A string-based event like rte_log
> > > > for pattern matching
> > >
> > > Would it be possible to replace rte_log with rte_trace?
> >
> > Yes, I kept public API similar to log to have that use case in mind.
>
> OK I think it would be good to have only one system.

Yes. We can switch over to rte_trace when it is decided.

>
>
> > > > - Dynamic enable/disable support.
> > >
> > > How dynamic it is? Can we start/stop a trace after starting DPDK?
> >
> > Yes, we have fine control enabling and disabling a specific or group of events.
> > See rte_trace_enable(), rte_trace_disable() and rte_trace_pattern()
> >
> > > I think we need a control channel for this.
> > > I propose to introduce a general control channel in DPDK.
> >
> > Yes. Once we have a general control channel in DPDK. We can hook
> > rte_trace_enable() and friends
> > in the message handler.
>
> That would be a nice feature.
>
>
> > > > - Instructmention overhead is ~1 cycle. i.e cost of adding the code
> > >
> > > Nice
> > >
> > >
> > > > wth out using trace feature.
> > > > - Timestamp support for all the events using DPDK rte_rtdsc
> > >
> > > Could we use other timestamp sources, like the mbuf timestamp
> > > for Rx traces?
> >
> > Trace will be running at the global wall clock. Rx specific tracepoint
> > can add mbuf->timestamp as FIELD in the future if needed.
>
> OK makes sense.
>
> Note: I prefer rte_get_timer_cycles() because rdtsc is an x86 instruction.

Code is using rte_get_tsc_cycles() public API as wall clock is based
on tsc clock(
Which is available in the system in all the cases)

>
>
> > > > - No dependency on another library. Clean room native implementation of
> > > >   CTF.
> > >
> > > No benefit of using an external CTF library maintained somewhere else?
> >
> > See performance data comparison with LTTng in the cover letter.
> >
> > We discussed this enough in the mailing list on RFC and decide to use
> > Faster trace support for Fastpath use case and avoid any external library to EAL
> > (We need tracepoints in the EAL too for tracing).
>
> I can understand the performance reason.

OK

> But if another library can provide the same performance,
> I don't see any reason not depending on it.
> If the dependency is not found, the feature can be disabled.

Yes. But, if we replace rte_log with trace, it can not be just an
optional component and
it can run wherever EAL can run(i.e Windows OS). Trace format will be
the same across all
the EAL environments a.k.a common post trace analysis tools.



>
>

Thomas Monjalon April 9, 2020, 5:11 p.m. UTC | #5

09/04/2020 18:34, Jerin Jacob:
> On Thu, Apr 9, 2020 at 9:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > 09/04/2020 17:36, Jerin Jacob:
> > > On Thu, Apr 9, 2020 at 7:30 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > > As discussed somewhere else, please do not introduce global level
> > > > in rte_trace. I think it is useless. If we need to change the level
> > > > of all trace types, we can just use a wildcard (globbing or regexp).
> > >
> > > Currently, In the log library, when EAL command-line argument
> > > specifies the "--log-level=val" it, will call
> > > the  rte_log_global_level_set(val)
> > >
> > > Is the suggestion to make rte_log_global_level_set() as an internal
> > > EAL API or remove that feature?
> >
> > Completely remove global level.
> > Why would we need 2 levels of level?
> >
> > > If we remove, then I dont know, how we can map --log-level or
> > > --trace-level EAL command-line argument.
> >
> > They are setting levels with regex or globbing.
> > --log-level supports 3 syntaxes today:
> >         - int (global level)
> >         - globbing:int
> >         - regex,int
> 
> Here is my understanding.
> 
> IMO, Actual Syntax is
>          - int (global level)
>          - globbing: int (global level)
>          - regex: int (global level)

The level apply to the logs matching the pattern (globbing or regex)
so I don't understand why you call it "global".

> i.e
> 
> 1) Each log/trace has a  "level"
> 2) There is a global level. See [1]
> 3) The trace will be emitted when
> a) When it is enabled(not applicable for log as it is always enabled)
> AND
> b) it is less than the global level.

When it is less than both global and logtype level.
See rte_log_can_log().

> The global level will be useful to control what category of trace
> "level" needs to enable.
> I think, it is useful to trace only DEBUG or CRITICAL events, Any
> reason for thinking, it is not.
> 
> 
> [1]
> /**
>  * Set the global log level.
>  *
>  * After this call, logs with a level lower or equal than the level
>  * passed as argument will be displayed.
>  *
>  * @param level
>  *   Log level. A value between RTE_LOG_EMERG (1) and RTE_LOG_DEBUG (8).
>  */
> void rte_log_set_global_level(uint32_t level);

rte_log_can_log(uint32_t logtype, uint32_t level)
{
    int log_level;

    if (level > rte_log_get_global_level())
        return false;

    log_level = rte_log_get_level(logtype);
    if (log_level < 0)
        return false;

    if (level > (uint32_t)log_level)
        return false;

    return true;
}

The global level is just disabling some logs even if it is enabled
in the logtype level.
It only makes usage complicate.
We should consider only logtype levels.

Currently, the global log level is disabled by default by setting
the global level to DEBUG:

RTE_INIT_PRIO(rte_log_init, LOG)
{
[...]
    rte_log_set_global_level(RTE_LOG_DEBUG);

It was done by Pavan 2 years ago.

Jerin Jacob April 9, 2020, 6:27 p.m. UTC | #6

> > >
> > > They are setting levels with regex or globbing.
> > > --log-level supports 3 syntaxes today:
> > >         - int (global level)
> > >         - globbing:int
> > >         - regex,int
> >
> > Here is my understanding.
> >
> > IMO, Actual Syntax is
> >          - int (global level)
> >          - globbing: int (global level)
> >          - regex: int (global level)
>
> The level apply to the logs matching the pattern (globbing or regex)
> so I don't understand why you call it "global".

What I meant is, if there is no global, what is the point of changing
the trace level with EAL command-line argument with globbing and regex.

>
> > i.e
> >
> > 1) Each log/trace has a  "level"
> > 2) There is a global level. See [1]
> > 3) The trace will be emitted when
> > a) When it is enabled(not applicable for log as it is always enabled)
> > AND
> > b) it is less than the global level.
>
> When it is less than both global and logtype level.
> See rte_log_can_log().

OK.

>
> The global level is just disabling some logs even if it is enabled
> in the logtype level.
> It only makes usage complicate.
> We should consider only logtype levels.

OK.  Do we care about the following use case?
# Trace only specific types of events(example, DEBUG or CRITICAL)?

IF NOT,

There is no need for,
# rte_trace_global_* API
# No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove
rte_trace_level_set() API etc


# In summary:
~~~~~~~~~~~~~
# In the existing code:
The trace will be emitted when
 a) When the trace is enabled
 AND
b) trace level than the global level.

# in new suggestion
The trace will be emitted when
 a) When it is enabled

And the EAL argument will be --trace=regex/globbing, This EAL argument
will call rte_trace_regexp() and enable the selected tracepoints.

The downside will be it will not be similar to log API. I am fine with
either way.

@Mattias Rönnblom, Do you have any thoughts as you spend some time
reviewing the public API.

Thoughts in general?

Jerin Jacob April 9, 2020, 6:52 p.m. UTC | #7

On Thu, Apr 9, 2020 at 11:57 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> > > >
> > > > They are setting levels with regex or globbing.
> > > > --log-level supports 3 syntaxes today:
> > > >         - int (global level)
> > > >         - globbing:int
> > > >         - regex,int
> > >
> > > Here is my understanding.
> > >
> > > IMO, Actual Syntax is
> > >          - int (global level)
> > >          - globbing: int (global level)
> > >          - regex: int (global level)
> >
> > The level apply to the logs matching the pattern (globbing or regex)
> > so I don't understand why you call it "global".
>
> What I meant is, if there is no global, what is the point of changing
> the trace level with EAL command-line argument with globbing and regex.

Self reply on this:

I understood where is the disconnect is:

In case of log:
~~~~~~~~~~~
It will be printed when:
a) When it is less than both the "global" and "log level"

NOTE: in case of log, log level passed on to log function (see
rte_log(uint32_t level..)

In case of trace:
~~~~~~~~~~~~
it will be emitted when:
a) The trace is enabled (rte_trace_enable() called on the tracepoint)
b) When it is less than the global level.

NOTE: in case of trace, Nothing like log level passed to trace emit
function. The control lies with a slow path enable/disable
decision(item (a)) due to performance requirements
to instrumentation overhead.

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

On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > The global level is just disabling some logs even if it is enabled
> > in the logtype level.
> > It only makes usage complicate.
> > We should consider only logtype levels.
>
> OK.  Do we care about the following use case?
> # Trace only specific types of events(example, DEBUG or CRITICAL)?

The event types can be encoded in the tracepoint names if we want to
organise trace points with types (maybe as a suffix).

>
> IF NOT,
>
> There is no need for,
> # rte_trace_global_* API
> # No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove
> rte_trace_level_set() API etc
>
>
> # In summary:
> ~~~~~~~~~~~~~
> # In the existing code:
> The trace will be emitted when
>  a) When the trace is enabled
>  AND
> b) trace level than the global level.
>
> # in new suggestion
> The trace will be emitted when
>  a) When it is enabled
>
> And the EAL argument will be --trace=regex/globbing, This EAL argument
> will call rte_trace_regexp() and enable the selected tracepoints.
>
> The downside will be it will not be similar to log API. I am fine with
> either way.

The level notion in the traces api is artificial.
I prefer a simple api where tracepoints state is controlled via a
single criteria: with the new suggestion that would be names.
So +1.


Afaiu, for logs, if we wanted to make use of the trace api, the level
notion can be done in the rte_log layer.

Jerin Jacob April 10, 2020, 1:29 p.m. UTC | #9

On Fri, Apr 10, 2020 at 6:45 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > The global level is just disabling some logs even if it is enabled
> > > in the logtype level.
> > > It only makes usage complicate.
> > > We should consider only logtype levels.
> >
> > OK.  Do we care about the following use case?
> > # Trace only specific types of events(example, DEBUG or CRITICAL)?
>
> The event types can be encoded in the tracepoint names if we want to
> organise trace points with types (maybe as a suffix).
>
> >
> > IF NOT,
> >
> > There is no need for,
> > # rte_trace_global_* API
> > # No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove
> > rte_trace_level_set() API etc
> >
> >
> > # In summary:
> > ~~~~~~~~~~~~~
> > # In the existing code:
> > The trace will be emitted when
> >  a) When the trace is enabled
> >  AND
> > b) trace level than the global level.
> >
> > # in new suggestion
> > The trace will be emitted when
> >  a) When it is enabled
> >
> > And the EAL argument will be --trace=regex/globbing, This EAL argument
> > will call rte_trace_regexp() and enable the selected tracepoints.
> >
> > The downside will be it will not be similar to log API. I am fine with
> > either way.
>
> The level notion in the traces api is artificial.
> I prefer a simple api where tracepoints state is controlled via a
> single criteria: with the new suggestion that would be names.
> So +1.

I thought it may be helpful to replace log and I decided to pick the
level in the trace.

Looks like the consensus is to remove "level" from Trace.
OK. I will rework the Trace API to remove the "level" and
rte_trace_global_* API from Trace library.
Let me know If you have any other top-level architecture comments if any.
I will send v5 with new changes.

>
>
> Afaiu, for logs, if we wanted to make use of the trace api, the level
> notion can be done in the rte_log layer.

That works too.

>
>
> --
> David Marchand
>

David Marchand April 10, 2020, 1:45 p.m. UTC | #10

On Fri, Apr 10, 2020 at 3:30 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> On Fri, Apr 10, 2020 at 6:45 PM David Marchand
> <david.marchand@redhat.com> wrote:
> >
> > On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > The global level is just disabling some logs even if it is enabled
> > > > in the logtype level.
> > > > It only makes usage complicate.
> > > > We should consider only logtype levels.
> > >
> > > OK.  Do we care about the following use case?
> > > # Trace only specific types of events(example, DEBUG or CRITICAL)?
> >
> > The event types can be encoded in the tracepoint names if we want to
> > organise trace points with types (maybe as a suffix).
> >
> > >
> > > IF NOT,
> > >
> > > There is no need for,
> > > # rte_trace_global_* API
> > > # No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove
> > > rte_trace_level_set() API etc
> > >
> > >
> > > # In summary:
> > > ~~~~~~~~~~~~~
> > > # In the existing code:
> > > The trace will be emitted when
> > >  a) When the trace is enabled
> > >  AND
> > > b) trace level than the global level.
> > >
> > > # in new suggestion
> > > The trace will be emitted when
> > >  a) When it is enabled
> > >
> > > And the EAL argument will be --trace=regex/globbing, This EAL argument
> > > will call rte_trace_regexp() and enable the selected tracepoints.
> > >
> > > The downside will be it will not be similar to log API. I am fine with
> > > either way.
> >
> > The level notion in the traces api is artificial.
> > I prefer a simple api where tracepoints state is controlled via a
> > single criteria: with the new suggestion that would be names.
> > So +1.
>
> I thought it may be helpful to replace log and I decided to pick the
> level in the trace.
>
> Looks like the consensus is to remove "level" from Trace.
> OK. I will rework the Trace API to remove the "level" and
> rte_trace_global_* API from Trace library.
> Let me know If you have any other top-level architecture comments if any.
> I will send v5 with new changes.

Thanks Jerin.


- I am still looking at the event record mode.
I just wonder why we have this notion per tracepoint.
The documentation talks about it being an attribute of the trace
buffers, and the described behavior looks fine.
But if we can set this mode per tracepoints, once a trace buffer is
full, won't it be hard to figure out why some events have been caught
and not others?


- Another comment, on the form, I can see that we talk about dataplane
tracepoints and fastpath API.
Is there a difference? or could everything be named in a consistent
way (the headers would have to be aligned too)?


- Do we really need to export the rte_trace_lib_eal_generic_* trace points?
They seem more like examples and I don't see a usage outside of the tests.

Jerin Jacob April 10, 2020, 2:37 p.m. UTC | #11

On Fri, Apr 10, 2020 at 7:15 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Fri, Apr 10, 2020 at 3:30 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> >
> > On Fri, Apr 10, 2020 at 6:45 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > >
> > > On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > > The global level is just disabling some logs even if it is enabled
> > > > > in the logtype level.
> > > > > It only makes usage complicate.
> > > > > We should consider only logtype levels.
> > > >
> > > > OK.  Do we care about the following use case?
> > > > # Trace only specific types of events(example, DEBUG or CRITICAL)?
> > >
> > > The event types can be encoded in the tracepoint names if we want to
> > > organise trace points with types (maybe as a suffix).
> > >
> > > >
> > > > IF NOT,
> > > >
> > > > There is no need for,
> > > > # rte_trace_global_* API
> > > > # No need to introduce trace type(ie DEBUG or CRITICAL) i.e remove
> > > > rte_trace_level_set() API etc
> > > >
> > > >
> > > > # In summary:
> > > > ~~~~~~~~~~~~~
> > > > # In the existing code:
> > > > The trace will be emitted when
> > > >  a) When the trace is enabled
> > > >  AND
> > > > b) trace level than the global level.
> > > >
> > > > # in new suggestion
> > > > The trace will be emitted when
> > > >  a) When it is enabled
> > > >
> > > > And the EAL argument will be --trace=regex/globbing, This EAL argument
> > > > will call rte_trace_regexp() and enable the selected tracepoints.
> > > >
> > > > The downside will be it will not be similar to log API. I am fine with
> > > > either way.
> > >
> > > The level notion in the traces api is artificial.
> > > I prefer a simple api where tracepoints state is controlled via a
> > > single criteria: with the new suggestion that would be names.
> > > So +1.
> >
> > I thought it may be helpful to replace log and I decided to pick the
> > level in the trace.
> >
> > Looks like the consensus is to remove "level" from Trace.
> > OK. I will rework the Trace API to remove the "level" and
> > rte_trace_global_* API from Trace library.
> > Let me know If you have any other top-level architecture comments if any.
> > I will send v5 with new changes.
>
> Thanks Jerin.

Thanks, David for the comments.

>
>
> - I am still looking at the event record mode.
> I just wonder why we have this notion per tracepoint.
> The documentation talks about it being an attribute of the trace
> buffers, and the described behavior looks fine.
> But if we can set this mode per tracepoints, once a trace buffer is
> full, won't it be hard to figure out why some events have been caught
> and not others?

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

Let me know.

>
>
> - Another comment, on the form, I can see that we talk about dataplane
> tracepoints and fastpath API.
> Is there a difference? or could everything be named in a consistent

No. Both are the same.

> way (the headers would have to be aligned too)?

Personally I like the fast path. The log calls it as DP( see RTE_LOG_DP).
In order to have consistency, I will pick the DP and change it everywhere to DP,
and header files to xxxxx_dp.h from xxxx_fp.h.

If you have a preference then I will change to the same. Let me know.


>
>
> - Do we really need to export the rte_trace_lib_eal_generic_* trace points?
> They seem more like examples and I don't see a usage outside of the tests.

This generic tracepoint for any quick debug, For example, if some need to add
tracepoint to just emit string in the middle of debugging a problem,
they can use rte_trace_lib_eal_generic_str.
That's the reason for exporting it.


>
>
> --
> David Marchand
>

David Marchand April 10, 2020, 3 p.m. UTC | #12

On Fri, Apr 10, 2020 at 4:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > - I am still looking at the event record mode.
> > I just wonder why we have this notion per tracepoint.
> > The documentation talks about it being an attribute of the trace
> > buffers, and the described behavior looks fine.
> > But if we can set this mode per tracepoints, once a trace buffer is
> > full, won't it be hard to figure out why some events have been caught
> > and not others?
>
> 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.


> > - Another comment, on the form, I can see that we talk about dataplane
> > tracepoints and fastpath API.
> > Is there a difference? or could everything be named in a consistent
>
> No. Both are the same.
>
> > way (the headers would have to be aligned too)?
>
> Personally I like the fast path. The log calls it as DP( see RTE_LOG_DP).
> In order to have consistency, I will pick the DP and change it everywhere to DP,
> and header files to xxxxx_dp.h from xxxx_fp.h.

I too have a preference for "fast path".
Thomas ?


> > - Do we really need to export the rte_trace_lib_eal_generic_* trace points?
> > They seem more like examples and I don't see a usage outside of the tests.
>
> This generic tracepoint for any quick debug, For example, if some need to add
> tracepoint to just emit string in the middle of debugging a problem,
> they can use rte_trace_lib_eal_generic_str.
> That's the reason for exporting it.

Ok, let's leave it as is, I need to think about it.

Thomas Monjalon April 10, 2020, 3:11 p.m. UTC | #13

10/04/2020 17:00, David Marchand:
> On Fri, Apr 10, 2020 at 4:38 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > - Another comment, on the form, I can see that we talk about dataplane
> > > tracepoints and fastpath API.
> > > Is there a difference? or could everything be named in a consistent
> >
> > No. Both are the same.
> >
> > > way (the headers would have to be aligned too)?
> >
> > Personally I like the fast path. The log calls it as DP( see RTE_LOG_DP).
> > In order to have consistency, I will pick the DP and change it everywhere to DP,
> > and header files to xxxxx_dp.h from xxxx_fp.h.
> 
> I too have a preference for "fast path".
> Thomas ?

OK for fp.

Thomas Monjalon April 10, 2020, 3:12 p.m. UTC | #14

10/04/2020 15:29, Jerin Jacob:
> On Fri, Apr 10, 2020 at 6:45 PM David Marchand
> <david.marchand@redhat.com> wrote:
> > On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > # In summary:
> > > ~~~~~~~~~~~~~
> > > # In the existing code:
> > > The trace will be emitted when
> > >  a) When the trace is enabled
> > >  AND
> > > b) trace level than the global level.
> > >
> > > # in new suggestion
> > > The trace will be emitted when
> > >  a) When it is enabled
> > >
> > > And the EAL argument will be --trace=regex/globbing, This EAL argument
> > > will call rte_trace_regexp() and enable the selected tracepoints.
> > >
> > > The downside will be it will not be similar to log API. I am fine with
> > > either way.
> >
> > The level notion in the traces api is artificial.
> > I prefer a simple api where tracepoints state is controlled via a
> > single criteria: with the new suggestion that would be names.
> > So +1.
> 
> I thought it may be helpful to replace log and I decided to pick the
> level in the trace.
> 
> Looks like the consensus is to remove "level" from Trace.

I miss the plan to rework logs on top of trace.
How could it be done?

Jerin Jacob April 10, 2020, 5:56 p.m. UTC | #15

On Fri, Apr 10, 2020 at 8:42 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 10/04/2020 15:29, Jerin Jacob:
> > On Fri, Apr 10, 2020 at 6:45 PM David Marchand
> > <david.marchand@redhat.com> wrote:
> > > On Thu, Apr 9, 2020 at 8:27 PM Jerin Jacob <jerinjacobk@gmail.com> wrote:
> > > > # In summary:
> > > > ~~~~~~~~~~~~~
> > > > # In the existing code:
> > > > The trace will be emitted when
> > > >  a) When the trace is enabled
> > > >  AND
> > > > b) trace level than the global level.
> > > >
> > > > # in new suggestion
> > > > The trace will be emitted when
> > > >  a) When it is enabled
> > > >
> > > > And the EAL argument will be --trace=regex/globbing, This EAL argument
> > > > will call rte_trace_regexp() and enable the selected tracepoints.
> > > >
> > > > The downside will be it will not be similar to log API. I am fine with
> > > > either way.
> > >
> > > The level notion in the traces api is artificial.
> > > I prefer a simple api where tracepoints state is controlled via a
> > > single criteria: with the new suggestion that would be names.
> > > So +1.
> >
> > I thought it may be helpful to replace log and I decided to pick the
> > level in the trace.
> >
> > Looks like the consensus is to remove "level" from Trace.
>
> I miss the plan to rework logs on top of trace.
> How could it be done?

# I think, David suggestion is to add "level" as a string in the trace
name. Say "debug.lib.eal.*"

# I am thinking, If we don't like above,
a) Add following EAL private APIs to get and set "level" as opaque
object when _needed_   from trace.
1) int eal_trace_opaque_set(rte_trace_t trace, uint8_t opaque);
2) uint8_t eal_trace_opaque_get(rte_trace_t trace);

The above private API would be enough to implement.


>
>