From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id B758A34F3 for ; Wed, 7 Nov 2018 11:14:44 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 07 Nov 2018 02:14:43 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,475,1534834800"; d="scan'208";a="277815509" Received: from fmsmsx106.amr.corp.intel.com ([10.18.124.204]) by fmsmga005.fm.intel.com with ESMTP; 07 Nov 2018 02:14:43 -0800 Received: from fmsmsx155.amr.corp.intel.com (10.18.116.71) by FMSMSX106.amr.corp.intel.com (10.18.124.204) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 7 Nov 2018 02:14:43 -0800 Received: from lcsmsx154.ger.corp.intel.com (10.186.165.229) by FMSMSX155.amr.corp.intel.com (10.18.116.71) with Microsoft SMTP Server (TLS) id 14.3.408.0; Wed, 7 Nov 2018 02:14:43 -0800 Received: from hasmsx112.ger.corp.intel.com ([169.254.11.97]) by LCSMSX154.ger.corp.intel.com ([169.254.7.79]) with mapi id 14.03.0415.000; Wed, 7 Nov 2018 12:14:40 +0200 From: "Jozwiak, TomaszX" To: "Verma, Shally" , "dev@dpdk.org" , "Trahe, Fiona" , "akhil.goyal@nxp.com" Thread-Topic: [PATCH v2 2/3] app/compress-perf: add performance measurement Thread-Index: AQHUcpCjZhIemjb16Uuf+VxWSS/SKaVAxD6AgAGswOCAAFWMAIABWJ4A Date: Wed, 7 Nov 2018 10:14:39 +0000 Message-ID: References: <1538400427-20164-1-git-send-email-tomaszx.jozwiak@intel.com> <1541151842-8746-1-git-send-email-tomaszx.jozwiak@intel.com> <1541151842-8746-3-git-send-email-tomaszx.jozwiak@intel.com> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.104.14.176] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH v2 2/3] app/compress-perf: add performance measurement 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: Wed, 07 Nov 2018 10:14:45 -0000 > -----Original Message----- > From: Verma, Shally [mailto:Shally.Verma@cavium.com] > Sent: Tuesday, November 6, 2018 4:37 PM > To: Jozwiak, TomaszX ; dev@dpdk.org; Trahe, > Fiona ; akhil.goyal@nxp.com > Subject: RE: [PATCH v2 2/3] app/compress-perf: add performance > measurement >=20 >=20 >=20 > >-----Original Message----- > >From: Jozwiak, TomaszX > >Sent: 06 November 2018 14:19 > >To: Verma, Shally ; dev@dpdk.org; Trahe, > Fiona > >; akhil.goyal@nxp.com > >Subject: RE: [PATCH v2 2/3] app/compress-perf: add performance > >measurement > > > >External Email > > > >> -----Original Message----- > >> From: Verma, Shally [mailto:Shally.Verma@cavium.com] > >> Sent: Monday, November 5, 2018 9:57 AM > >> To: Jozwiak, TomaszX ; dev@dpdk.org; > >> Trahe, Fiona ; akhil.goyal@nxp.com > >> Subject: RE: [PATCH v2 2/3] app/compress-perf: add performance > >> measurement > >> > >> > >> > >> >-----Original Message----- > >> >From: Tomasz Jozwiak > >> >Sent: 02 November 2018 15:14 > >> >To: dev@dpdk.org; fiona.trahe@intel.com; tomaszx.jozwiak@intel.com; > >> >Verma, Shally ; akhil.goyal@nxp.com > >> >Subject: [PATCH v2 2/3] app/compress-perf: add performance > >> measurement > >> > > >> >External Email > >> > > >> >Added performance measurement part into compression perf. test. > >> > > >> >Signed-off-by: De Lara Guarch, Pablo > >> > > >> >Signed-off-by: Tomasz Jozwiak > >> >--- > >> > app/test-compress-perf/comp_perf_options_parse.c | 8 +- > >> > app/test-compress-perf/main.c | 886 > >> ++++++++++++++++++++++- > >> > 2 files changed, 883 insertions(+), 11 deletions(-) > >> > > >> >diff --git a/app/test-compress-perf/comp_perf_options_parse.c > >> >b/app/test-compress-perf/comp_perf_options_parse.c > >> >index bef4d2f..e5da3ad 100644 > >> >--- a/app/test-compress-perf/comp_perf_options_parse.c > >> >+++ b/app/test-compress-perf/comp_perf_options_parse.c > >> >@@ -431,10 +431,6 @@ parse_huffman_enc(struct comp_test_data > >> *test_data, const char *arg) > >> > { > >> > "fixed", > >> > RTE_COMP_HUFFMAN_FIXED > >> >- }, > >> >- { > >> >- "dynamic", > >> >- RTE_COMP_HUFFMAN_DYNAMIC > >> > } > >> > }; > >> > > >> >@@ -569,9 +565,9 @@ comp_perf_options_default(struct > comp_test_data > >> *test_data) > >> > test_data->seg_sz =3D 2048; > >> > test_data->burst_sz =3D 32; > >> > test_data->pool_sz =3D 8192; > >> >- test_data->max_sgl_segs =3D UINT16_MAX; > >> >+ test_data->max_sgl_segs =3D 16; > >> > test_data->num_iter =3D 10000; > >> >- test_data->huffman_enc =3D RTE_COMP_HUFFMAN_DYNAMIC; > >> >+ test_data->huffman_enc =3D RTE_COMP_HUFFMAN_FIXED; > >> > test_data->test_op =3D COMPRESS_DECOMPRESS; > >> > test_data->window_sz =3D -1; > >> > test_data->level.min =3D 1; > >> >diff --git a/app/test-compress-perf/main.c > >> >b/app/test-compress-perf/main.c index f52b98d..e3f4bf6 100644 > >> >--- a/app/test-compress-perf/main.c > >> >+++ b/app/test-compress-perf/main.c > >> >@@ -5,14 +5,728 @@ > >> > #include > >> > #include > >> > #include > >> >+#include > >> > #include > >> > > >> > #include "comp_perf_options.h" > >> > > >> >+#define NUM_MAX_XFORMS 16 > >> >+#define NUM_MAX_INFLIGHT_OPS 512 > >> >+#define EXPANSE_RATIO 1.05 > >> >+#define MIN_ISAL_SIZE 8 > >> Can we avoid ISAL specific naming ? > > > >TJ: yes true :) will be fixed in V3 > > > > > > > > > >> >+ > >> >+#define DIV_CEIL(a, b) ((a) / (b) + ((a) % (b) !=3D 0)) > >> >+ > >> >+/* Cleanup state machine */ > >> >+static enum cleanup_st { > >> >+ ST_CLEAR =3D 0, > >> >+ ST_TEST_DATA, > >> >+ ST_COMPDEV, > >> >+ ST_INPUT_DATA, > >> >+ ST_MEMORY_ALLOC, > >> >+ ST_PREPARE_BUF, > >> >+ ST_DURING_TEST > >> >+} cleanup =3D ST_CLEAR; > >> >+ > >> >+static int > >> >+param_range_check(uint16_t size, const struct rte_param_log2_range > >> >+*range) { > >> >+ unsigned int next_size; > >> >+ > >> >+ /* Check lower/upper bounds */ > >> >+ if (size < range->min) > >> >+ return -1; > >> >+ > >> >+ if (size > range->max) > >> >+ return -1; > >> >+ > >> >+ /* If range is actually only one value, size is correct */ > >> >+ if (range->increment =3D=3D 0) > >> >+ return 0; > >> >+ > >> >+ /* Check if value is one of the supported sizes */ > >> >+ for (next_size =3D range->min; next_size <=3D range->max; > >> >+ next_size +=3D range->increment) > >> >+ if (size =3D=3D next_size) > >> >+ return 0; > >> >+ > >> >+ return -1; > >> >+} > >> >+ > >> >+static int > >> >+comp_perf_check_capabilities(struct comp_test_data *test_data) { > >> >+ const struct rte_compressdev_capabilities *cap; > >> >+ > >> >+ cap =3D rte_compressdev_capability_get(test_data->cdev_id, > >> >+ RTE_COMP_ALGO_DEFLATE); > >> >+ > >> >+ if (cap =3D=3D NULL) { > >> >+ RTE_LOG(ERR, USER1, > >> >+ "Compress device does not support DEFLATE\n")= ; > >> >+ return -1; > >> >+ } > >> >+ > >> >+ uint64_t comp_flags =3D cap->comp_feature_flags; > >> >+ > >> >+ /* Huffman enconding */ > >> >+ if (test_data->huffman_enc =3D=3D RTE_COMP_HUFFMAN_FIXED && > >> >+ (comp_flags & RTE_COMP_FF_HUFFMAN_FIXED) =3D= =3D 0) { > >> >+ RTE_LOG(ERR, USER1, > >> >+ "Compress device does not supported Fixed Huf= fman\n"); > >> >+ return -1; > >> >+ } > >> >+ > >> >+ if (test_data->huffman_enc =3D=3D RTE_COMP_HUFFMAN_DYNAMIC > && > >> >+ (comp_flags & RTE_COMP_FF_HUFFMAN_DYNAMIC) = =3D=3D 0) > { > >> >+ RTE_LOG(ERR, USER1, > >> >+ "Compress device does not supported Dynamic > Huffman\n"); > >> >+ return -1; > >> >+ } > >> >+ > >> >+ /* Window size */ > >> >+ if (test_data->window_sz !=3D -1) { > >> >+ if (param_range_check(test_data->window_sz, &cap- > >> >window_size) > >> >+ < 0) { > >> >+ RTE_LOG(ERR, USER1, > >> >+ "Compress device does not support " > >> >+ "this window size\n"); > >> >+ return -1; > >> >+ } > >> >+ } else > >> >+ /* Set window size to PMD maximum if none was specifi= ed */ > >> >+ test_data->window_sz =3D cap->window_size.max; > >> >+ > >> >+ /* Check if chained mbufs is supported */ > >> >+ if (test_data->max_sgl_segs > 1 && > >> >+ (comp_flags & RTE_COMP_FF_OOP_SGL_IN_SGL_OUT)= =3D=3D > 0) { > >> >+ RTE_LOG(INFO, USER1, "Compress device does not suppor= t " > >> >+ "chained mbufs. Max SGL segments set = to 1\n"); > >> >+ test_data->max_sgl_segs =3D 1; > >> >+ } > >> >+ > >> >+ /* Level 0 support */ > >> >+ if (test_data->level.min =3D=3D 0 && > >> >+ (comp_flags & > >> >+ RTE_COMP_FF_NONCOMPRESSED_BLOCKS) =3D=3D > >> 0) { > >> >+ RTE_LOG(ERR, USER1, "Compress device does not support= " > >> >+ "level 0 (no compression)\n"); > >> >+ return -1; > >> >+ } > >> >+ > >> >+ return 0; > >> >+} > >> >+ > >> >+static int > >> >+comp_perf_allocate_memory(struct comp_test_data *test_data) { > >> >+ /* Number of segments for input and output > >> >+ * (compression and decompression) > >> >+ */ > >> >+ uint32_t total_segs =3D DIV_CEIL(test_data->input_data_sz, > >> >+ test_data->seg_sz); > >> >+ test_data->comp_buf_pool =3D > >> rte_pktmbuf_pool_create("comp_buf_pool", > >> >+ total_segs, > >> >+ 0, 0, test_data->seg_sz + RTE_PKTMBUF= _HEADROOM, > >> >+ rte_socket_id()); > >> >+ if (test_data->comp_buf_pool =3D=3D NULL) { > >> >+ RTE_LOG(ERR, USER1, "Mbuf mempool could not be > created\n"); > >> >+ return -1; > >> >+ } > >> >+ > >> >+ cleanup =3D ST_MEMORY_ALLOC; > >> >+ test_data->decomp_buf_pool =3D > >> rte_pktmbuf_pool_create("decomp_buf_pool", > >> >+ total_segs, > >> >+ 0, 0, test_data->seg_sz + RTE_PKTMBUF= _HEADROOM, > >> >+ rte_socket_id()); > >> >+ if (test_data->decomp_buf_pool =3D=3D NULL) { > >> >+ RTE_LOG(ERR, USER1, "Mbuf mempool could not be > created\n"); > >> >+ return -1; > >> >+ } > >> Unless am missing to see it, you need to free pre-allocated memories > >> here before return call for all failed cases. > > > >TJ: There's only one 'freeing stack' at the end of main application > >function to avoid double freeing resources (which was previously n V1). > >We have state machine for that stuff (static enum cleanup_st) to know > what should be free and what has been allocated already. > >In case you mean the state machine is set just after first alloc in line= 136: > > > >cleanup =3D ST_MEMORY_ALLOC; > > > >so we know what should be free at the end of application running in line > 891: > > > >end: > > switch (cleanup) { > > > > case ST_DURING_TEST: > > case ST_PREPARE_BUF: > > free_bufs(test_data); > > /* fallthrough */ > > case ST_MEMORY_ALLOC: > > rte_free(test_data->decomp_bufs); > Even if we are in this state but it doesn't guarantee all of the buffers = in this > state are allocated. So shouldn't every pointer be null-checked before? > Thanks > Shally This check is inside free function already - not need to add double check.= =20 Thx, Tomek