On Tue, Jan 17, 2023 at 01:56:23AM +0000, Cheng Jiang wrote:
> There are many high-performance DMA devices supported in DPDK now, and
> these DMA devices can also be integrated into other modules of DPDK as
> accelerators, such as Vhost. Before integrating DMA into applications,
> developers need to know the performance of these DMA devices in various
> scenarios and the performance of CPUs in the same scenario, such as
> different buffer lengths. Only in this way can we know the target
> performance of the application accelerated by using them. This patch
> introduces a high-performance testing tool, which supports comparing the
> performance of CPU and DMA in different scenarios automatically with a
> pre-set config file. Memory Copy performance test are supported for now.
>
> Signed-off-by: Cheng Jiang <cheng1.ji...@intel.com>
> Signed-off-by: Jiayu Hu <jiayu...@intel.com>
> Signed-off-by: Yuan Wang <yuanx.w...@intel.com>
> Acked-by: Morten Brørup <m...@smartsharesystems.com>
> ---
> v2: fixed some CI issues.
Some first review comments inline below. More will likely follow as I
review it further and try testing it out.
/Bruce
>
> app/meson.build | 1 +
> app/test-dma-perf/benchmark.c | 539 ++++++++++++++++++++++++++++++++++
> app/test-dma-perf/benchmark.h | 12 +
> app/test-dma-perf/config.ini | 61 ++++
> app/test-dma-perf/main.c | 434 +++++++++++++++++++++++++++
> app/test-dma-perf/main.h | 53 ++++
> app/test-dma-perf/meson.build | 22 ++
> 7 files changed, 1122 insertions(+)
> create mode 100644 app/test-dma-perf/benchmark.c
> create mode 100644 app/test-dma-perf/benchmark.h
> create mode 100644 app/test-dma-perf/config.ini
> create mode 100644 app/test-dma-perf/main.c
> create mode 100644 app/test-dma-perf/main.h
> create mode 100644 app/test-dma-perf/meson.build
>
> diff --git a/app/meson.build b/app/meson.build
> index e32ea4bd5c..a060ad2725 100644
> --- a/app/meson.build
> +++ b/app/meson.build
> @@ -28,6 +28,7 @@ apps = [
> 'test-regex',
> 'test-sad',
> 'test-security-perf',
> + 'test-dma-perf',
> ]
Lists in DPDK are always alphabetical when no other order is required,
therefore this new app should be further up the list, after
"test-crypto-perf".
>
> default_cflags = machine_args + ['-DALLOW_EXPERIMENTAL_API']
> diff --git a/app/test-dma-perf/benchmark.c b/app/test-dma-perf/benchmark.c
> new file mode 100644
> index 0000000000..1cb5b0b291
> --- /dev/null
> +++ b/app/test-dma-perf/benchmark.c
> @@ -0,0 +1,539 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2022 Intel Corporation
> + */
> +
> +#include <inttypes.h>
> +
> +#include <rte_time.h>
> +#include <rte_mbuf.h>
> +#include <rte_dmadev.h>
> +#include <rte_malloc.h>
> +#include <rte_lcore.h>
> +
> +#include "main.h"
> +#include "benchmark.h"
> +
> +
> +#define MAX_DMA_CPL_NB 255
> +
> +#define CSV_LINE_DMA_FMT "Scenario %u,%u,%u,%u,%u,%u,%" PRIu64 ",%.3lf,%"
> PRIu64 "\n"
> +#define CSV_LINE_CPU_FMT "Scenario %u,%u,NA,%u,%u,%u,%" PRIu64 ",%.3lf,%"
> PRIu64 "\n"
> +
> +struct lcore_params {
> + uint16_t dev_id;
> + uint32_t nr_buf;
> + uint16_t kick_batch;
> + uint32_t buf_size;
> + uint32_t repeat_times;
> + uint16_t mpool_iter_step;
> + struct rte_mbuf **srcs;
> + struct rte_mbuf **dsts;
> + uint8_t scenario_id;
> +};
> +
> +struct buf_info {
> + struct rte_mbuf **array;
> + uint32_t nr_buf;
> + uint32_t buf_size;
> +};
> +
> +static struct rte_mempool *src_pool;
> +static struct rte_mempool *dst_pool;
> +
> +uint16_t dmadev_ids[MAX_WORKER_NB];
> +uint32_t nb_dmadevs;
> +
> +#define PRINT_ERR(...) print_err(__func__, __LINE__, __VA_ARGS__)
> +
> +static inline int
> +__rte_format_printf(3, 4)
> +print_err(const char *func, int lineno, const char *format, ...)
> +{
> + va_list ap;
> + int ret;
> +
> + ret = fprintf(stderr, "In %s:%d - ", func, lineno);
> + va_start(ap, format);
> + ret += vfprintf(stderr, format, ap);
> + va_end(ap);
> +
> + return ret;
> +}
> +
> +static inline void
> +calc_result(struct lcore_params *p, uint64_t cp_cycle_sum, double time_sec,
> + uint32_t repeat_times, uint32_t *memory, uint64_t
> *ave_cycle,
> + float *bandwidth, uint64_t *ops)
> +{
> + *memory = (p->buf_size * p->nr_buf * 2) / (1024 * 1024);
> + *ave_cycle = cp_cycle_sum / (p->repeat_times * p->nr_buf);
> + *bandwidth = p->buf_size * 8 * rte_get_timer_hz() / (*ave_cycle * 1000
> * 1000 * 1000.0);
> + *ops = (double)p->nr_buf * repeat_times / time_sec;
> +}
> +
> +static void
> +output_result(uint8_t scenario_id, uint32_t lcore_id, uint16_t dev_id,
> uint64_t ave_cycle,
> + uint32_t buf_size, uint32_t nr_buf, uint32_t memory,
> + float bandwidth, uint64_t ops, bool is_dma)
> +{
> + if (is_dma)
> + printf("lcore %u, DMA %u:\n"
> + "average cycles: %" PRIu64 ","
> + " buffer size: %u, nr_buf: %u,"
> + " memory: %uMB, frequency: %" PRIu64 ".\n",
> + lcore_id,
> + dev_id,
> + ave_cycle,
> + buf_size,
> + nr_buf,
> + memory,
> + rte_get_timer_hz());
Longer lines are allowed for strings, so you can merge each line of output
to a single line, which will improve readability.
Also, to shorten the code, there is no reason each parameter needs to go on
its own line.
> + else
> + printf("lcore %u\n"
> + "average cycles: %" PRIu64 ","
> + " buffer size: %u, nr_buf: %u,"
> + " memory: %uMB, frequency: %" PRIu64 ".\n",
> + lcore_id,
> + ave_cycle,
> + buf_size,
> + nr_buf,
> + memory,
> + rte_get_timer_hz());
Suggestion, rather than duplicating the whole output, only the first line
needs to change based on SW vs HW copies. How about:
if (is_dma)
printf("lcore %u, DMA %u\n", lcore_id, dev_id);
else
printf("lcore %u\n", lcore_id);
printf("average cycles: ..." , ...);
> +
> + printf("Average bandwidth: %.3lfGbps, OPS: %" PRIu64 "\n", bandwidth,
> ops);
> +
> + if (is_dma)
> + snprintf(output_str[lcore_id], MAX_OUTPUT_STR_LEN,
> + CSV_LINE_DMA_FMT,
> + scenario_id, lcore_id, dev_id, buf_size,
> + nr_buf, memory, ave_cycle, bandwidth, ops);
> + else
> + snprintf(output_str[lcore_id], MAX_OUTPUT_STR_LEN,
> + CSV_LINE_CPU_FMT,
> + scenario_id, lcore_id, buf_size,
> + nr_buf, memory, ave_cycle, bandwidth, ops);
> +}
> +
> +static inline void
> +cache_flush_buf(void *arg)
For non-x86 builds, you probably need to mark "arg" as unused to avoid
compiler warnings.
Why is the parameter type given as a void pointer, when the type is
unconditionally cast below as "struct buf_info"? Void pointer type should
only be needed if you need to call this via a generic function pointer.
> +{
> +#ifdef RTE_ARCH_X86_64
> + char *data;
> + char *addr;
> + struct buf_info *info = arg;
> + struct rte_mbuf **srcs = info->array;
> + uint32_t i, k;
> +
> + for (i = 0; i < info->nr_buf; i++) {
> + data = rte_pktmbuf_mtod(srcs[i], char *);
> + for (k = 0; k < info->buf_size / 64; k++) {
> + addr = (k * 64 + data);
> + __builtin_ia32_clflush(addr);
> + }
inner loop may be shorter by incrementing loop var by 64, rather than dividing
and then multiplying, since you can eliminate variable "addr".
Also can be more readable with a variable rename:
for (offset = 0; offset < info->buf_size; offset += 64)
__buildin_ia32_clflush(data + offset);
> + }
> +#endif
> +}
> +
> +/* Configuration of device. */
> +static void
> +configure_dmadev_queue(uint32_t dev_id, uint32_t ring_size)
> +{
> + uint16_t vchan = 0;
> + struct rte_dma_info info;
> + struct rte_dma_conf dev_config = { .nb_vchans = 1 };
> + struct rte_dma_vchan_conf qconf = {
> + .direction = RTE_DMA_DIR_MEM_TO_MEM,
> + .nb_desc = ring_size
> + };
> +
> + if (rte_dma_configure(dev_id, &dev_config) != 0)
> + rte_exit(EXIT_FAILURE, "Error with rte_dma_configure()\n");
> +
> + if (rte_dma_vchan_setup(dev_id, vchan, &qconf) != 0) {
> + printf("Error with queue configuration\n");
> + rte_panic();
> + }
> +
Inconsistency here - and below too. Either use rte_exit on failure or use
rte_panic, but don't mix them. Panic seems a little severe, so I suggest
just using rte_exit() in all cases.
> + rte_dma_info_get(dev_id, &info);
> + if (info.nb_vchans != 1) {
> + printf("Error, no configured queues reported on device id
> %u\n", dev_id);
> + rte_panic();
> + }
> + if (rte_dma_start(dev_id) != 0)
> + rte_exit(EXIT_FAILURE, "Error with rte_dma_start()\n");
> +}
> +
> +static int
> +config_dmadevs(uint32_t nb_workers, uint32_t ring_size)
> +{
> + int16_t dev_id = rte_dma_next_dev(0);
> + uint32_t i;
> +
> + nb_dmadevs = 0;
> +
> + for (i = 0; i < nb_workers; i++) {
> + if (dev_id == -1)
> + goto end;
> +
> + dmadev_ids[i] = dev_id;
> + configure_dmadev_queue(dmadev_ids[i], ring_size);
> + dev_id = rte_dma_next_dev(dev_id + 1);
> + ++nb_dmadevs;
Very minor nit, but I'd suggest swapping these last two lines, incrementing
nb_dmadevs right after configuring the device, but before finding a new
one. It just makes more sense to me.
> + }
> +
> +end:
> + if (nb_dmadevs < nb_workers) {
> + printf("Not enough dmadevs (%u) for all workers (%u).\n",
> nb_dmadevs, nb_workers);
> + return -1;
> + }
> +
> + RTE_LOG(INFO, DMA, "Number of used dmadevs: %u.\n", nb_dmadevs);
> +
> + return 0;
> +}
> +
> +static inline void
> +do_dma_mem_copy(uint16_t dev_id, uint32_t nr_buf, uint16_t kick_batch,
> uint32_t buf_size,
> + uint16_t mpool_iter_step, struct rte_mbuf **srcs,
> struct rte_mbuf **dsts)
> +{
> + int64_t async_cnt = 0;
> + int nr_cpl = 0;
> + uint32_t index;
> + uint16_t offset;
> + uint32_t i;
> +
> + for (offset = 0; offset < mpool_iter_step; offset++) {
> + for (i = 0; index = i * mpool_iter_step + offset, index <
> nr_buf; i++) {
Assignment in the condition part of a loop seems wrong. I suggest reworking
this to avoid it.
> + if (unlikely(rte_dma_copy(dev_id,
> + 0,
> + srcs[index]->buf_iova +
> srcs[index]->data_off,
> + dsts[index]->buf_iova +
> dsts[index]->data_off,
rte_pktmbuf_iova() macro can be used here.
> + buf_size,
> + 0) < 0)) {
> + rte_dma_submit(dev_id, 0);
> + while (rte_dma_burst_capacity(dev_id, 0) == 0) {
> + nr_cpl = rte_dma_completed(dev_id, 0,
> MAX_DMA_CPL_NB,
> + NULL, NULL);
> + async_cnt -= nr_cpl;
> + }
> + if (rte_dma_copy(dev_id,
> + 0,
> + srcs[index]->buf_iova +
> srcs[index]->data_off,
> + dsts[index]->buf_iova +
> dsts[index]->data_off,
> + buf_size,
> + 0) < 0) {
> + printf("enqueue fail again at %u\n",
> index);
> + printf("space:%d\n",
> rte_dma_burst_capacity(dev_id, 0));
> + rte_exit(EXIT_FAILURE, "DMA enqueue
> failed\n");
> + }
> + }
> + async_cnt++;
> +
> + /**
> + * When '&' is used to wrap an index, mask must be a
> power of 2.
> + * That is, kick_batch must be 2^n.
I assume that is checked on input processing when parsing the config file?
> + */
> + if (unlikely((async_cnt % kick_batch) == 0)) {
This is an expected condition that will occur with repeatable frequency.
Therefore, unlikely is not really appropriate.
> + rte_dma_submit(dev_id, 0);
> + /* add a poll to avoid ring full */
> + nr_cpl = rte_dma_completed(dev_id, 0,
> MAX_DMA_CPL_NB, NULL, NULL);
> + async_cnt -= nr_cpl;
> + }
> + }
> +
> + rte_dma_submit(dev_id, 0);
> + while (async_cnt > 0) {
> + nr_cpl = rte_dma_completed(dev_id, 0, MAX_DMA_CPL_NB,
> NULL, NULL);
> + async_cnt -= nr_cpl;
> + }
Do we need a timeout here or in the loop above incase of errors that cause
us to not get all the elements back?
> + }
> +}
> +
> +static int
> +dma_mem_copy(void *p)
> +{
I see the call to this function within "remote_launch" uses a cast on the
function. I don't think that typecast should be necessary, but if you keep
it, you can avoid using the void pointer here and just mark the input type
as "struct lcore_params" directly.
> + uint64_t ops;
> + uint32_t memory;
> + float bandwidth;
> + double time_sec;
> + uint32_t lcore_id = rte_lcore_id();
> + struct lcore_params *params = (struct lcore_params *)p;
> + uint32_t repeat_times = params->repeat_times;
> + uint32_t buf_size = params->buf_size;
> + uint16_t kick_batch = params->kick_batch;
> + uint32_t lcore_nr_buf = params->nr_buf;
> + uint16_t dev_id = params->dev_id;
> + uint16_t mpool_iter_step = params->mpool_iter_step;
> + struct rte_mbuf **srcs = params->srcs;
> + struct rte_mbuf **dsts = params->dsts;
> + uint64_t begin, end, total_cycles = 0, avg_cycles = 0;
> + uint32_t r;
> +
> + begin = rte_rdtsc();
> +
> + for (r = 0; r < repeat_times; r++)
> + do_dma_mem_copy(dev_id, lcore_nr_buf, kick_batch, buf_size,
> + mpool_iter_step, srcs, dsts);
> +
> + end = rte_rdtsc();
> + total_cycles = end - begin;
You can do without "end" easily enough:
total_cycles = rte_rdtsc() - begin;
> + time_sec = (double)total_cycles / rte_get_timer_hz();
> +
> + calc_result(params, total_cycles, time_sec, repeat_times, &memory,
> + &avg_cycles, &bandwidth, &ops);
> + output_result(params->scenario_id, lcore_id, dev_id, avg_cycles,
> buf_size, lcore_nr_buf,
> + memory, bandwidth, ops, true);
> +
> + rte_free(p);
> +
> + return 0;
> +}
> +
> +static int
> +cpu_mem_copy(void *p)
> +{
Most of comments from above, also apply here.
> + uint32_t idx;
> + uint32_t lcore_id;
> + uint32_t memory;
> + uint64_t ops;
> + float bandwidth;
> + double time_sec;
> + struct lcore_params *params = (struct lcore_params *)p;
> + uint32_t repeat_times = params->repeat_times;
> + uint32_t buf_size = params->buf_size;
> + uint32_t lcore_nr_buf = params->nr_buf;
> + uint16_t mpool_iter_step = params->mpool_iter_step;
> + struct rte_mbuf **srcs = params->srcs;
> + struct rte_mbuf **dsts = params->dsts;
> + uint64_t begin, end, total_cycles = 0, avg_cycles = 0;
> + uint32_t k, j, offset;
> +
> + begin = rte_rdtsc();
> +
> + for (k = 0; k < repeat_times; k++) {
> + /* copy buffer form src to dst */
> + for (offset = 0; offset < mpool_iter_step; offset++) {
> + for (j = 0; idx = j * mpool_iter_step + offset, idx <
> lcore_nr_buf; j++) {
> + rte_memcpy((void
> *)(uintptr_t)rte_mbuf_data_iova(dsts[idx]),
> + (void
> *)(uintptr_t)rte_mbuf_data_iova(srcs[idx]),
> + (size_t)buf_size);
> + }
> + }
> + }
> +
> + end = rte_rdtsc();
> + total_cycles = end - begin;
> + time_sec = (double)total_cycles / rte_get_timer_hz();
> +
> + lcore_id = rte_lcore_id();
> +
> + calc_result(params, total_cycles, time_sec, repeat_times, &memory,
> + &avg_cycles, &bandwidth, &ops);
> + output_result(params->scenario_id, lcore_id, 0, avg_cycles, buf_size,
> lcore_nr_buf,
> + memory, bandwidth, ops, false);
> +
> + rte_free(p);
> +
> + return 0;
> +}
> +
> +static int
> +setup_memory_env(struct test_configure *cfg, struct rte_mbuf ***srcs,
> + struct rte_mbuf ***dsts)
> +{
> + uint32_t i;
> + unsigned int buf_size = cfg->buf_size.cur;
> + unsigned int nr_sockets;
> + uint32_t nr_buf = cfg->nr_buf;
> +
> + nr_sockets = rte_socket_count();
> + if (cfg->src_numa_node >= nr_sockets ||
> + cfg->dst_numa_node >= nr_sockets) {
> + printf("Error: Source or destination numa exceeds the acture
> numa nodes.\n");
> + return -1;
> + }
> +
> + src_pool = rte_pktmbuf_pool_create("Benchmark_DMA_SRC",
> + nr_buf, /* n == num elements */
> + 64, /* cache size */
> + 0, /* priv size */
> + buf_size + RTE_PKTMBUF_HEADROOM,
> + cfg->src_numa_node);
> + if (src_pool == NULL) {
> + PRINT_ERR("Error with source mempool creation.\n");
> + return -1;
> + }
> +
> + dst_pool = rte_pktmbuf_pool_create("Benchmark_DMA_DST",
> + nr_buf, /* n == num elements */
> + 64, /* cache size */
> + 0, /* priv size */
> + buf_size + RTE_PKTMBUF_HEADROOM,
> + cfg->dst_numa_node);
> + if (dst_pool == NULL) {
> + PRINT_ERR("Error with destination mempool creation.\n");
> + return -1;
> + }
> +
> + *srcs = (struct rte_mbuf **)(malloc(nr_buf * sizeof(struct rte_mbuf
> *)));
Typecast for void * to other types aren't actually necessary in C.
I note some inconsistency in this file with regards to malloc. Here you use
regular malloc, while when building the parameters to pass to the memcpy
functions you use rte_malloc. I suggest standardizing on one or the other
rather than mixing.
> + if (*srcs == NULL) {
> + printf("Error: srcs malloc failed.\n");
> + return -1;
> + }
> +
> + *dsts = (struct rte_mbuf **)(malloc(nr_buf * sizeof(struct rte_mbuf
> *)));
> + if (*dsts == NULL) {
> + printf("Error: dsts malloc failed.\n");
> + return -1;
> + }
> +
> + for (i = 0; i < nr_buf; i++) {
> + (*srcs)[i] = rte_pktmbuf_alloc(src_pool);
> + (*dsts)[i] = rte_pktmbuf_alloc(dst_pool);
Rather than individually allocating you may well manage with
rte_mempool_get_bulk() to allocate all mbufs in one call.
> + if ((!(*srcs)[i]) || (!(*dsts)[i])) {
> + printf("src: %p, dst: %p\n", (*srcs)[i], (*dsts)[i]);
> + return -1;
> + }
> +
> + (*srcs)[i]->data_len = (*srcs)[i]->pkt_len = buf_size;
> + (*dsts)[i]->data_len = (*dsts)[i]->pkt_len = buf_size;
rte_pktmbuf_append() macro can be used here, rather than setting the
lengths manually. However, these values are not actually used anywhere else
in the code, I believe, so setting them is unnecessary. You are manually
tracking the copy lengths throughout the test, and nothing else is working
on the mbufs, so the length the mbuf reports is immaterial..
> + }
> +
> + return 0;
> +}
> +
> +void
> +dma_mem_copy_benchmark(struct test_configure *cfg)
> +{
> + uint32_t i;
> + uint32_t offset;
> + unsigned int lcore_id = 0;
> + struct rte_mbuf **srcs = NULL, **dsts = NULL;
> + unsigned int buf_size = cfg->buf_size.cur;
> + uint16_t kick_batch = cfg->kick_batch.cur;
> + uint16_t mpool_iter_step = cfg->mpool_iter_step;
> + uint32_t nr_buf = cfg->nr_buf = (cfg->mem_size.cur * 1024 * 1024) /
> (cfg->buf_size.cur * 2);
> + uint16_t nb_workers = cfg->nb_workers;
> + uint32_t repeat_times = cfg->repeat_times;
> +
> + if (setup_memory_env(cfg, &srcs, &dsts) < 0)
> + goto out;
> +
> + if (config_dmadevs(nb_workers, cfg->ring_size.cur) < 0)
> + goto out;
> +
> + if (cfg->cache_flush) {
> + struct buf_info info;
> +
> + info.array = srcs;
> + info.buf_size = buf_size;
> + info.nr_buf = nr_buf;
> + cache_flush_buf(&info);
> +
>From what I can see, struct buf_info is only used for passing parameters to
the cache_flush_buf function. The code would be a lot simpler to remove the
structure and just pass 3 parameters to the function directly.
> + info.array = dsts;
> + cache_flush_buf(&info);
> + rte_mb();
> + }
> +
> + printf("Start testing....\n");
> +
> + for (i = 0; i < nb_workers; i++) {
> + lcore_id = rte_get_next_lcore(lcore_id, true, true);
> + offset = nr_buf / nb_workers * i;
> +
> + struct lcore_params *p = rte_malloc(NULL, sizeof(*p), 0);
> + if (!p) {
> + printf("lcore parameters malloc failure for lcore
> %d\n", lcore_id);
> + break;
> + }
> + *p = (struct lcore_params) {
> + dmadev_ids[i],
> + (uint32_t)(nr_buf/nb_workers),
> + kick_batch,
> + buf_size,
> + repeat_times,
> + mpool_iter_step,
> + srcs + offset,
> + dsts + offset,
> + cfg->scenario_id
> + };
> +
> + rte_eal_remote_launch((lcore_function_t *)dma_mem_copy, p,
> lcore_id);
> + }
> +
> + rte_eal_mp_wait_lcore();
> +
> +out:
> + /* free env */
> + if (srcs) {
> + for (i = 0; i < nr_buf; i++)
> + rte_pktmbuf_free(srcs[i]);
> + free(srcs);
> + }
> + if (dsts) {
> + for (i = 0; i < nr_buf; i++)
> + rte_pktmbuf_free(dsts[i]);
> + free(dsts);
> + }
> +
> + if (src_pool)
> + rte_mempool_free(src_pool);
> + if (dst_pool)
> + rte_mempool_free(dst_pool);
> +
> + for (i = 0; i < nb_dmadevs; i++) {
> + printf("Stopping dmadev %d\n", dmadev_ids[i]);
> + rte_dma_stop(dmadev_ids[i]);
> + }
> +}
> +
> +void
> +cpu_mem_copy_benchmark(struct test_configure *cfg)
> +{
> + uint32_t i, offset;
> + uint32_t repeat_times = cfg->repeat_times;
> + uint32_t kick_batch = cfg->kick_batch.cur;
> + uint32_t buf_size = cfg->buf_size.cur;
> + uint32_t nr_buf = cfg->nr_buf = (cfg->mem_size.cur * 1024 * 1024) /
> (cfg->buf_size.cur * 2);
> + uint16_t nb_workers = cfg->nb_workers;
> + uint16_t mpool_iter_step = cfg->mpool_iter_step;
> + struct rte_mbuf **srcs = NULL, **dsts = NULL;
> + unsigned int lcore_id = 0;
> +
> + if (setup_memory_env(cfg, &srcs, &dsts) < 0)
> + goto out;
> +
> + for (i = 0; i < nb_workers; i++) {
> + lcore_id = rte_get_next_lcore(lcore_id, rte_lcore_count() > 1 ?
> 1 : 0, 1);
> + offset = nr_buf / nb_workers * i;
> + struct lcore_params *p = rte_malloc(NULL, sizeof(*p), 0);
> + if (!p) {
> + printf("lcore parameters malloc failure for lcore
> %d\n", lcore_id);
> + break;
> + }
> + *p = (struct lcore_params) { 0, nr_buf/nb_workers, kick_batch,
> + buf_size, repeat_times,
> mpool_iter_step,
> + srcs + offset, dsts + offset,
> cfg->scenario_id };
Formatting should be the same as function above.
> + rte_eal_remote_launch((lcore_function_t *)cpu_mem_copy, p,
> lcore_id);
> + }
> +
> + rte_eal_mp_wait_lcore();
> +
> +out:
> + /* free env */
> + if (srcs) {
> + for (i = 0; i < nr_buf; i++)
> + rte_pktmbuf_free(srcs[i]);
> + free(srcs);
> + }
> + if (dsts) {
> + for (i = 0; i < nr_buf; i++)
> + rte_pktmbuf_free(dsts[i]);
> + free(dsts);
> + }
> +
> + if (src_pool)
> + rte_mempool_free(src_pool);
> + if (dst_pool)
> + rte_mempool_free(dst_pool);
> +}
There seems a quite a bit of common code between the dma_mem_copy_benchmark
and cpu_mem_copy_benchmark. Might be worth investigating if they can be
merged while still keeping readability.
> diff --git a/app/test-dma-perf/benchmark.h b/app/test-dma-perf/benchmark.h
> new file mode 100644
> index 0000000000..f5ad8d6d99
> --- /dev/null
> +++ b/app/test-dma-perf/benchmark.h
> @@ -0,0 +1,12 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2022 Intel Corporation
> + */
> +
> +#ifndef _BENCHMARK_H_
> +#define _BENCHMARK_H_
> +
> +void dma_mem_copy_benchmark(struct test_configure *cfg);
> +
> +void cpu_mem_copy_benchmark(struct test_configure *cfg);
> +
> +#endif /* _BENCHMARK_H_ */
You don't really need two separate headers in this application. Both main.h
and benchmark.h can be merged into one header, since both are always
included in both c files.
> diff --git a/app/test-dma-perf/config.ini b/app/test-dma-perf/config.ini
> new file mode 100644
> index 0000000000..e24bb19414
> --- /dev/null
> +++ b/app/test-dma-perf/config.ini
> @@ -0,0 +1,61 @@
> +
> +; Supported test types:
> +; DMA_MEM_COPY|CPU_MEM_COPY
> +
> +; Parameters:
> +; "mem_size","buf_size","dma_ring_size","kick_batch".
> +; "mem_size" means the size of the memory footprint.
> +; "buf_size" means the memory size of a single operation.
> +; "dma_ring_size" means the dma ring buffer size.
> +; "kick_batch" means dma operation batch size.
> +
> +; Format: variable=first[,last,increment[,ADD|MUL]]
> +; ADD is the default mode.
> +
> +; src_numa_node is used to control the numa node where the source memory is
> allocated.
> +; dst_numa_node is used to control the numa node where the destination
> memory is allocated.
> +
> +; cache_flush is used to control if the cache should be flushed.
> +
> +; repeat_times is used to control the repeat times of the whole case.
> +
> +; worker_threads is used to control the threads number of the test app.
> +; It should be less than the core number.
> +
> +; mpool_iter_step is used to control the buffer continuity.
> +
> +; Bind DMA to lcore:
> +; Specify the "lcore_dma" parameter.
> +; The number of "lcore_dma" should be greater than or equal to the number of
> "worker_threads".
> +; Otherwise the remaining DMA devices will be automatically allocated to
> threads that are not
> +; specified. If EAL parameters "-l" and "-a" are specified, the "lcore_dma"
> should be within
> +; their range.
> +
> +[case1]
> +type=DMA_MEM_COPY
> +mem_size=10
> +buf_size=64,8192,2,MUL
> +dma_ring_size=1024
> +kick_batch=32
> +src_numa_node=0
> +dst_numa_node=0
> +cache_flush=0
> +repeat_times=10
> +worker_threads=1
> +mpool_iter_step=1
> +lcore_dma=lcore3@0000:00:04.0
> +eal_args=--legacy-mem --file-prefix=test
> +
> +[case2]
> +type=CPU_MEM_COPY
> +mem_size=10
> +buf_size=64,8192,2,MUL
> +dma_ring_size=1024
> +kick_batch=32
> +src_numa_node=0
> +dst_numa_node=1
> +cache_flush=0
> +repeat_times=100
> +worker_threads=1
> +mpool_iter_step=1
> +eal_args=--no-pci
> diff --git a/app/test-dma-perf/main.c b/app/test-dma-perf/main.c
> new file mode 100644
> index 0000000000..94ba369539
> --- /dev/null
> +++ b/app/test-dma-perf/main.c
> @@ -0,0 +1,434 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2022 Intel Corporation
> + */
> +
> +#include <stdio.h>
> +#if !defined(RTE_EXEC_ENV_LINUX)
> +
> +int
> +main(int argc, char *argv[])
> +{
> + printf("OS not supported, skipping test\n");
> + return 0;
> +}
> +
What is linux-specific about this app?
If we do need to limit the app to Linux-only I suggest using meson to do so
rather than putting #ifdefs in the code.
> +#else
> +
> +#include <stdlib.h>
> +#include <getopt.h>
> +#include <signal.h>
<snip>
