From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id AE67DA0540; Thu, 22 Sep 2022 07:56:26 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 6143F4067C; Thu, 22 Sep 2022 07:56:26 +0200 (CEST) Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by mails.dpdk.org (Postfix) with ESMTP id 2934B40156 for ; Thu, 22 Sep 2022 07:56:24 +0200 (CEST) Received: by mail-qt1-f173.google.com with SMTP id s18so5634072qtx.6 for ; Wed, 21 Sep 2022 22:56:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=f5uJSLZNEVtLdTvi7b7k2O5l09uvaD0urQkKAigZJHE=; b=NuSRJsWgKkIA4T3ThB/lmSkb8JB05R+c4X5jBRRgITQDH82MuAXlAA/hFZd+EAFf6c oawJbMeCDHogC9MWiknpgCECq1ZOPX/BBm6u7o5tuC8q0fQsTRBxjdK+jMCFJYOGxTDn KxCqMOZgNMQFUl8YRPHEBk/Ryt19j3W5SYi1SZOCMWyDwDIEw08pLZJOepVZ7oWnHfqc g0MaMi2I80SRO+gi7dodyGBzyuMUfpt/DK6igTqecG7q/P60lXEQx30HLPN09m5LrHc5 LN7fdB11V5X8a1qtvhCAWbbguPdKmusd83NL38JRhb/DqQyPL/ZqRK1oPqQW6/Q/10+f Xrrg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=f5uJSLZNEVtLdTvi7b7k2O5l09uvaD0urQkKAigZJHE=; b=oXSMxJHv+wYwTSs7v6DYSQ7Yss+Ndbg3QNYRu+9c6SsklDNDObsJQlHspuqxZJKAi0 DfDxeDbeBKsx8+/EOmo2f6Aw1I9qydWSP3jokQIGkbMocSJkElcbV/eMIBo10jahNrT+ 5jvGHI4YQ+BvobNBzUqNZUiNQioPpKNJJXtIlCUC37xtbSq1pSwwC1B0FVwHI/KDWgIy tIc0qXN5IUDpCe8IzS0mm+s7i5obxWyzURIlcNufsUaI3DA+QZ5am3zKv6owj+Ck5FqB oucWyONpwZRw23jFskKaX0PyqUyRu4kn6tTStwOGs2Ue2WeS1ilGiiESBAciu2e/VxC2 MM4Q== X-Gm-Message-State: ACrzQf0GOMI7csmtzwBwBdnTjw+ngINtxnJkM6sG4O0IPuYDIjBbfpnM gTR8H9qKSFMORYYYzBj/gh68rQogh2/YArnqRhSL7Dia8tA= X-Google-Smtp-Source: AMsMyM5DufIn98up5TooEKsmi1ILejmstbTYVhGzMVHN0HmiJgil3LKnhV/ohPfkHjrq1XtvmW0P2QYFzRy7sTmaonY= X-Received: by 2002:ac8:5ccd:0:b0:35c:e18b:2be3 with SMTP id s13-20020ac85ccd000000b0035ce18b2be3mr1479571qta.502.1663826183386; Wed, 21 Sep 2022 22:56:23 -0700 (PDT) MIME-Version: 1.0 References: <20220902084533.675698-1-felix.moessbauer@siemens.com> <20220902084533.675698-3-felix.moessbauer@siemens.com> <20220921171302.5289665a@md1za8fc.ad001.siemens.net> In-Reply-To: <20220921171302.5289665a@md1za8fc.ad001.siemens.net> From: Jerin Jacob Date: Thu, 22 Sep 2022 11:25:55 +0530 Message-ID: Subject: Re: [PATCH v6 2/2] Add l2reflect measurement application To: Henning Schild , "Laatz, Kevin" Cc: Felix Moessbauer , dev@dpdk.org, jan.kiszka@siemens.com, thomas@monjalon.net Content-Type: text/plain; charset="UTF-8" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Wed, Sep 21, 2022 at 8:43 PM Henning Schild wrote: > > Am Wed, 21 Sep 2022 20:12:21 +0530 > schrieb Jerin Jacob : > > > On Fri, Sep 2, 2022 at 2:16 PM Felix Moessbauer > > 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. Yes. I would suggest to take this path to accommodate more use case in future like - finding CPU idle time -latency for crypto/dmadev/eventdev enqueue to dequeue -histogram of occupancy for different queues etc This would translate to 1)Adding app/proc-info style app to pull the live trace from primary process 2)Add plugin framework to operate on live trace 3)Add a plugin for this specific use case 4)If needed, a communication from secondary to primary to take action based on live analysis like in this case if stop the primary when latency exceeds certain limit On the plus side, If we move all analysis and presentation to new generic application, your packet forwarding logic can simply move as new fwd_engine in testpmd(see app/test-pmd/noisy_vnf.c as a example for fwdengine) Ideally "eal: add lcore poll busyness telemetry"[1] could converge to this model. [1] https://patches.dpdk.org/project/dpdk/patch/20220914092929.1159773-2-kevin.laatz@intel.com/ > > 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 > > > Signed-off-by: Henning Schild > > > --- > > > 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 > > > + > > > +#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(ð->src_addr, ð->dst_addr); > > > + /* src addr */ > > > + rte_ether_addr_copy(&l2reflect_port_eth_addr, > > > ð->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 > > > + > > > +#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_ */ >