Re: [dpdk-dev] [PATCH v4 2/2] Add l2reflect measurement application

[Thomas Monjalon <thomas@monjalon.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.