DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Medvedkin Vladimir <medvedkinv@gmail.com>
Cc: dev@dpdk.org, thomas@monjalon.net, cristian.dumitrescu@intel.com
Subject: Re: [dpdk-dev] [PATCH v4 3/4] Add autotests for RIB library
Date: Fri, 29 Jun 2018 16:07:50 +0100	[thread overview]
Message-ID: <20180629150750.GA4020@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <1524780214-23196-4-git-send-email-medvedkinv@gmail.com>

On Fri, Apr 27, 2018 at 01:03:33AM +0300, Medvedkin Vladimir wrote:
> Signed-off-by: Medvedkin Vladimir <medvedkinv@gmail.com>
> ---
>  test/test/Makefile               |   5 +
>  test/test/meson.build            |   8 +
>  test/test/test_rib.c             | 308 +++++++++++++++++++++++++++++++++++++++
>  test/test/test_rib_generate_rt.c | 297 +++++++++++++++++++++++++++++++++++++
>  test/test/test_rib_generate_rt.h |  38 +++++
>  test/test/test_rib_lpm_comp.c    | 189 ++++++++++++++++++++++++
>  test/test/test_rib_perf.c        | 145 ++++++++++++++++++
>  7 files changed, 990 insertions(+)
>  create mode 100644 test/test/test_rib.c
>  create mode 100644 test/test/test_rib_generate_rt.c
>  create mode 100644 test/test/test_rib_generate_rt.h
>  create mode 100644 test/test/test_rib_lpm_comp.c
>  create mode 100644 test/test/test_rib_perf.c
> 

<snip>

> diff --git a/test/test/test_rib_lpm_comp.c b/test/test/test_rib_lpm_comp.c
> new file mode 100644
> index 0000000..ef48c8c
> --- /dev/null
> +++ b/test/test/test_rib_lpm_comp.c
> @@ -0,0 +1,189 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018 Vladimir Medvedkin <medvedkinv@gmail.com>
> + */
> +
> +#include <stdio.h>
> +#include <stdint.h>
> +#include <stdlib.h>
> +
> +#include <rte_random.h>
> +#include <rte_cycles.h>
> +#include <rte_branch_prediction.h>
> +#include <rte_ip.h>
> +#include <rte_malloc.h>
> +#include <rte_lpm.h>
> +#include <rte_rib.h>
> +
> +#include "test.h"
> +#include "test_xmmt_ops.h"
> +#include "test_rib_generate_rt.h"
> +
> +#define TEST_RIB_ASSERT(cond) do {				\
> +	if (!(cond)) {						\
> +		printf("Error at line %d:\n", __LINE__);	\
> +		return -1;					\
> +	}							\
> +} while (0)
> +
> +#define ITERATIONS (1 << 25)
> +#define BATCH_SIZE (1 << 7)
> +#define BULK_SIZE 32
> +#define LPM_NH_MASK	((1 << 24) - 1)
> +
> +static uint64_t default_nh;
> +
> +static int
> +test_lookup(struct rte_rib *rib, struct rte_lpm *lpm)

It should be fairly obvious, but put in a comment explaining the function
and what it does, and how.

> +{
> +	static uint32_t ip_batch[BATCH_SIZE];
> +	uint64_t rib_next_hops[BULK_SIZE];
> +	uint32_t lpm_next_hops[BULK_SIZE];
> +	int i, j, k;
> +
> +	for (i = 0; i < ITERATIONS; i++) {
> +		for (j = 0; j < BATCH_SIZE; j++)
> +			ip_batch[j] = (i << 7) + j;
> +
> +		for (j = 0; j < BATCH_SIZE; j += BULK_SIZE) {
> +			rte_rib_fib_lookup_bulk(rib, &ip_batch[j],
> +				rib_next_hops, BULK_SIZE);
> +			rte_lpm_lookup_bulk(lpm, &ip_batch[j],
> +				lpm_next_hops, BULK_SIZE);
> +			for (k = 0; k < BULK_SIZE; k++) {
> +				if (likely(lpm_next_hops[k] &
> +					RTE_LPM_LOOKUP_SUCCESS))
> +					lpm_next_hops[k] &= LPM_NH_MASK;
> +				else
> +					lpm_next_hops[k] = default_nh;
> +			}
> +			for (k = 0; k < BULK_SIZE; k++)
> +				TEST_RIB_ASSERT(rib_next_hops[k] ==
> +						lpm_next_hops[k]);
> +		}
> +	}
> +	return 0;
> +}

This looks a good unit test for comparisons. Although it's scanning
linearly, I wonder if it may be worthwhile to rework the loops so you do
all lookups for a batch for lpm first then for fib, and track the cycles
for each. Then at the end you can print out the lookup perf comparison.
Alternatively, an additional batch at the end with random lookups could be
done. [Yes, I know the info can be got by running the perf tests for lpm
and rib separately, but it would be nice to have it as part of a comparison
autotest]

