DPDK patches and discussions
 help / color / mirror / Atom feed
From: Thomas Monjalon <thomas@monjalon.net>
To: jerinj@marvell.com, xiang.w.wang@intel.com, matan@mellanox.com,
	viacheslavo@mellanox.com, John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	dev@dpdk.org, Ori Kam <orika@mellanox.com>
Cc: guyk@marvell.com, dev@dpdk.org, pbhagavatula@marvell.com,
	shahafs@mellanox.com, hemant.agrawal@nxp.com, opher@mellanox.com,
	alexr@mellanox.com, dovrat@marvell.com, pkapoor@marvell.com,
	nipun.gupta@nxp.com, bruce.richardson@intel.com,
	yang.a.hong@intel.com, harry.chang@intel.com,
	gu.jian1@zte.com.cn, shanjiangh@chinatelecom.cn,
	zhangy.yun@chinatelecom.cn, lixingfu@huachentel.com,
	wushuai@inspur.com, yuyingxia@yxlink.com,
	fanchenggang@sunyainfo.com, davidfgao@tencent.com,
	liuzhong1@chinaunicom.cn, zhaoyong11@huawei.com, oc@yunify.com,
	jim@netgate.com, hongjun.ni@intel.com, deri@ntop.org,
	fc@napatech.com, arthur.su@lionic.com, orika@mellanox.com,
	rasland@mellanox.com, Yuval Avnery <yuvalav@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v1] app/test-regex: add RegEx test application
Date: Mon, 27 Jul 2020 19:09:34 +0200
Message-ID: <3437146.xsi8y2SdMt@thomas> (raw)
In-Reply-To: <1595793496-73205-1-git-send-email-orika@mellanox.com>

