From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
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 <dev@dpdk.org>; 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: <xms:vmNOX6mF6WAxxizvCslVhcELm1wz5yMMhq475E-bKL_yABePIS3_ow>
<xme:vmNOXx3bRR_BwlyBcyQIIr_ys-R2_nLxHovdYx4mx9uWaE508UjlHcDvFJL_UkVi9
fFZEu1NODEyMPDWIw>
X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgeduiedrudefjedgkeeiucetufdoteggodetrfdotf
fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen
uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne
cujfgurhephffvufffkfgjfhgggfgtsehtufertddttddvnecuhfhrohhmpefvhhhomhgr
shcuofhonhhjrghlohhnuceothhhohhmrghssehmohhnjhgrlhhonhdrnhgvtheqnecugg
ftrfgrthhtvghrnhepgffhteelfeeuueeuvddtteeugfevtdeftdeiudfgkeeileeikedu
hfdvvdeitddvnecuffhomhgrihhnpehfohhsuggvmhdrohhrghdplhhinhhugihfohhunh
gurghtihhonhdrohhrghenucfkphepjeejrddufeegrddvtdefrddukeegnecuvehluhhs
thgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrhhomhepthhhohhmrghssehmoh
hnjhgrlhhonhdrnhgvth
X-ME-Proxy: <xmx:vmNOX4qxti-7zQTX-N_9BcVE58YiJLgpdmQHBOtLl1J6zOmBqrnWCA>
<xmx:vmNOX-n_ZhBdwSfK8IHt5ifsHNfTzIFJtWahz77FZzGwbPdAQauuTQ>
<xmx:vmNOX40Q1rrGStvVWvV_DnAsAdxpyXwjNw6IcgCAEg5TWptPOUcc4g>
<xmx:wGNOXw56ItdB-b22LlHOqO75Z8ychWcrx0d8WfeOwFGLbFWqY_XAvg>
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 <thomas@monjalon.net>
To: Henning Schild <henning.schild@siemens.com>
Cc: Felix Moessbauer <felix.moessbauer@siemens.com>, dev@dpdk.org,
ktraynor@redhat.com, ali.rizvi@emumba.com,
Muhammad Ahmad <muhammad.ahamd@emumba.com>, 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 <ohilyard@iol.unh.edu>,
Morten =?ISO-8859-1?Q?Br=F8rup?= <mb@smartsharesystems.com>,
viacheslavo@nvidia.com, Stephen Hemminger <stephen@networkplumber.org>,
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 <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
<mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
<mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org
Sender: "dev" <dev-bounces@dpdk.org>
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 <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
> > 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 <felix.moessbauer@siemens.com>
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
[...]
> > --- 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 <felix.moessbauer@siemens.com>
> > + * Henning Schild <henning.schild@siemens.com>
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 <felix.moessbauer@siemens.com>
> > + * Henning Schild <henning.schild@siemens.com>
> > + *
> > + * 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.