Re: [PATCH v6 2/2] Add l2reflect measurement application

[Jerin Jacob <jerinjacobk@gmail.com>]

On Fri, Sep 2, 2022 at 2:16 PM Felix Moessbauer
<felix.moessbauer@siemens.com> wrote:
>
> The l2reflect application implements a ping-pong benchmark to
> measure the latency between two instances. For communication,
> we use raw ethernet and send one packet at a time. The timing data
> is collected locally and min/max/avg values are displayed in a TUI.
> Finally, a histogram of the latencies is printed which can be

One highlevel comment,

IMO, We don't need to add code for capturing in histogram and analyze
it in C code.
We can simply use, rte_trace and so that we can visualize with
standard trace viewers.
Also add add python based script parse the CTF emitted by trace and find
min/max/avg values to simply the code.

> further processed with the jitterdebugger visualization scripts.
> To debug latency spikes, a max threshold can be defined.
> If it is hit, a trace point is created on both instances.
>
> Signed-off-by: Felix Moessbauer <felix.moessbauer@siemens.com>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  app/l2reflect/colors.c    |   34 ++
>  app/l2reflect/colors.h    |   19 +
>  app/l2reflect/l2reflect.h |   53 ++
>  app/l2reflect/main.c      | 1007 +++++++++++++++++++++++++++++++++++++
>  app/l2reflect/meson.build |   21 +
>  app/l2reflect/payload.h   |   26 +
>  app/l2reflect/stats.c     |  225 +++++++++
>  app/l2reflect/stats.h     |   67 +++
>  app/l2reflect/utils.c     |   67 +++
>  app/l2reflect/utils.h     |   20 +
>  app/meson.build           |    1 +

Need to add a doc for this example at doc/guides/sample_app_ug/

>
> diff --git a/app/l2reflect/colors.c b/app/l2reflect/colors.c
> new file mode 100644
> index 0000000000..af881d8788
> --- /dev/null
> +++ b/app/l2reflect/colors.c
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Siemens AG

2022. Please fix in all files.

> diff --git a/app/l2reflect/l2reflect.h b/app/l2reflect/l2reflect.h
> new file mode 100644
> index 0000000000..922bd7c281
> --- /dev/null
> +++ b/app/l2reflect/l2reflect.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Siemens AG
> + */
> +
> +#include <stdatomic.h>
> +
> +#ifndef _L2REFLECT_L2REFLECT_H_
> +#define _L2REFLECT_L2REFLECT_H_
> +
> +#define RTE_LOGTYPE_L2REFLECT RTE_LOGTYPE_USER1

Don't define RTE_ symbols in application.

> +
> +/* max size that common 1G NICs support */
> +#define MAX_JUMBO_PKT_LEN 9600
> +
> +/* Used to compare MAC addresses. */
> +#define MAC_ADDR_CMP 0xFFFFFFFFFFFFull
> +
> +#define MAX(x, y) (((x) > (y)) ? (x) : (y))
> +#define MIN(x, y) (((x) < (y)) ? (x) : (y))


Use RTE_MAX and friends. Similar comments for other macros.


> +extern int l2reflect_hist;
> +extern unsigned int l2reflect_hist_buckets;
> +extern atomic_int l2reflect_output_hist;
> +extern int l2reflect_interrupt;
> +extern uint64_t l2reflect_sleep_msec;
> +extern uint64_t l2reflect_pkt_bytes;
> +extern uint16_t l2reflect_port_number;
> +extern atomic_int l2reflect_state;
> +extern struct rte_ether_addr l2reflect_port_eth_addr;
> +extern struct rte_ether_addr l2reflect_remote_eth_addr;

It is better to move all global variables to app_ctx kind of structure and
allocate from hugepage as some of them are used in fastpath and easy to track.

> +/* Configurable number of RX/TX ring descriptors */
> +#define RTE_TEST_RX_DESC_DEFAULT 128
> +#define RTE_TEST_TX_DESC_DEFAULT 128

Don't define RTE_ symbols in application.

> +/* Send a burst of one packet */
> +static inline int
> +l2reflect_send_packet(struct rte_mbuf **m, uint16_t port)
> +{
> +       unsigned int ret;
> +       struct rte_ether_hdr *eth;
> +       struct my_magic_packet *pkt;
> +       uint16_t type;
> +
> +       eth = rte_pktmbuf_mtod(*m, struct rte_ether_hdr *);
> +       pkt = (struct my_magic_packet *)eth;
> +       type = pkt->type;
> +
> +       if (likely(type == TRACE_TYPE_DATA))
> +               clock_gettime(CLOCK_MONOTONIC, &last_sent);

Use rte_rdtsc_precise() instead of system call  in fastpath

> +       ret = rte_eth_tx_burst(port, l2reflect_q, m, 1);
> +       if (unlikely(ret < 1))
> +               rte_pktmbuf_free(*m);
> +       return 0;
> +}
> +
> +static inline void
> +l2reflect_simple_forward(struct rte_mbuf *m)
> +{
> +       struct rte_ether_hdr *eth;
> +       struct my_magic_packet *pkt;
> +
> +       eth = rte_pktmbuf_mtod(m, struct rte_ether_hdr *);
> +       pkt = (struct my_magic_packet *)eth;
> +
> +       /* dst addr */
> +       rte_ether_addr_copy(&eth->src_addr, &eth->dst_addr);
> +       /* src addr */
> +       rte_ether_addr_copy(&l2reflect_port_eth_addr, &eth->src_addr);
> +
> +       if (unlikely(pkt->magic == MAGIC_TRACE_PAYLOAD))
> +               rte_eal_trace_generic_str("sending traced packet");

Create app_trace tracepoint specific to application with new type to
avoid emit the string.


> +
> +       l2reflect_send_packet(&m, l2reflect_port_number);
> +}

