From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id EA08D1D7 for ; Fri, 29 Jun 2018 17:07:55 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2018 08:07:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,285,1526367600"; d="scan'208";a="67410515" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.107]) by fmsmga004.fm.intel.com with SMTP; 29 Jun 2018 08:07:52 -0700 Received: by (sSMTP sendmail emulation); Fri, 29 Jun 2018 16:07:51 +0100 Date: Fri, 29 Jun 2018 16:07:50 +0100 From: Bruce Richardson To: Medvedkin Vladimir Cc: dev@dpdk.org, thomas@monjalon.net, cristian.dumitrescu@intel.com Message-ID: <20180629150750.GA4020@bricha3-MOBL.ger.corp.intel.com> References: <1524780214-23196-1-git-send-email-medvedkinv@gmail.com> <1524780214-23196-4-git-send-email-medvedkinv@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1524780214-23196-4-git-send-email-medvedkinv@gmail.com> Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.10.0 (2018-05-17) Subject: Re: [dpdk-dev] [PATCH v4 3/4] Add autotests for RIB library 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: , X-List-Received-Date: Fri, 29 Jun 2018 15:07:56 -0000 On Fri, Apr 27, 2018 at 01:03:33AM +0300, Medvedkin Vladimir wrote: > Signed-off-by: Medvedkin Vladimir > --- > 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 > > 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 > + */ > + > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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);