26/07/2020 21:58, Ori Kam:
> --- a/app/meson.build
> +++ b/app/meson.build
> @@ -12,6 +12,7 @@ apps = [
>  	'test-bbdev',
>  	'test-cmdline',
>  	'test-compress-perf',
> +	'test-regex',
>  	'test-crypto-perf',
>  	'test-eventdev',
>  	'test-fib',

In this list, I think the alphabetical order was chosen.

> diff --git a/app/test-regex/Makefile b/app/test-regex/Makefile
> new file mode 100644
> index 0000000..d73e776
> --- /dev/null
> +++ b/app/test-regex/Makefile
> @@ -0,0 +1,24 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright(c) 2020 Mellanox Corporation

It does not comply with Mellanox copyright syntax.
Note: I already did this comment in recent past.

> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +ifeq ($(CONFIG_RTE_LIBRTE_REGEXDEV),y)
> +
> +#
> +# library name
> +#
> +APP = testregex
> +
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -Wno-deprecated-declarations

This flag is not acceptable.

> +
> +#
> +# all source are stored in SRCS-y
> +#
> +SRCS-y := main.c
> +
> +include $(RTE_SDK)/mk/rte.app.mk
> +
> +endif
[...]
> --- /dev/null
> +++ b/app/test-regex/generate_data_file.py
> @@ -0,0 +1,29 @@
> +#!/usr/bin/env python
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2020 Mellanox Technologies, Ltd
> +
> +import random
> +
> +KEYWORD = 'hello world'
> +MAX_COUNT = 10
> +MIN_COUNT = 5
> +MAX_LEN = 1024
> +REPEAT_COUNT = random.randrange(MIN_COUNT, MAX_COUNT)
> +
> +current_pos = 0;
> +match_pos = []
> +
> +fd_input = open('input.txt','w')
> +fd_res = open('res.txt','w')

space missing

> +
> +for i in range(REPEAT_COUNT):
> +    rand = random.randrange(MAX_LEN)
> +    fd_input.write(' ' * rand)
> +    current_pos += rand
> +    fd_input.write(KEYWORD)
> +    match_pos.append(current_pos)
> +    fd_res.write('{}\n'.format(str(current_pos)))
> +    current_pos += len(KEYWORD)
> +
> +fd_input.close()
> +fd_res.close()

I think there is a more pythonic way of writing in a file.
At least, you can use "with".

> --- /dev/null
> +++ b/app/test-regex/hello_world.rof2

Already discussed in a separate thread.
This file should be generated.

> --- /dev/null
> +++ b/app/test-regex/main.c
> @@ -0,0 +1,429 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020 Mellanox Technologies, Ltd
> + *
> + * This file contain the application main file
> + * This application provides a way to test the RegEx class.

In general I like comments explaining what is a file for.
But here it looks useless.

> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <stdint.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

Please add comments to explain what are these constants for.
I think an enum, and a common prefix would be better.

> +#define MAX_FILE_NAME 255
> +
> +static char rules_file[MAX_FILE_NAME];
> +static char data_file[MAX_FILE_NAME];
> +static uint32_t jobs;
> +static struct rte_mempool *mbuf_mp;
> +static uint8_t nb_max_matches;
> +static uint16_t nb_max_payload;
> +static int perf_test;
> +static uint32_t iter;

Please avoid global variables.

> +
> +static void
> +usage(const char *prog_name)
> +{
> +	printf("%s [EAL options] --\n"
> +		" --rules NAME: precompiled rules file\n"
> +		" --data NAME: data file to use\n"
> +		" --nb_jobs: number of jobs to use\n"
> +		" --perf N: only outputs the performance data\n"
> +		" --nb_iter N: number of iteration to run\n",
> +		prog_name);
> +}
> +
> +static void
> +args_parse(int argc, char **argv)
> +{
> +	char **argvopt;
> +	int opt;
> +	int opt_idx;
> +	size_t len;
> +	static struct option lgopts[] = {
> +		{ "help",  0, 0, HELP_VAL },
> +		{ "rules",  1, 0, RULES_VAL },
> +		/* Rules database file to load. */
> +		{ "data",  1, 0, DATA_VAL },
> +		/* Data file to load. */
> +		{ "nb_jobs",  1, 0, JOB_VAL },
> +		/* Number of jobs to create. */
> +		{ "perf", 0, 0, PERF_VAL},
> +		/* Perf test only */
> +		{ "nb_iter", 1, 0, ITER_VAL}
> +		/* Number of iterations to run with perf test */
> +	};
> +
> +	argvopt = argv;
> +

Useless newline.

> +	while ((opt = getopt_long(argc, argvopt, "",
> +				lgopts, &opt_idx)) != EOF) {
> +		switch (opt) {

[...]
> +#define MBUF_SIZE (1 << 14)

Why this size?
Add a comment?

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

Empty function can be dropped.

[...]
> +It is based on precomplied rule file, and an input file, both of them can

precompiled

> +be selected using command-line options.
> +
> +In general case, each PMD has it's own rule file.

its

> +
> +The test outputs the performance, the results matching (rule id, position, len)

length

A list will look better.

> +for each job and also a list of matches (rule id, position , len) in absulote

absolute

> +position.
> +
> +
> +Limitations
> +~~~~~~~~~~~
> +
> +* Only one queue is supported.
> +
> +* Supports only precompiled rules.
> +
> +EAL Options
> +~~~~~~~~~~~
> +
> +The following are the EAL command-line options that can be used in conjunction
> +with the ``dpdk-test-regex`` application.
> +See the DPDK Getting Started Guides for more information on these options.
> +
> +
> +*   ``-w <PCI>``
> +
> +	Add a PCI device in white list.

Please drop "EAL options" chapter.
It is not specific to the app.

> +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

Please use definition list.

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

Useless

> +
> +
> +Generating the data
> +-------------------
> +
> +In the current version, the compiled rule file is loaded with a rule that
> +matches 'hello world'. To create the data file,
> +it is possible to use the included python script ``generate_data_file.py``
> + which generates two files,
> +``input.txt`` which holds the input buffer. An input buffer is a random number
> +of spaces chars followed by the phrase 'hello world'.
> +This sequence is repeated a random number of times.
> +The second file is ``res.txt`` which holds the position of each
> +of the 'hello world' in the input file.

A script is missing to generate a default set of input data.


> +Running the Tool
> +----------------
> +
> +The tool has a number of command line options. Here is the sample command line:
> +
> +.. code-block:: console
> +
> +   ./build/app/testregex -w 83:00.0 -- --rules app/test-regex/hello_world.rof2 --data app/test-regex/input.txt --job 100


Bottom line, I think this application is not ready for DPDK 20.08.
It's good to have it available as a patch for first users who
want to play with the new regex library.
However, I propose waiting 20.11 to integrate a better version of it.



  parent reply	other threads:[~2020-07-27 17:09 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-26 19:58 Ori Kam
2020-07-27  4:50 ` Jerin Jacob
2020-07-27  5:12   ` Ori Kam
2020-07-27 16:36     ` Thomas Monjalon
2020-07-28  4:36       ` Ori Kam
2020-07-27 17:09 ` Thomas Monjalon [this message]
2020-07-28  4:29   ` Ori Kam
2020-07-29 11:13 ` [dpdk-dev] [PATCH v2] " Ori Kam
2020-07-29 11:26 ` [dpdk-dev] [PATCH v3] " Ori Kam
2020-07-29 13:54   ` Thomas Monjalon
2020-07-29 18:09 ` [dpdk-dev] [PATCH v4] " Ori Kam
2020-07-30  7:08   ` Thomas Monjalon

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=3437146.xsi8y2SdMt@thomas \
    --to=thomas@monjalon.net \
    --cc=alexr@mellanox.com \
    --cc=arthur.su@lionic.com \
    --cc=bruce.richardson@intel.com \
    --cc=davidfgao@tencent.com \
    --cc=deri@ntop.org \
    --cc=dev@dpdk.org \
    --cc=dovrat@marvell.com \
    --cc=fanchenggang@sunyainfo.com \
    --cc=fc@napatech.com \
    --cc=gu.jian1@zte.com.cn \
    --cc=guyk@marvell.com \
    --cc=harry.chang@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=hongjun.ni@intel.com \
    --cc=jerinj@marvell.com \
    --cc=jim@netgate.com \
    --cc=john.mcnamara@intel.com \
    --cc=liuzhong1@chinaunicom.cn \
    --cc=lixingfu@huachentel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=matan@mellanox.com \
    --cc=nipun.gupta@nxp.com \
    --cc=oc@yunify.com \
    --cc=opher@mellanox.com \
    --cc=orika@mellanox.com \
    --cc=pbhagavatula@marvell.com \
    --cc=pkapoor@marvell.com \
    --cc=rasland@mellanox.com \
    --cc=shahafs@mellanox.com \
    --cc=shanjiangh@chinatelecom.cn \
    --cc=viacheslavo@mellanox.com \
    --cc=wushuai@inspur.com \
    --cc=xiang.w.wang@intel.com \
    --cc=yang.a.hong@intel.com \
    --cc=yuvalav@mellanox.com \
    --cc=yuyingxia@yxlink.com \
    --cc=zhangy.yun@chinatelecom.cn \
    --cc=zhaoyong11@huawei.com \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git