From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id E3D77A04AC; Tue, 1 Sep 2020 17:07:49 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id E80B71C0AF; Tue, 1 Sep 2020 17:07:48 +0200 (CEST) Received: from new2-smtp.messagingengine.com (new2-smtp.messagingengine.com [66.111.4.224]) by dpdk.org (Postfix) with ESMTP id 2C9DB5B30 for ; Tue, 1 Sep 2020 17:07:47 +0200 (CEST) Received: from compute7.internal (compute7.nyi.internal [10.202.2.47]) by mailnew.nyi.internal (Postfix) with ESMTP id B9950580600; Tue, 1 Sep 2020 11:07:44 -0400 (EDT) Received: from mailfrontend1 ([10.202.2.162]) by compute7.internal (MEProxy); Tue, 01 Sep 2020 11:07:44 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding:content-type; s=fm2; bh= OxZ7f0GA5peiz1j3cow0eGofTKnFZ8WWIIZri69m1pg=; b=vhywwEmNq5bdGWBL ReXch3qn23n1NIVXspz0Qw23zXaV9AWRuhNHebw0EmtRNKHGJQ6cAwqYlLTXJ7au HeraN2236aou4+sAbR6AWea/A2WVWr2xQ8JZWw7UD3q6I9Yjb4Q4ykarRWO83zFb NrMn9nH4eJwdczntrKuLvWBFWeQQmRfOWm9PPHZkll9vv/iUuuxzxWR2ItErxxsv YLm4+ASaSxl28Gu5SPLNbyPHEWIB3f9Y2NBEZajD6UjKxDcj5tvE6E/XYGSZr0RC 8kaP+TFzmCoV0nUVHBlBo/284gWMXEHoL6nEePKJ1dSgopqc+CQZhDXphEoZ8RzL /jo9mA== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-proxy:x-me-proxy:x-me-sender:x-me-sender :x-sasl-enc; s=fm3; bh=OxZ7f0GA5peiz1j3cow0eGofTKnFZ8WWIIZri69m1 pg=; b=aXVs+UyQ83f/D9QzwRfG9JunXmHbNOrB7n+VToYhEnCnjqf3CSeXhdvfi 2aUtuGDXb2L4B+7sHsaP4ZEnNHxjH3obvxZsnFec7WXATTILTteAeMbL4QuEoKbn tG/pU7/UeKgAOD+MdTH2X4p5hG5SasdT2X35Bv4aCq81+jXZ4W3pQ4Nh2Ec0rcg+ tb3VB6L/ieWQLR7+/CmS5Hp0OE3nU4BO9LR09KUcLIoPtSonDCZyNHoLE8eHg3aD /BsaUo73m0Wh83lvhHKP/4rP6UQ+EjwiuIQX4+gYCCAXJ+BvJTFBuBYBxhMdxkV7 CDy6M+6Bz1KPnaXZrUpZdgzzevI7g== X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedrudefjedgkeeiucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg ftrfgrthhtvghrnhepgffhteelfeeuueeuvddtteeugfevtdeftdeiudfgkeeileeikedu hfdvvdeitddvnecuffhomhgrihhnpehfohhsuggvmhdrohhrghdplhhinhhugihfohhunh gurghtihhonhdrohhrghenucfkphepjeejrddufeegrddvtdefrddukeegnecuvehluhhs thgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmoh hnjhgrlhhonhdrnhgvth X-ME-Proxy: Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 67D94328005D; Tue, 1 Sep 2020 11:07:40 -0400 (EDT) From: Thomas Monjalon To: Henning Schild Cc: Felix Moessbauer , dev@dpdk.org, ktraynor@redhat.com, ali.rizvi@emumba.com, Muhammad Ahmad , david.hunt@intel.com, reshma.pattan@intel.com, konstantin.ananyev@intel.com, bruce.richardson@intel.com, ferruh.yigit@intel.com, jerinj@marvell.com, honnappa.nagarahalli@arm.com, olivier.matz@6wind.com, lijuan.tu@intel.com, Owen Hilyard , Morten =?ISO-8859-1?Q?Br=F8rup?= , viacheslavo@nvidia.com, Stephen Hemminger , anatoly.burakov@intel.com Date: Tue, 01 Sep 2020 17:07:38 +0200 Message-ID: <7023104.8KzyT1xApx@thomas> In-Reply-To: <20200828162203.320d06da@md1za8fc.ad001.siemens.net> References: <20200709092711.14900-3-felix.moessbauer@siemens.com> <20200715155409.9835-3-felix.moessbauer@siemens.com> <20200828162203.320d06da@md1za8fc.ad001.siemens.net> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [PATCH v4 2/2] Add l2reflect measurement application X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 28/08/2020 16:22, Henning Schild: > Thomas, > > what is the state on this one? Any more steps to take or people to > involve? I can try adding some key people in Cc list, while doing a quick review. > I first showed that in action back in 2016 on FOSDEM. > https://archive.fosdem.org/2016/schedule/event/virt_iaas_real_time_cloud/ > After my talk two devs from dpdk approached me because they liked the > idea of a "network cyclictest". It took us some time to finally go > upstream with it, unfortunately i do not recall names. I think I was one of them. There will be some talks about latency in the next virtual DPDK event: https://events.linuxfoundation.org/dpdk-userspace-summit/program/schedule/ (speakers are Cc'ed) > This application can potentially be integrated into the test-suite for > QA, making sure no changes heavily increase the package transmission > worst case timing. Good > 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 > > further processed with the jitterdebugger visualization scripts. Please can you show an example of script usage? > > 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 [...] > > --- a/app/Makefile > > +++ b/app/Makefile No need to update Makefile, it will be removed. > > --- /dev/null > > +++ b/app/l2reflect/l2reflect.h > > @@ -0,0 +1,62 @@ > > +/* > > + * SPDX-License-Identifier: BSD-3-Clause SPDX should be the first line. > > + * Copyright(c) 2020 Siemens AG > > + * > > + * authors: > > + * Felix Moessbauer > > + * Henning Schild It is uncommon to mention authors in the file. In general, git metadata covers this info. Any special requirement? [...] > > + > > +/* > > + * Compares a packet destination MAC address to a device MAC address. > > + */ > > +static __rte_always_inline int > > +ether_addr_cmp(struct rte_ether_addr *ea, struct rte_ether_addr *eb) > > +{ > > + return ((*(uint64_t *)ea ^ *(uint64_t *)eb) & MAC_ADDR_CMP) > > == 0; +} Please use rte_is_same_ether_addr() > > --- /dev/null > > +++ b/app/l2reflect/main.c > > @@ -0,0 +1,872 @@ > > +/* > > + * SPDX-License-Identifier: BSD-3-Clause > > + * Copyright(c) 2020 Siemens AG > > + * > > + * authors: > > + * Felix Moessbauer > > + * Henning Schild > > + * > > + * launch (non-rt kernel): l2reflect --lcores 0@0,1@6 -n 1 > > + * launch (rt kernel): l2reflect --lcores 0@0,1@6 -n 1 -- -P 50 -r -l Would be better in a --help section or in the doc. > > + * > > + * 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 > > + * 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. > > + */ [...] > > +__rte_format_printf(1, 0) > > +static void > > +trace_write(const char *fmt, ...) > > +{ > > + va_list ap; > > + char buf[256]; > > + int n, err; > > + > > + if (trace_fd == 0) > > + trace_fd = > > open("/sys/kernel/debug/tracing/trace_marker", > > + O_WRONLY); Why not using rte_trace framework? > > + if (trace_fd < 0) > > + return; > > + > > + va_start(ap, fmt); > > + n = vsnprintf(buf, 256, fmt, ap); > > + va_end(ap); > > + > > + err = write(trace_fd, buf, n); > > + assert(err >= 1); > > +} [...] > > +static 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(ð->s_addr, ð->d_addr); > > + /* src addr */ > > + rte_ether_addr_copy(&l2reflect_port_eth_addr, ð->s_addr); > > + > > + if ((eth->ether_type == > > rte_cpu_to_be_16(ETHER_TYPE_L2REFLECT)) && > > + (pkt->magic == MAGIC_TRACE_PAYLOAD)) { > > + /* and the native one */ > > + trace_write("sending traced packet\n"); > > + } > > + > > + l2reflect_send_packet(&m, l2reflect_port_number); > > +} If I understand well, this requires a special cabling? Would it be possible to measure latency of hardware NIC internal loopback or software PMD loopback? [...] > > + printf("from MAC address: %02X:%02X:%02X:%02X:%02X:%02X to" > > + " %02X:%02X:%02X:%02X:%02X:%02X\n\n", > > + eth->s_addr.addr_bytes[0], > > eth->s_addr.addr_bytes[1], > > + eth->s_addr.addr_bytes[2], > > eth->s_addr.addr_bytes[3], > > + eth->s_addr.addr_bytes[4], > > eth->s_addr.addr_bytes[5], > > + eth->d_addr.addr_bytes[0], > > eth->d_addr.addr_bytes[1], > > + eth->d_addr.addr_bytes[2], > > eth->d_addr.addr_bytes[3], > > + eth->d_addr.addr_bytes[4], > > eth->d_addr.addr_bytes[5]); You could also use rte_ether_format_addr() [...] > > +static inline unsigned int > > +l2reflect_rx_filter( > > + struct rte_mbuf **bufs, > > + unsigned int nb_rx, > > + unsigned int data_only) > > +{ > > + struct rte_ether_hdr *eth; > > + struct my_magic_packet *pkt; > > + unsigned int i, ret; > > + > > + ret = 0; > > + for (i = 0; i < nb_rx; i++) { > > + eth = rte_pktmbuf_mtod(bufs[i], struct rte_ether_hdr > > *); > > + if (l2reflect_state != S_ELECT_LEADER && > > + !ether_addr_cmp(ð->s_addr, > > &l2reflect_remote_eth_addr)) > > + goto drop; > > + > > + if (eth->ether_type != > > rte_cpu_to_be_16(ETHER_TYPE_L2REFLECT)) > > + goto drop; > > + > > + pkt = (struct my_magic_packet *)eth; > > + if (data_only && (pkt->type != TRACE_TYPE_DATA && > > + pkt->type != TRACE_TYPE_RSET && > > + pkt->type != TRACE_TYPE_QUIT)) > > + goto drop; > > + > > + bufs[ret++] = bufs[i]; > > + continue; > > +drop: > > + rte_pktmbuf_free(bufs[i]); > > + } > > + > > + return ret; > > +} This function deserves some comments to explain the logic. [...] > > --- /dev/null > > +++ b/app/l2reflect/meson.build > > @@ -0,0 +1,25 @@ > > +# SPDX-License-Identifier: BSD-3-Clause > > +# Copyright(c) 2020 Siemens AG > > + > > +# meson file, for building this example as part of a main DPDK build. > > +cc = meson.get_compiler('c') Not sure you need that line. > > + > > +cjson = dependency('libcjson', required: false) Some other parts require jansson: jansson = dependency('jansson', required: false) How libcjson is different? Would it be possible to unify dependency? > > +if not is_linux > > + build = false Why limiting to Linux? [...] > > +#define ANSI_BOLD_RED "\x1b[01;31m" > > +#define ANSI_BOLD_GREEN "\x1b[01;32m" > > +#define ANSI_BOLD_YELLOW "\x1b[01;33m" > > +#define ANSI_BOLD_BLUE "\x1b[01;34m" > > +#define ANSI_BOLD_MAGENTA "\x1b[01;35m" > > +#define ANSI_BOLD_CYAN "\x1b[01;36m" > > +#define ANSI_RESET "\x1b[0m" Not sure about using colors. If it really adds a value, I think it should be an option, automatically disabled if stdout is redirected. [...] > > --- a/app/meson.build > > +++ b/app/meson.build > > @@ -8,6 +8,7 @@ endif > > apps = [ > > 'pdump', > > 'proc-info', > > + 'l2reflect', > > 'test-acl', > > 'test-bbdev', > > 'test-cmdline', I think you should keep alphabetical ordering. I'm not sure about adding this application in DPDK. I think we need some latency tools, but if it requires a specific setup, it could better fit in DTS. We need more opinions here.