Re: [dpdk-dev] [PATCH v1 1/6] compress/zlib: add ZLIB PMD support

["Daly, Lee" <lee.daly@intel.com>]

Hi,
 thanks for the work, reviewed the PMD see comments below.

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Shally Verma
> Sent: Tuesday, May 15, 2018 11:32 AM
> To: De Lara Guarch, Pablo <pablo.de.lara.guarch@intel.com>
> Cc: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org;
> pathreay@caviumnetworks.com; Sunila Sahu
> <sunila.sahu@caviumnetworks.com>; Ashish Gupta
> <ashish.gupta@caviumnetworks.com>
> Subject: [dpdk-dev] [PATCH v1 1/6] compress/zlib: add ZLIB PMD support

<...>

> diff --git a/config/common_base b/config/common_base index
> 28557ed..537e9e4 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -586,6 +586,12 @@ CONFIG_RTE_COMPRESSDEV_TEST=n
> CONFIG_RTE_LIBRTE_PMD_ISAL=n
> 
>  #
> +# Compile PMD for ZLIB compression device #
> +CONFIG_RTE_LIBRTE_PMD_ZLIB=n
> CONFIG_RTE_LIBRTE_PMD_ZLIB_DEBUG=n
[Lee] DPDK is moving from the static debugging, I see you have implemented dynamic logging but have not yet used it in the PMD.
i.e using ZLIB_LOG_ERR/INFO() rather than ZLIB_PMD_LOG(INFO, "").
When you have removed the static debugging use, could you remove this flag to enable it please.

> +
> +#
>  # Compile generic event device library
>  #
>  CONFIG_RTE_LIBRTE_EVENTDEV=y
> diff --git a/drivers/compress/Makefile b/drivers/compress/Makefile index
> 592497f..1f159a5 100644
> --- a/drivers/compress/Makefile
> +++ b/drivers/compress/Makefile
> @@ -4,5 +4,6 @@
>  include $(RTE_SDK)/mk/rte.vars.mk
> 
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_ISAL) += isal
> +DIRS-$(CONFIG_RTE_LIBRTE_PMD_ZLIB) += zlib
> 
>  include $(RTE_SDK)/mk/rte.subdir.mk
> diff --git a/drivers/compress/zlib/Makefile b/drivers/compress/zlib/Makefile
> new file mode 100644 index 0000000..e613960
> --- /dev/null
> +++ b/drivers/compress/zlib/Makefile
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2018 Cavium
> +Networks
> +
> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +# library name
> +LIB = librte_pmd_zlib.a
> +
> +# build flags
> +CFLAGS += -O3
> +CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -DALLOW_EXPERIMENTAL_API
> +
> +# library version
> +LIBABIVER := 1
> +
> +# versioning export map
> +EXPORT_MAP := rte_pmd_zlib_version.map
> +
> +# external library dependencies
> +LDLIBS += -lrte_eal -lrte_mbuf -lrte_mempool -lrte_ring -lz LDLIBS +=
> +-lrte_compressdev LDLIBS += -lrte_bus_vdev
> +
> +# library source files
> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_ZLIB) += zlib_pmd.c
> +SRCS-$(CONFIG_RTE_LIBRTE_PMD_ZLIB) += zlib_pmd_ops.c
> +
> +# export include files
> +SYMLINK-y-include +=
> +
[Lee] Do you have a need for this in the future, not being used now.

> +include $(RTE_SDK)/mk/rte.lib.mk
> diff --git a/drivers/compress/zlib/rte_pmd_zlib_version.map
> b/drivers/compress/zlib/rte_pmd_zlib_version.map
> new file mode 100644
> index 0000000..33c1b97
> --- /dev/null
> +++ b/drivers/compress/zlib/rte_pmd_zlib_version.map
> @@ -0,0 +1,3 @@
> +EXPERIMENTAL {
[Lee] Would be better to replace EXPERMENTAL with 18.08 in this case.

> +	local: *;
> +};
> diff --git a/drivers/compress/zlib/zlib_pmd.c
> b/drivers/compress/zlib/zlib_pmd.c
> new file mode 100644
> index 0000000..bbf49f1
> --- /dev/null
> +++ b/drivers/compress/zlib/zlib_pmd.c
> @@ -0,0 +1,106 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017-2018 Cavium Networks  */
> +
> +#include <rte_common.h>
> +#include <rte_hexdump.h>
> +#include <rte_comp.h>
> +#include <rte_compressdev.h>
> +#include <rte_compressdev_pmd.h>
> +#include <rte_bus_vdev.h>
> +#include <rte_malloc.h>
> +#include <rte_cpuflags.h>
> +#include <rte_byteorder.h>
> +
> +#include <math.h>
> +#include <assert.h>
> +#include "zlib_pmd_private.h"
> +
> +static uint8_t compressdev_driver_id;
[Lee] The current API no longer has rte_compressdev->driver_id anymore therefor this variable will be unused.

> +int zlib_logtype_driver;
> +
> +static int zlib_remove(struct rte_vdev_device *vdev);
> +
> +static int
> +zlib_create(const char *name,
> +		struct rte_vdev_device *vdev,
> +		struct rte_compressdev_pmd_init_params *init_params) {
> +	struct rte_compressdev *dev;
> +	struct zlib_private *internals;
> +
> +	dev = rte_compressdev_pmd_create(name, &vdev->device,
> +			sizeof(struct zlib_private), init_params);
> +	if (dev == NULL) {
> +		ZLIB_LOG_ERR("driver %s: create failed", init_params-
> >name);
> +		return -ENODEV;
> +	}
> +
> +	dev->driver_id = compressdev_driver_id;
[Lee] See above comment.

> +	dev->dev_ops = rte_zlib_pmd_ops;
> +
> +	dev->feature_flags = 0;
> +	dev->feature_flags |= RTE_COMP_FF_SHAREABLE_PRIV_XFORM |
> +				RTE_COMP_FF_NONCOMPRESSED_BLOCKS |
> +				RTE_COMP_FF_ADLER32_CHECKSUM;

Thanks,
Lee.