Re: [dpdk-dev] [PATCH v2 01/20] regex/mlx5: add RegEx PMD layer and mlx5 driver

[Thomas Monjalon <thomas@monjalon.net>]

12/07/2020 22:58, Ori Kam:
> From: Yuval Avnery <yuvalav@mellanox.com>
> 
> This commit introduce the RegEx poll mode drivers class, and
> adds Mellanox RegEx PMD.
> 
> Signed-off-by: Yuval Avnery <yuvalav@mellanox.com>
> Signed-off-by: Ori Kam <orika@mellanox.com>
> ---
> v2:
> * Add documantion.

First typo. Bad start for a doc update...

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -453,7 +453,9 @@ F: doc/guides/compressdevs/features/default.ini
>  RegEx API - EXPERIMENTAL
>  M: Ori Kam <orika@mellanox.com>
>  F: lib/librte_regexdev/
> +F: drivers/regex/

No, please don't.

>  F: doc/guides/prog_guide/regexdev.rst
> +F: doc/guides/regexdevs/features/default.ini

> +RegEx Drivers
> +------------------

Too much underlines

[...]
> +; Features of a default RegEx driver.
> +;
> +; This file defines the features that are valid for inclusion in
> +; the other driver files and also the order that they appear in
> +; the features table in the documentation. The feature description
> +; string should not exceed feature_str_len defined in conf.py.
> +;
> +[Features]
> +ARMv7                =

obsolete

> +ARMv8                =

Should be "Armv8" (with lower cases) or aarch64.

> +Power8               =

not very used

> +x86-32               =

not very used

> +x86-64               =

You can add simply "x86".

> +Usage doc            =
> +Design doc           =
> +Perf doc             =

I think you drop docs from the features list.

[...]
> +++ b/doc/guides/regexdevs/features_overview.rst
> @@ -0,0 +1,118 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright 2020 Mellanox Technologies, Ltd

Why 4 spaces? 3 are enough (and recommended in code style I think).

> +
> +Overview of RegEx Drivers Features
> +==================================
> +
> +This section explains the supported features that are listed in the table below.
> +
> +Cross buffer
> +  Support cross buffer detection.
> +
> +PCRE start anchor
> +  Support PCRE start anchor.
> +
> +PCRE atomic grouping
> +  Support PCRE atomic grouping.
> +
> +PCRE back reference
> +  Support PCRE back regerence.
> +
> +PCRE back tracking ctrl
> +  Support PCRE back tracking ctrl.
> +
> +PCRE call outs
> +  Support PCRE call outes.
> +
> +PCRE forward reference
> +  Support Forward reference.
> +
> +PCRE greedy
> +  Support PCRE greedy mode.
> +
> +PCRE match all
> +  Support PCRE match all.
> +
> +PCRE match as end
> +  Support match as end.
> +
> +PCRE match point rst
> +  Support PCRE match point reset directive.
> +
> +PCRE New line conventions
> +  Support new line conventions.
> +
> +PCRE new line SEQ
> +  Support new line sequence.
> +
> +PCRE look around
> +  Support PCRE look around.
> +
> +PCRE possessive qualifiers
> +  Support PCRE possessive qualifiers.
> +
> +PCRE subroutine references
> +  Support PCRE subroutine references.
> +
> +PCRE UTF 8
> +  Support UTF-8.
> +
> +PCRE UTF 16
> +  Support UTF-16.
> +
> +PCRE UTF 32
> +  Support UTF-32.
> +
> +PCRE word boundary
> +  Support word boundaries.
> +
> +Run time compilation
> +  Support compilation during run time.
> +

All these features are not in features.ini

> +.. note::
> +
> +   Most of the features capabilities should be provided by the drivers via the
> +   next vDPA operations: ``get_features`` and ``get_protocol_features``.

Now I understand why we don't have doc reviews:
even the author is not reading its own doc!

> +
> +
> +References
> +==========
> +
> +  * `PCRE: PCRE patteren man page <https://www.pcre.org/original/doc/html/pcrepattern.html>`_

Typo

> +++ b/doc/guides/regexdevs/index.rst
> @@ -0,0 +1,15 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright 2020 Mellanox Technologies, Ltd

3 spaces are enough

> +++ b/doc/guides/regexdevs/mlx5.rst
> @@ -0,0 +1,95 @@
> +..  SPDX-License-Identifier: BSD-3-Clause
> +    Copyright 2020 Mellanox Technologies, Ltd

3 spaces are enough

> +Design
> +------
> +
> +This PMD is configuring the RegEx HW engine.
> +For the PMD to work, the application must supply
> +a precompiled rule file in rof2 format.

A link to rof2?

> +
> +The PMD can use libibverbs and libmlx5 to access the device firmware
> +or directly the hardware components.

can? or always?
s/can use/uses/