> +
> +static int
> +test_rib_lpm_comp(void)
> +{
> +	struct rte_rib *rib = NULL;
> +	struct rte_lpm *lpm = NULL;
> +	struct route_rule *rt = NULL;
> +	unsigned int i;
> +	int rib_add = 0, lpm_add = 0;
> +	int ret, nh_bits, nr_tbl8;
> +	uint32_t num_routes;
> +	struct rte_rib_conf conf;
> +	struct rte_lpm_config config;
> +
> +	rte_srand(rte_rdtsc());
> +	default_nh = 17;
> +
> +	conf.max_nodes = 3000000;
> +	conf.node_sz = sizeof(struct rte_rib_node);
> +	conf.type = RTE_RIB_DIR24_8;
> +	conf.fib_conf.dir24_8.def_nh = default_nh;
> +	conf.fib_conf.dir24_8.nh_sz = RTE_DIR24_8_8B;
> +
> +	nh_bits = RTE_MIN(((1 << (3 + conf.fib_conf.dir24_8.nh_sz)) - 1), 24);
> +	nr_tbl8 = RTE_MIN(((1 << nh_bits) - 1), 65535);

These two lines need a comment explaining them - especially the former one.

> +	config.number_tbl8s = nr_tbl8;
> +	conf.fib_conf.dir24_8.num_tbl8 = nr_tbl8;
> +	config.max_rules = 2000000;
> +	config.flags = 0;
> +
> +	num_routes = 1200000;
> +
> +	rt = rte_zmalloc("struct route_rule *", sizeof(struct route_rule) *
> +		num_routes + 5, 0);
> +	TEST_RIB_ASSERT(rt != NULL);
> +
> +	num_routes = generate_large_route_rule_table(num_routes, rt);
> +	TEST_RIB_ASSERT(num_routes != 0);
> +	printf("No. routes = %u\n", (unsigned int) num_routes);
> +
> +	shuffle_rt(rt, num_routes);
> +
> +	print_route_distribution(rt, (uint32_t) num_routes);
> +
> +	rib = rte_rib_create(__func__, SOCKET_ID_ANY, &conf);
> +	TEST_RIB_ASSERT(rib != NULL);
> +
> +	lpm = rte_lpm_create(__func__, SOCKET_ID_ANY, &config);
> +	TEST_RIB_ASSERT(lpm != NULL);
> +
> +	for (i = 0; i < num_routes; i++)
> +		rt[i].nh = rte_rand() & ((1ULL << nh_bits) - 1);
> +

Put comment here explaining this next block.
> +	for (i = 0; i < num_routes; i++) {
> +		ret = rte_rib_add(rib, rt[i].ip, rt[i].depth, rt[i].nh);
> +		if (ret == 0)
> +			rib_add++;
> +		else
> +			continue;
> +
> +		ret = rte_lpm_add(lpm, rt[i].ip, rt[i].depth, rt[i].nh);
> +		if (ret == 0)
> +			lpm_add++;
> +		else {
> +			rte_rib_delete(rib, rt[i].ip, rt[i].depth);
> +			rib_add--;
> +		}
> +	}
> +	TEST_RIB_ASSERT(rib_add == lpm_add);
> +
> +	ret = test_lookup(rib, lpm);
> +	if (ret != 0)
> +		return ret;
> +
> +	for (i = 0; i < num_routes; i++) {
> +		if ((i % 3) == 0) {
I assume the intention here is after filling the table and doing the lookup
tests, we drop 1/3 of the entries and retest. Put in a comment explaining
the why.
Rather than putting in the if statement, why not just change the loop to
be:

	for (i = 0; i < num_routes; i += 3)

> +			ret = rte_rib_delete(rib, rt[i].ip, rt[i].depth);
> +			if (ret == 0)
> +				rib_add--;
> +			else
> +				continue;
> +
> +			ret = rte_lpm_delete(lpm, rt[i].ip, rt[i].depth);
> +			if (ret == 0)
> +				lpm_add--;
> +		}
> +	}
> +	TEST_RIB_ASSERT(rib_add == lpm_add);
> +
> +	ret = test_lookup(rib, lpm);
> +	if (ret != 0)
> +		return ret;
> +
> +	for (i = 0; i < num_routes; i++) {
> +		if ((i % 6) == 0) {

As above, put in a comment, and consider removing the if statment. It helps
having the reduced indentation.

> +			ret = rte_rib_add(rib, rt[i].ip, rt[i].depth, rt[i].nh);
> +			if (ret == 0)
> +				rib_add++;
> +			else
> +				continue;
> +
> +			ret = rte_lpm_add(lpm, rt[i].ip, rt[i].depth, rt[i].nh);
> +			if (ret == 0)
> +				lpm_add++;
> +			else {
> +				rte_rib_delete(rib, rt[i].ip, rt[i].depth);
> +				rib_add--;
> +			}
> +		}
> +	}
> +	TEST_RIB_ASSERT(rib_add == lpm_add);
> +
> +	ret = test_lookup(rib, lpm);
> +	if (ret != 0)
> +		return ret;
> +
> +	rte_rib_free(rib);
> +	rte_lpm_free(lpm);
> +	rte_free(rt);
> +
> +	return 0;
> +}

