DPDK patches and discussions
 help / color / mirror / Atom feed
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 Brørup <mb@smartsharesystems.com>,
	viacheslavo@nvidia.com,
	Stephen Hemminger <stephen@networkplumber.org>,
	anatoly.burakov@intel.com
Subject: Re: [dpdk-dev] [PATCH v4 2/2] Add l2reflect measurement application
Date: Tue, 01 Sep 2020 17:07:38 +0200
Message-ID: <7023104.8KzyT1xApx@thomas> (raw)
In-Reply-To: <20200828162203.320d06da@md1za8fc.ad001.siemens.net>

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(&eth->s_addr, &eth->d_addr);
> > +	/* src addr */
> > +	rte_ether_addr_copy(&l2reflect_port_eth_addr, &eth->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(&eth->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.



  reply	other threads:[~2020-09-01 15:07 UTC|newest]

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-03 15:18 [dpdk-dev] [RFC PATCH 0/1] " Felix Moessbauer
2020-07-03 15:18 ` [dpdk-dev] [RFC PATCH 1/1] " Felix Moessbauer
2020-07-07 14:11   ` [dpdk-dev] [PATCH v2 0/2] " Felix Moessbauer
2020-07-07 14:11   ` [dpdk-dev] [PATCH v2 1/2] Fix build of apps with external dependencies Felix Moessbauer
2020-07-07 14:11   ` [dpdk-dev] [PATCH v2 2/2] Add l2reflect measurement application Felix Moessbauer
2020-07-09  9:27   ` [dpdk-dev] [PATCH v3 0/2] " Felix Moessbauer
2020-07-09  9:27   ` [dpdk-dev] [PATCH v3 1/2] Fix build of apps with external dependencies Felix Moessbauer
2020-07-09  9:27   ` [dpdk-dev] [PATCH v3 2/2] Add l2reflect measurement application Felix Moessbauer
2020-07-15 15:54     ` [dpdk-dev] [PATCH v4 0/2] " Felix Moessbauer
2020-07-15 15:54     ` [dpdk-dev] [PATCH v4 1/2] Fix build of apps with external dependencies Felix Moessbauer
2020-07-15 15:54     ` [dpdk-dev] [PATCH v4 2/2] Add l2reflect measurement application Felix Moessbauer
2020-08-28 14:22       ` Henning Schild
2020-09-01 15:07         ` Thomas Monjalon [this message]
2020-09-02  7:56           ` Henning Schild
2020-09-02  8:30             ` Thomas Monjalon
2020-09-02  9:10               ` Henning Schild
2020-09-15  8:38       ` [dpdk-dev] [PATCH v5 0/2] " Felix Moessbauer
2020-09-15  8:38       ` [dpdk-dev] [PATCH 1/2] Fix build of apps with external dependencies Felix Moessbauer
2020-10-20 14:12         ` Thomas Monjalon
2020-10-20 14:32           ` Bruce Richardson
2020-09-15  8:38       ` [dpdk-dev] [PATCH 2/2] Add l2reflect measurement application Felix Moessbauer
2020-07-03 15:51 ` [dpdk-dev] [RFC PATCH 0/1] " Thomas Monjalon

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7023104.8KzyT1xApx@thomas \
    --to=thomas@monjalon.net \
    --cc=ali.rizvi@emumba.com \
    --cc=anatoly.burakov@intel.com \
    --cc=bruce.richardson@intel.com \
    --cc=david.hunt@intel.com \
    --cc=dev@dpdk.org \
    --cc=felix.moessbauer@siemens.com \
    --cc=ferruh.yigit@intel.com \
    --cc=henning.schild@siemens.com \
    --cc=honnappa.nagarahalli@arm.com \
    --cc=jerinj@marvell.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=ktraynor@redhat.com \
    --cc=lijuan.tu@intel.com \
    --cc=mb@smartsharesystems.com \
    --cc=muhammad.ahamd@emumba.com \
    --cc=ohilyard@iol.unh.edu \
    --cc=olivier.matz@6wind.com \
    --cc=reshma.pattan@intel.com \
    --cc=stephen@networkplumber.org \
    --cc=viacheslavo@nvidia.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git