Re: [dpdk-dev] [PATCH v1 2/6] compress/zlib: add device setup PMD ops

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

Hi Shally,
Comments inline.


> -----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 2/6] compress/zlib: add device setup PMD
> ops
> 
>diff --git a/drivers/compress/zlib/zlib_pmd_ops.c b/drivers/compress/zlib/zlib_pmd_ops.c
>new file mode 100644
>index 0000000..0bd42f3
>--- /dev/null
>+++ b/drivers/compress/zlib/zlib_pmd_ops.c
>@@ -0,0 +1,238 @@ 
>+/* SPDX-License-Identifier: BSD-3-Clause
>+ * Copyright(c) 2018 Cavium Networks
>+ */
>+
>+#include <string.h>
>+
>+#include <rte_common.h>
>+#include <rte_malloc.h>
>+#include <rte_compressdev_pmd.h>
>+
>+#include "zlib_pmd_private.h"
>+
>+static const struct rte_compressdev_capabilities zlib_pmd_capabilities[] = {
>+	{   /* Deflate */
>+		.algo = RTE_COMP_ALGO_DEFLATE,
>+		.comp_feature_flags = RTE_COMP_FF_SHAREABLE_PRIV_XFORM,
[Lee] The priv_xform structure in this case is not shareable, as it contains your zlib_stream structure, which contains zlibs own zstream struct. This is not read only, the contents of this zstream will be written to, which means it is not shareable across queue pairs or devices.

>+		.window_size = {
>+			.min = 8,
>+			.max = 15,
>+			.increment = 2
>+		},
>+	},
>+
>+	RTE_COMP_END_OF_CAPABILITIES_LIST()
>+
>+};
> +/** Configure device */
> +static int
> +zlib_pmd_config(struct rte_compressdev *dev,
> +		struct rte_compressdev_config *config) {
> +	struct rte_mempool *mp;
> +
> +	struct zlib_private *internals = dev->data->dev_private;
> +	snprintf(internals->mp_name, RTE_MEMPOOL_NAMESIZE,
> +			"stream_mp_%u", dev->data->dev_id);
> +	mp = rte_mempool_create(internals->mp_name,
> +			config->max_nb_priv_xforms + config-
> >max_nb_streams,
> +			sizeof(struct zlib_priv_xform),
> +			0, 0, NULL, NULL, NULL,
> +			NULL, config->socket_id,
> +			0);
[Lee] Could you add a mempool_lookup here to ensure its not already created please.

> +	if (mp == NULL) {
> +		ZLIB_LOG_ERR("Cannot create private xform pool on socket
> %d\n",
> +				config->socket_id);
> +		return -ENOMEM;
> +	}
> +	return 0;
> +}
> +
> +/** Start device */
> +static int
> +zlib_pmd_start(__rte_unused struct rte_compressdev *dev) {
> +	return 0;
> +}
> +
> +/** Stop device */
> +static void
> +zlib_pmd_stop(struct rte_compressdev *dev) {
> +	struct zlib_private *internals = dev->data->dev_private;
> +	struct rte_mempool *mp = rte_mempool_lookup(internals-
> >mp_name);
> +	rte_mempool_free(mp);
> +}
> +
[Lee] I believe it would be better to have the freeing functionality in the pmd_close function, as a user may want to stop a device, without freeing its memory, especially since the start function does nothing here. i.e. if the user stops device then starts again, memory needed has been free'd but not realloc'ed. Hope this makes sense.

> +/** Close device */
> +static int
> +zlib_pmd_close(__rte_unused struct rte_compressdev *dev) {
> +	return 0;
> +}

<...>
> diff --git a/drivers/compress/zlib/zlib_pmd_private.h
> b/drivers/compress/zlib/zlib_pmd_private.h
> new file mode 100644
> index 0000000..d29dc59
> --- /dev/null
> +++ b/drivers/compress/zlib/zlib_pmd_private.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2017-2018 Cavium Networks  */
> +
> +#ifndef _RTE_ZLIB_PMD_PRIVATE_H_
> +#define _RTE_ZLIB_PMD_PRIVATE_H_
> +
> +#include <zlib.h>
> +#include <rte_comp.h>
> +#include <rte_compressdev.h>
> +#include <rte_compressdev_pmd.h>
> +
> +#define COMPRESSDEV_NAME_ZLIB_PMD	compress_zlib
> +/**< ZLIB PMD device name */
> +
> +#define ZLIB_PMD_MAX_NB_QUEUE_PAIRS	1
> +/**< ZLIB PMD specified queue pairs */
[Lee] Doesn't look like this macro is being used anywhere, may be better to remove this altogether as there is no limit in software for queue pairs.

> +
> +#define DEF_MEM_LEVEL			8
> +
> +int zlib_logtype_driver;
> +#define ZLIB_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, zlib_logtype_driver, "%s(): "fmt "\n", \
> +			__func__, ##args)
> +
> +#define ZLIB_LOG_INFO(fmt, args...) \
> +	ZLIB_LOG(INFO, fmt, ## args)
> +#define ZLIB_LOG_ERR(fmt, args...) \
> +	ZLIB_LOG(ERR, fmt, ## args)
> +#define ZLIB_LOG_WARN(fmt, args...) \
> +	ZLIB_LOG(WARNING, fmt, ## args)
[Lee] See previous comments re/ static logging.

> +
> +struct zlib_private {
> +	uint32_t max_nb_queue_pairs;
> +	char mp_name[RTE_MEMPOOL_NAMESIZE];
> +};
> +
Thanks,
Lee.