Looks a really good test. Only issue I have is that it sits for a long time
doing the checks in the background without any output. I would therefore
suggest:
* At end of each stage, add, lookup, delete, etc print out a message
  stating what was done with how many entries, e.g X adds, Y lookups etc.
* For any of those individual items that takes a long time on its own,
  consider printing out every e.g. 100,000 items, and/or printing a dot
  every 10,000 items, so the user can see progress.

> +
> +REGISTER_TEST_COMMAND(rib_lpm_comp_autotest, test_rib_lpm_comp);

<snip>

  parent reply	other threads:[~2018-06-29 15:07 UTC|newest]

Thread overview: 61+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-26 22:03 [dpdk-dev] [PATCH v4 0/4] lib/rib: Add Routing Information Base library Medvedkin Vladimir
2018-04-26 22:03 ` [dpdk-dev] [PATCH v4 1/4] Add RIB library Medvedkin Vladimir
2018-04-26 22:17   ` Stephen Hemminger
2018-04-26 22:18   ` Stephen Hemminger
2018-04-26 22:19   ` Stephen Hemminger
2018-04-26 22:19   ` Stephen Hemminger
2018-04-26 22:20   ` Stephen Hemminger
2018-04-27  6:45     ` Vladimir Medvedkin
2018-06-29 13:54   ` Bruce Richardson
2018-06-29 14:02   ` Bruce Richardson
2018-04-26 22:03 ` [dpdk-dev] [PATCH v4 2/4] Add dir24_8 implementation for rib library Medvedkin Vladimir
2018-04-26 22:03 ` [dpdk-dev] [PATCH v4 3/4] Add autotests for RIB library Medvedkin Vladimir
2018-06-29 14:13   ` Bruce Richardson
2018-06-29 15:07   ` Bruce Richardson [this message]
2018-06-29 15:31   ` Bruce Richardson
2018-04-26 22:03 ` [dpdk-dev] [PATCH v4 4/4] Add support for lpm and rib bulk lookup Medvedkin Vladimir
2018-04-26 22:24 ` [dpdk-dev] [PATCH v4 0/4] lib/rib: Add Routing Information Base library Stephen Hemminger
2018-04-26 22:27   ` Thomas Monjalon
2018-04-26 22:42     ` Stephen Hemminger
2018-04-26 22:49     ` Stephen Hemminger
2018-04-27  8:27   ` Vladimir Medvedkin
2018-06-29 15:48 ` Bruce Richardson
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 00/12] lib: add RIB and FIB liraries Vladimir Medvedkin
2019-09-12  7:37   ` Morten Brørup
2019-09-12  9:47     ` Medvedkin, Vladimir
2019-09-12 12:00       ` Morten Brørup
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 " Vladimir Medvedkin
2019-11-05 23:14     ` Thomas Monjalon
2019-11-06  5:50       ` David Marchand
2019-11-06  7:50         ` Thomas Monjalon
2019-11-06 11:53           ` Medvedkin, Vladimir
2019-11-06 11:59             ` Thomas Monjalon
2019-11-06 14:37               ` Aaron Conole
2019-11-06 11:50         ` Medvedkin, Vladimir
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 01/12] rib: add RIB library Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 02/12] test/rib: add RIB library autotests Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 03/12] rib: add ipv6 support for RIB Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 04/12] test/rib: add ipv6 support for RIB autotests Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 05/12] fib: add FIB library Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 06/12] fib: add FIB ipv6 support Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 07/12] fib: add DIR24-8 dataplane algorithm Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 08/12] fib: add dataplane algorithm for ipv6 Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 09/12] test/fib: add FIB library autotests Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 10/12] test/fib: add ipv6 support for FIB autotests Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 11/12] test/fib: add FIB library performance autotests Vladimir Medvedkin
2019-11-01 15:21   ` [dpdk-dev] [PATCH v6 12/12] test/fib: add FIB library ipv6 " Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 01/12] rib: add RIB library Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 02/12] test/rib: add RIB library autotests Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 03/12] rib: add ipv6 support for RIB Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 04/12] test/rib: add ipv6 support for RIB autotests Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 05/12] fib: add FIB library Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 06/12] fib: add FIB ipv6 support Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 07/12] fib: add DIR24-8 dataplane algorithm Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 08/12] fib: add dataplane algorithm for ipv6 Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 09/12] test/fib: add FIB library autotests Vladimir Medvedkin
2019-09-12 14:07   ` Aaron Conole
2019-10-01 17:12     ` Medvedkin, Vladimir
2019-10-24 15:55       ` Thomas Monjalon
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 10/12] test/fib: add ipv6 support for FIB autotests Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 11/12] test/fib: add FIB library performance autotests Vladimir Medvedkin
2019-09-11 17:09 ` [dpdk-dev] [PATCH v5 12/12] test/fib: add FIB library ipv6 " Vladimir Medvedkin

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=20180629150750.GA4020@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=cristian.dumitrescu@intel.com \
    --cc=dev@dpdk.org \
    --cc=medvedkinv@gmail.com \
    --cc=thomas@monjalon.net \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).