From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 18F091D6F8 for ; Fri, 15 Jun 2018 13:09:03 +0200 (CEST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Jun 2018 04:09:03 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,226,1526367600"; d="scan'208";a="50140071" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by orsmga006.jf.intel.com with ESMTP; 15 Jun 2018 04:09:01 -0700 Received: from irsmsx106.ger.corp.intel.com ([169.254.8.80]) by IRSMSX154.ger.corp.intel.com ([169.254.12.250]) with mapi id 14.03.0319.002; Fri, 15 Jun 2018 12:09:00 +0100 From: "Daly, Lee" To: 'Shally Verma' CC: "Trahe, Fiona" , "dev@dpdk.org" , "pathreay@caviumnetworks.com" , Sunila Sahu , Ashish Gupta , "De Lara Guarch, Pablo" Thread-Topic: [dpdk-dev] [PATCH v1 2/6] compress/zlib: add device setup PMD ops Thread-Index: AQHT7Dgwqolm9e5EzUSvJW3z3OQoI6RgIh7Q Date: Fri, 15 Jun 2018 11:08:59 +0000 Message-ID: References: <1526380346-7386-1-git-send-email-shally.verma@caviumnetworks.com> <1526380346-7386-3-git-send-email-shally.verma@caviumnetworks.com> In-Reply-To: <1526380346-7386-3-git-send-email-shally.verma@caviumnetworks.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.200.100 dlp-reaction: no-action x-ctpclassification: CTP_NT x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiODIyM2YwYTEtNWIzMi00NTJlLWJkMmEtNTEwZTc3YjlmNDQ1IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJRVUNKeWpSQ01wSnNocjZnU2VHcExNK0Z3VHczSVVTZnpSNkc1ZG1VeUN1Nngwa1FKekNCQm9DeDVHcXBDZVJyIn0= x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v1 2/6] compress/zlib: add device setup PMD ops X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 15 Jun 2018 11:09:04 -0000 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 > Cc: Trahe, Fiona ; dev@dpdk.org; > pathreay@caviumnetworks.com; Sunila Sahu > ; Ashish Gupta > > Subject: [dpdk-dev] [PATCH v1 2/6] compress/zlib: add device setup PMD > ops >=20 >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 @@=20 >+/* SPDX-License-Identifier: BSD-3-Clause >+ * Copyright(c) 2018 Cavium Networks >+ */ >+ >+#include >+ >+#include >+#include >+#include >+ >+#include "zlib_pmd_private.h" >+ >+static const struct rte_compressdev_capabilities zlib_pmd_capabilities[] = =3D { >+ { /* Deflate */ >+ .algo =3D RTE_COMP_ALGO_DEFLATE, >+ .comp_feature_flags =3D RTE_COMP_FF_SHAREABLE_PRIV_XFORM, [Lee] The priv_xform structure in this case is not shareable, as it contain= s 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 m= eans it is not shareable across queue pairs or devices. >+ .window_size =3D { >+ .min =3D 8, >+ .max =3D 15, >+ .increment =3D 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 =3D dev->data->dev_private; > + snprintf(internals->mp_name, RTE_MEMPOOL_NAMESIZE, > + "stream_mp_%u", dev->data->dev_id); > + mp =3D 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 =3D=3D 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 =3D dev->data->dev_private; > + struct rte_mempool *mp =3D 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 i= ts memory, especially since the start function does nothing here. i.e. if t= he user stops device then starts again, memory needed has been free'd but n= ot 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 > +#include > +#include > +#include > + > +#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.