Hi Lee
>-----Original Message----- >From: Daly, Lee [mailto:lee.d...@intel.com] >Sent: 15 June 2018 16:39 >To: Verma, Shally <shally.ve...@cavium.com> >Cc: Trahe, Fiona <fiona.tr...@intel.com>; dev@dpdk.org; >pathr...@caviumnetworks.com; Sahu, Sunila <sunila.s...@cavium.com>; >Gupta, Ashish <ashish.gu...@cavium.com>; De Lara Guarch, Pablo ><pablo.de.lara.gua...@intel.com> >Subject: RE: [dpdk-dev] [PATCH v1 2/6] compress/zlib: add device setup PMD ops > >External Email > >Hi Shally, >Comments inline. > > >> -----Original Message----- >> From: dev [mailto:dev-boun...@dpdk.org] On Behalf Of Shally Verma >> Sent: Tuesday, May 15, 2018 11:32 AM >> To: De Lara Guarch, Pablo <pablo.de.lara.gua...@intel.com> >> Cc: Trahe, Fiona <fiona.tr...@intel.com>; dev@dpdk.org; >> pathr...@caviumnetworks.com; Sunila Sahu >> <sunila.s...@caviumnetworks.com>; Ashish Gupta >> <ashish.gu...@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. > [Shally] Per my understanding, SHAREABLE_PRIV_XFORM here means xform is shareable by all ops in one single enqueue_burst() but not across devices or qps by multiple threads in parallel. Does your implementation support such usage of shareable priv_xforms? Thanks for review. Shally >>+ .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.
