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

[Henning Schild <henning.schild@siemens.com>]

Am Wed, 21 Sep 2022 20:12:21 +0530
schrieb 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.

We need some logic in the application to have a live view of
min/max/avg. More importantly for the break threshold where we stop the
measurement and the tracing ... so you find the root-cause of your
spike at the end of your trace.

And a trace is only so long until the ring wraps.

So tracing could be the input, but the processing should happen live.
To react on a max threshold, to show live data, to not miss anything
even when running for weeks.

Given that trace processing would need to be live, would you still
suggest to use traces as input and use python to process them? I guess
that could be done with another process and IPC but not sure it would
be nice or how much code it would save.

Henning

> > 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_ */