> +There are different levels of objects and bypassing abilities
> +to get the best performances:
> +
> +- Verbs is a complete high-level generic API
> +- Direct Verbs is a device-specific API
> +- DevX allows to access firmware objects
> +- Direct Rules manages flow steering at low-level hardware layer

How flow steering is related to RegEx engine?

> +
> +Enabling librte_pmd_mlx5_regex causes DPDK applications to be linked against
> +libibverbs.
> +
> +A Mellanox mlx5 PCI device can be probed by either net/mlx5 driver or regex/mlx5
> +driver but not in parallel. Hence, the user should decide the driver by dissabling

disabling

> +the net device using ``CONFIG_RTE_LIBRTE_MLX5_PMD``.

The meson disabling option is missing.

Isn't is possible to decide at runtime with devargs?

> +
> +Supported NICs
> +--------------
> +
> +* Mellanox\ |reg| BlueField 2 SmartNIC
> +
> +Prerequisites
> +-------------
> +
> +- Mellanox OFED version: **5.0**
> +  see :doc:`../../nics/mlx5` guide for more Mellanox OFED details.

Which upstream rdma-core version?

> +- Enable the RegEx caps using system call from the BlueField 2.
> +  Contact Mellanox support for detail explanation.

Why not giving the details here?
Or link to an external doc?

> +
> +Compilation options
> +~~~~~~~~~~~~~~~~~~~
> +
> +These options can be modified in the ``.config`` file.
> +
> +- ``CONFIG_RTE_LIBRTE_MLX5_REGEX_PMD`` (default **n**)
> +
> +  Toggle compilation of librte_pmd_mlx5 itself.
> +
> +- ``CONFIG_RTE_IBVERBS_LINK_DLOPEN`` (default **n**)
> +
> +  Build PMD with additional code to make it loadable without hard
> +  dependencies on **libibverbs** nor **libmlx5**, which may not be installed
> +  on the target system.
> +
> +  In this mode, their presence is still required for it to run properly,
> +  however their absence won't prevent a DPDK application from starting (with
> +  ``CONFIG_RTE_BUILD_SHARED_LIB`` disabled) and they won't show up as
> +  missing with ``ldd(1)``.
> +
> +  It works by moving these dependencies to a purpose-built rdma-core "glue"
> +  plug-in which must either be installed in a directory whose name is based
> +  on ``CONFIG_RTE_EAL_PMD_PATH`` suffixed with ``-glue`` if set, or in a
> +  standard location for the dynamic linker (e.g. ``/lib``) if left to the
> +  default empty string (``""``).
> +
> +  This option has no performance impact.
> +
> +- ``CONFIG_RTE_IBVERBS_LINK_STATIC`` (default **n**)
> +
> +  Embed static flavor of the dependencies **libibverbs** and **libmlx5**
> +  in the PMD shared library or the executable static binary.

These options are not specific to this driver.
We should link to the original explanations in the net PMD.

By the way it is missing meson explanations.

[...]
> --- /dev/null
> +++ b/doc/guides/regexdevs/overview_feature_table.txt

This generated file should not be committed in the repo.

You are missing an update in doc/guides/conf.py and .gitignore.

[...]
> +* **Added the RegEx Library, a generic RegEx service library.**

Redundant with previous lib addition.

> +
> +  Added Mellanox MLX5 RegEx PMD driver, which implements the RegEx library
> +  and allows to offload RegEx searches.
> +

Please prefer this rebased change:

   Added the RegEx library which provides an API for offload of regular
   expressions search operations to hardware or software accelerator devices.
 
+  Added Mellanox RegEx PMD, allowing to offload RegEx searches.
+

> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -24,5 +24,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_EVENTDEV) += event
>  DEPDIRS-event := common bus mempool net crypto
>  DIRS-$(CONFIG_RTE_LIBRTE_RAWDEV) += raw
>  DEPDIRS-raw := common bus mempool net event
> +DIRS-$(CONFIG_RTE_LIBRTE_REGEXDEV) += regex
> +DEPDIRS-regex := common

Please keep same order everywhere: regex between compressdev and eventdev.
(yes vdpa should have been just after net)

[...]
> --- a/drivers/meson.build
> +++ b/drivers/meson.build
> @@ -11,7 +11,8 @@ dpdk_driver_classes = ['common',
>  	       'compress', # depends on common, bus, mempool.
>  	       'vdpa',    # depends on common, bus and mempool.
>  	       'event',   # depends on common, bus, mempool and net.
> -	       'baseband'] # depends on common and bus.
> +	       'baseband', # depends on common and bus.
> +	       'regex'] # depends on common, bus, regexdev.

Again, please add after compress.

[...]
> +if get_option('buildtype').contains('debug')
> +	cflags += [ '-pedantic', '-DPEDANTIC' ]
> +else
> +	cflags += [ '-UPEDANTIC' ]
> +endif

Please let's stop with pedantic now.

By the way, it does not compile:
	drivers/regex/mlx5/mlx5_regex.c:6: error:
	ISO C forbids an empty translation unit [-Werror=pedantic]