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.gua...@intel.com>
> Sent: Monday, April 6, 2020 5:09 PM
>
> Hi David,
>
> > -----Original Message-----
> > From: Coyle, David <david.co...@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.co...@intel.com>
> > Signed-off-by: Mairtin o Loingsigh <mairtin.oloings...@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
