Re: [dpdk-dev] [PATCH v2 1/4] raw/common: add multi-function interface

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi David,

> -----Original Message-----
> From: Coyle, David <david.coyle@intel.com>
> Sent: Friday, April 3, 2020 5:37 PM
> To: dev@dpdk.org
> Cc: Doherty, Declan <declan.doherty@intel.com>; Trahe, Fiona
> <fiona.trahe@intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch@intel.com>; Ryan, Brendan <brendan.ryan@intel.com>;
> shreyansh.jain@nxp.com; hemant.agrawal@nxp.com; Coyle, David
> <david.coyle@intel.com>; O'loingsigh, Mairtin <mairtin.oloingsigh@intel.com>
> Subject: [PATCH v2 1/4] raw/common: add multi-function interface
> 
> The multi-function interface provides a flexible and extensible way of combining
> one or more packet processing functions into a single operation. The interface
> can be used by applications to send the combined operations to a optimized
> software or hardware accelerator via a raw device.
> 
> Signed-off-by: David Coyle <david.coyle@intel.com>
> Signed-off-by: Mairtin o Loingsigh <mairtin.oloingsigh@intel.com>
> ---
> 
> In particular, looking for feedback on the meson script changes that were
> required to build the drivers/raw/common/multi_fn directory. Thank you.
> 
>  config/common_base                            |   5 +
>  drivers/meson.build                           |   5 +
>  drivers/raw/Makefile                          |   1 +
>  drivers/raw/common/Makefile                   |   8 +
>  drivers/raw/common/meson.build                |   7 +
>  drivers/raw/common/multi_fn/Makefile          |  27 ++
>  drivers/raw/common/multi_fn/meson.build       |   9 +
>  .../multi_fn/rte_common_multi_fn_version.map  |  11 +
>  drivers/raw/common/multi_fn/rte_multi_fn.c    | 166 +++++++++
>  drivers/raw/common/multi_fn/rte_multi_fn.h    | 350 ++++++++++++++++++
>  .../raw/common/multi_fn/rte_multi_fn_driver.h |  55 +++
>  meson.build                                   |   4 +
>  mk/rte.app.mk                                 |   1 +
>  13 files changed, 649 insertions(+)
>  create mode 100644 drivers/raw/common/Makefile  create mode 100644
> drivers/raw/common/meson.build  create mode 100644
> drivers/raw/common/multi_fn/Makefile
>  create mode 100644 drivers/raw/common/multi_fn/meson.build
>  create mode 100644
> drivers/raw/common/multi_fn/rte_common_multi_fn_version.map
>  create mode 100644 drivers/raw/common/multi_fn/rte_multi_fn.c
>  create mode 100644 drivers/raw/common/multi_fn/rte_multi_fn.h
>  create mode 100644 drivers/raw/common/multi_fn/rte_multi_fn_driver.h
> 
> diff --git a/config/common_base b/config/common_base index
> c31175f9d..4f004968b 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -818,6 +818,11 @@
> CONFIG_RTE_LIBRTE_PMD_OCTEONTX2_EP_RAWDEV=y
>  #
>  CONFIG_RTE_LIBRTE_PMD_NTB_RAWDEV=y
> 
> +#
> +# Compile multi-fn raw device interface #
> +CONFIG_RTE_LIBRTE_MULTI_FN_COMMON=n

This can be enabled by default, right? It doesn't have any external dependency.

...

> +++ b/drivers/raw/common/multi_fn/rte_common_multi_fn_version.map
> @@ -0,0 +1,11 @@
> +EXPERIMENTAL {
> +	global:
> +
> +	rte_multi_fn_session_create;
> +	rte_multi_fn_session_destroy;
> +	rte_multi_fn_op_pool_create;
> +	rte_multi_fn_op_bulk_alloc;
> +	rte_multi_fn_op_free;

This list should be sorted alphabetically.

> +
> +	local: *;
> +};
> diff --git a/drivers/raw/common/multi_fn/rte_multi_fn.c
> b/drivers/raw/common/multi_fn/rte_multi_fn.c
> new file mode 100644
> index 000000000..4f8e7fd94
> --- /dev/null
> +++ b/drivers/raw/common/multi_fn/rte_multi_fn.c
> @@ -0,0 +1,166 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#include <ctype.h>
> +#include <stdio.h>

...

> +#include <rte_rawdev.h>

A bunch of these includes are not needed.
From what I could see, only <inttypes.h>, <rte_log.h>, <rte_common.h>,
<rte_rawdev.h> and <rte_mempool.h> are needed, apart from the two below.


> +
> +#include "rte_multi_fn_driver.h"
> +#include "rte_multi_fn.h"

...

> +++ b/drivers/raw/common/multi_fn/rte_multi_fn.h
> @@ -0,0 +1,350 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#ifndef _RTE_MULTI_FN_H_
> +#define _RTE_MULTI_FN_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_compat.h>
> +#include <rte_common.h>
> +#include <rte_mbuf.h>
> +#include <rte_memory.h>
> +#include <rte_mempool.h>
> +#include <rte_comp.h>
> +#include <rte_crypto.h>
> +#include <rte_rawdev.h>

Only <rte_comp.h> and <rte_crypto.h> are needed.

> +

...

> +__rte_experimental
> +static inline void

This is a public API, so I'd say this shouldn't be inline, right?

> +rte_multi_fn_op_free(struct rte_multi_fn_op *op) {
> +	if (op != NULL && op->mempool != NULL)
> +		rte_mempool_put(op->mempool, op);
> +}
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_MULTI_FN_H_ */
> diff --git a/drivers/raw/common/multi_fn/rte_multi_fn_driver.h
> b/drivers/raw/common/multi_fn/rte_multi_fn_driver.h
> new file mode 100644
> index 000000000..7e1e57fa3
> --- /dev/null
> +++ b/drivers/raw/common/multi_fn/rte_multi_fn_driver.h
> @@ -0,0 +1,55 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation.
> + */
> +
> +#ifndef _RTE_MULTI_FN_DRIVER_H_
> +#define _RTE_MULTI_FN_DRIVER_H_
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include <rte_compat.h>
> +#include <rte_common.h>
> +#include <rte_rawdev.h>
> +#include <rte_multi_fn.h>
> +

Only <rte_rawdev.h> and <rte_multi_fn.h> are needed.
Actually, since rte_multi_fn.h is in the same folder, you can use double quotes.