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

["Verma, Shally" <Shally.Verma@cavium.com>]

Hi Lee


>-----Original Message-----
>From: Daly, Lee [mailto:lee.daly@intel.com]
>Sent: 15 June 2018 16:39
>To: Verma, Shally <Shally.Verma@cavium.com>
>Cc: Trahe, Fiona <fiona.trahe@intel.com>; dev@dpdk.org; pathreay@caviumnetworks.com; Sahu, Sunila <Sunila.Sahu@cavium.com>;
>Gupta, Ashish <Ashish.Gupta@cavium.com>; De Lara Guarch, Pablo <pablo.de.lara.guarch@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-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.
>
[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.