From: Ori Kam <orika@mellanox.com>
To: Thomas Monjalon <thomas@monjalon.net>,
"jerinj@marvell.com" <jerinj@marvell.com>,
"xiang.w.wang@intel.com" <xiang.w.wang@intel.com>,
Matan Azrad <matan@mellanox.com>,
Slava Ovsiienko <viacheslavo@mellanox.com>,
John McNamara <john.mcnamara@intel.com>,
Marko Kovacevic <marko.kovacevic@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "guyk@marvell.com" <guyk@marvell.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"pbhagavatula@marvell.com" <pbhagavatula@marvell.com>,
Shahaf Shuler <shahafs@mellanox.com>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
Opher Reviv <opher@mellanox.com>,
Alex Rosenbaum <alexr@mellanox.com>,
"dovrat@marvell.com" <dovrat@marvell.com>,
"pkapoor@marvell.com" <pkapoor@marvell.com>,
"nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
"bruce.richardson@intel.com" <bruce.richardson@intel.com>,
"yang.a.hong@intel.com" <yang.a.hong@intel.com>,
"harry.chang@intel.com" <harry.chang@intel.com>,
"gu.jian1@zte.com.cn" <gu.jian1@zte.com.cn>,
"shanjiangh@chinatelecom.cn" <shanjiangh@chinatelecom.cn>,
"zhangy.yun@chinatelecom.cn" <zhangy.yun@chinatelecom.cn>,
"lixingfu@huachentel.com" <lixingfu@huachentel.com>,
"wushuai@inspur.com" <wushuai@inspur.com>,
"yuyingxia@yxlink.com" <yuyingxia@yxlink.com>,
"fanchenggang@sunyainfo.com" <fanchenggang@sunyainfo.com>,
"davidfgao@tencent.com" <davidfgao@tencent.com>,
"liuzhong1@chinaunicom.cn" <liuzhong1@chinaunicom.cn>,
"zhaoyong11@huawei.com" <zhaoyong11@huawei.com>,
"oc@yunify.com" <oc@yunify.com>,
"jim@netgate.com" <jim@netgate.com>,
"hongjun.ni@intel.com" <hongjun.ni@intel.com>,
"deri@ntop.org" <deri@ntop.org>,
"fc@napatech.com" <fc@napatech.com>,
"arthur.su@lionic.com" <arthur.su@lionic.com>,
Raslan Darawsheh <rasland@mellanox.com>,
Yuval Avnery <yuvalav@mellanox.com>
Subject: Re: [dpdk-dev] [PATCH v1] app/test-regex: add RegEx test application
Date: Tue, 28 Jul 2020 04:29:50 +0000 [thread overview]
Message-ID: <AM6PR05MB5176DF7B858247C97FFD59CCDB730@AM6PR05MB5176.eurprd05.prod.outlook.com> (raw)
In-Reply-To: <3437146.xsi8y2SdMt@thomas>
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
>
> 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.
>
Will change, I did that since in all other cases the regex is after the compress.
> > 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.
>
Will fix.
> > +
> > +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.
>
Will remove.
> > +
> > +#
> > +# 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
>
Will remove this file.
> > +
> > +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".
>
Will remove this file.
Since this file use used to generate the data for the given
rule file and the rule file will be removed I will also remove this file.
> > --- /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.
>
Will remove.
> > + */
> > +
> > +#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.
>
Will fix.
> > +#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.
>
Will change.
> > +
> > +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.
>
Will remove.
> > + while ((opt = getopt_long(argc, argvopt, "",
> > + lgopts, &opt_idx)) != EOF) {
> > + switch (opt) {
>
> [...]
> > +#define MBUF_SIZE (1 << 14)
>
> Why this size?
> Add a comment?
>
No reason will change it to smaller size in any case.
> > +static void
> > +extbuf_free_cb(void *addr __rte_unused, void *fcb_opaque __rte_unused)
> > +{
> > +
> > +}
>
> Empty function can be dropped.
>
It must exist since I need to pass it to function call.
> [...]
> > +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
>
Will fix.
> > +
> > +The test outputs the performance, the results matching (rule id, position,
> len)
>
> length
>
> A list will look better.
>
Will change.
> > +for each job and also a list of matches (rule id, position , len) in absulote
>
> absolute
>
Will fix.
> > +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.
>
Sure.
> > +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.
>
Will change.
> > +Compiling the Tool
> > +------------------
> > +
> > +The ``dpdk-test-regex`` application depends on RegEx lib ``rte_regexdev``.
>
> Useless
>
Will remove,
> > +
> > +
> > +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.
>
Why? It has the python script that generate this input.
In any case I'm going to remove it. The generation of data will be done outside DPDK just like the
rule compilation.
>
> > +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.
>
I think the change are no major remarks that can't be fixed in a day.
can it target rc4?
next prev parent reply other threads:[~2020-07-28 4:29 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
2020-07-28 4:29 ` Ori Kam [this message]
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=AM6PR05MB5176DF7B858247C97FFD59CCDB730@AM6PR05MB5176.eurprd05.prod.outlook.com \
--to=orika@mellanox.com \
--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=pbhagavatula@marvell.com \
--cc=pkapoor@marvell.com \
--cc=rasland@mellanox.com \
--cc=shahafs@mellanox.com \
--cc=shanjiangh@chinatelecom.cn \
--cc=thomas@monjalon.net \
--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
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).