> +
> +/*
> + * process a single packet.
> + * return false if latency threshold is hit
> + */
> +static inline int
> +process_packet(
> +       struct my_magic_packet *pkt,
> +       struct timespec *rx_time,
> +       uint64_t *diff)
> +{
> +       if (pkt->type == TRACE_TYPE_DATA) {
> +               rte_memcpy(&last_recv, rx_time, sizeof(*rx_time));
> +               *diff = calcdiff_ns(last_recv, last_sent);
> +               if (!unlikely(add_to_record(*diff))) {
> +                       /* TODO: improve tracing */
> +                       rte_eal_trace_generic_u64(record.max_round_ns / 1000);

Add a new trace point as an application specific item.

> +                       return 0;
> +               }
> +       }
> +       if (pkt->magic == MAGIC_TRACE_PAYLOAD)
> +               rte_eal_trace_generic_str("received traced packet");

Add a new trace point as an application specific item.

> +
> +       return 1;
> +}
> +
> +/*
> + * free all packet buffers in the range [begin, end[.
> + */
> +static void
> +free_pktbufs(
> +               struct rte_mbuf **bufs,
> +               int begin,
> +               int end)
> +{
> +       int i = begin;
> +       for (; i < end; i++)
> +               rte_pktmbuf_free(bufs[0]);

Freeing the same packet multiple times?

Use burst varint if possible.



> +}
> +
> +/*
> + * return 1 in case the ball was lost (cheap check)
> + */
> +static inline void
> +check_ball_lost(const uint64_t dp_idle) {
> +       /* only check if we are in running state and have a breakval */
> +       if (unlikely(dp_idle & RX_TIMEOUT_MASK) &&
> +          l2reflect_state == S_RUNNING &&
> +          l2reflect_break_usec &&
> +          record.rounds > WARMUP_ROUNDS) {
> +               RTE_LOG(INFO, L2REFLECT, "lost ball after %" PRIu64 " rounds\n", record.rounds);
> +               l2reflect_state = S_LOCAL_TERM;
> +       }
> +}
> +}
> +
> +       /*
> +        * in quiet mode the primary executes the main packet loop
> +        * otherwise the one worker does it and the primary prints stats
> +        */
> +       if (quiet) {
> +               assert(rte_lcore_count() == 1);
> +#ifdef HAS_SYS_IO
> +               if (disable_int) {
> +                       iopl(3);
> +                       asm("cli");

Do we need x86 specific items in application?

> +               }
> +#endif
> +               RTE_LOG(INFO, L2REFLECT, "PID %i, Parent %i\n", getpid(), getppid());
> +               l2reflect_launch_one_lcore(NULL);
> +       } else {
> +               assert(rte_lcore_count() == 2);
> +               /* the worker reflects the packets */
> +               RTE_LCORE_FOREACH_WORKER(lcore_id)
> +               {
> +                       rte_eal_remote_launch(l2reflect_launch_one_lcore, NULL,
> +                                             lcore_id);
> +               }
> +
> +               /* the primary prints the stats */
> +               init_record();
> +               l2reflect_stats_loop();
> +               rte_eal_mp_wait_lcore();
> +       }
> +       rte_eal_cleanup();
> +
> +       if (l2reflect_hist)
> +               output_histogram_snapshot();
> +
> +       cleanup_record();
> +
> +       return 0;
> +}
> diff --git a/app/l2reflect/meson.build b/app/l2reflect/meson.build
> new file mode 100644
> index 0000000000..14b154ef06
> --- /dev/null
> +++ b/app/l2reflect/meson.build
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2022 Siemens AG
> +
> +cc = meson.get_compiler('c')
> +
> +jansson = dependency('libjansson', required: false)
> +
> +if not jansson.found()
> +    jansson = cc.find_library('jansson', required: false)
> +endif
> +
> +if cc.has_header('sys/io.h')
> +    cflags += '-DHAS_SYS_IO'
> +endif
> +
> +sources = files('main.c', 'utils.c', 'stats.c', 'colors.c')
> +deps += ['ethdev']
> +if jansson.found()
> +    ext_deps += jansson
> +    cflags += '-DRTE_HAS_JANSSON'
> +endif
> diff --git a/app/l2reflect/payload.h b/app/l2reflect/payload.h
> new file mode 100644
> index 0000000000..c1fae5d5e4
> --- /dev/null
> +++ b/app/l2reflect/payload.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2022 Siemens AG
> + */
> +#include <rte_ether.h>
> +
> +#ifndef _L2REFLECT_PAYLOAD_H_
> +#define _L2REFLECT_PAYLOAD_H_
> +
> +#define MAGIC_TRACE_PAYLOAD 0xd00faffeaffed00full
> +/* IEEE Std 802 - Local Experimental Ethertype */
> +#define ETHER_TYPE_L2REFLECT 0x88B5
> +
> +struct my_magic_packet {
> +       /* l2 packet header */
> +       struct rte_ether_hdr eth;
> +       /* type of the l2reflect packet */
> +       uint8_t type;
> +       /* magic easy-to-spot pattern for tracing */
> +       uint64_t magic;
> +       /* break if latency is larger than this */
> +       uint64_t breakval;
> +       /* intended size of the packet */
> +       uint64_t req_pkt_bytes;
> +};
> +
> +#endif /* _L2REFLECT_PAYLOAD_H_ */