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

["Coyle, David" <david.coyle@intel.com>]

Hi Pablo
Thank you for reviewing and the comments - see below for resolutions.
The changes will be available in v3 shortly

David

> -----Original Message-----
> From: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Sent: Monday, April 6, 2020 5:09 PM
> 
> Hi David,
> 
> > -----Original Message-----
> > From: Coyle, David <david.coyle@intel.com>
> > Sent: Friday, April 3, 2020 5:37 PM
> >
> > 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.

[DC] That is true, so yes this is now enabled by default
> 
> ...
> 
> > +++
> 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.

[DC] Fixed
> 
> > +
> > +	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.

[DC] Most of the includes weren't needed... these have been tidied up now
> 
> 
> > +
> > +#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.

[DC] Again includes have been tidied up... left common, mbuf, mempool and crypto as these are referenced directly in this file
rte_comp.h has been removed as he have removed compression completely from this patchset
> 
> > +
> 
> ...
> 
> > +__rte_experimental
> > +static inline void
> 
> This is a public API, so I'd say this shouldn't be inline, right?

[DC] Lots of example of public APIs being inline, so no issue here
> 
> > +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.

[DC] Includes tidied up, only rawdev and multi_fn (in quotes) left now