Re: [dpdk-dev] [PATCH v3] app/test-regex: add RegEx test application

[Thomas Monjalon <thomas@monjalon.net>]

29/07/2020 13:26, Ori Kam:
> --- /dev/null
> +++ b/app/test-regex/Makefile
> @@ -0,0 +1,21 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2020 Mellanox Technologies, Ltd
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +#
> +# library name

It's not a library.
You can completely drop this useless comment.

> +#
> +APP = testregex
> +
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
> +
> +#
> +# all source are stored in SRCS-y
> +#
> +SRCS-y := main.c
> +
> +include $(RTE_SDK)/mk/rte.app.mk
> +

extra blank line at EOF

> diff --git a/app/test-regex/main.c b/app/test-regex/main.c
> new file mode 100644
> index 0000000..789d9ec
> --- /dev/null
> +++ b/app/test-regex/main.c
> @@ -0,0 +1,447 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Mellanox Technologies, Ltd
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdint.h>
> +#include <stdbool.h>
> +#include <stdarg.h>
> +#include <ctype.h>
> +#include <errno.h>
> +#include <getopt.h>
> +#include <signal.h>
> +
> +#include <rte_eal.h>
> +#include <rte_common.h>
> +#include <rte_malloc.h>
> +#include <rte_mempool.h>
> +#include <rte_mbuf.h>
> +#include <rte_cycles.h>
> +#include <rte_regexdev.h>
> +
> +#define HELP_VAL 0
> +#define RULES_VAL 1
> +#define DATA_VAL 2
> +#define JOB_VAL 3
> +#define PERF_VAL 4
> +#define ITER_VAL 5

These macros are not used anymore.

> +
> +#define MAX_FILE_NAME 255
> +
> +/* enum that holds the value for the application arguments. */

There is no value in "enum that holds the value for the".
You can just keep "application arguments",
but the real info is to say it is not a value but an argument index.
What about "arguments parsed with getopt_long"?

> +enum app_arg_values {

_values suffix look wrong

> +	ARG_HELP,
> +	ARG_RULES_FILE_NAME,
> +	ARG_DATA_FILE_NAME,
> +	ARG_NUM_OF_JOBS,
> +	ARG_PERF_MODE,
> +	ARG_NUM_OF_ITERATIONS,
> +
> +};
> +
[...]
> +
> +#define MBUF_CACHE_SIZE 256
> +#define MBUF_SIZE (1 << 8)

I expect such definitions at the beginning of the file.

[...]
> +static void
> +extbuf_free_cb(void *addr __rte_unused, void *fcb_opaque __rte_unused)
> +{
> +
> +}

extra blank line

> +
> +#define START_BURST_SIZE 32u

could be at beginning also

> --- /dev/null
> +++ b/app/test-regex/meson.build
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2018 Intel Corporation

Please don't assign copyright to someone not involved.

> +
> +sources = files('main.c')
> +deps = ['regexdev']

[...]
> +Application Options
> +~~~~~~~~~~~~~~~~~~~
> +
> +* ``--rules NAME``: precompiled rule file
> +
> +* ``--data NAME``: data file to use
> +
> +* ``--nb_jobs N``: number of jobs to use
> +
> +* ``--perf N``: only outputs the performance data
> +
> +* ``--nb_iter N``: number of iteration to run
> +
> +* ``--help``: prints this help

Same comment as v1, definition list is better.

> +Compiling the Tool
> +------------------
> +
> +The ``dpdk-test-regex`` application depends on RegEx lib ``rte_regexdev``.

It is obvious.

> +Running the Tool
> +----------------
> +
> +**Step 1: Compile a rule file**
> +
> +In order for the RegEx to work it must have a precompiled rule file.
> +to generate this file there is a need to use a RegEx compiler that matches the
> +RegEx PMD.
> +
> +**Step 2: Generate a data file**
> +
> +The data file, will be used as a source data for the RegEx to work on.
> +
> +**Step 3: Run the tool**
> +
> +The tool has a number of command line options. Here is the sample command line::
> +
> +   .testregex -w 83:00.0 -- --rules rule_file.rof2 --data data_file.txt --job 100

What is .testregex?

OK, these steps are important to understand well.