Re: [dpdk-dev] [EXT] [PATCH v7 1/6] app/test-compress-perf: add weak functions for multi-cores test

[Shally Verma <shallyv@marvell.com>]

> -----Original Message-----
> From: Fiona Trahe <fiona.trahe@intel.com>
> Sent: Friday, July 5, 2019 4:46 PM
> To: dev@dpdk.org
> Cc: akhil.goyal@nxp.com; Shally Verma <shallyv@marvell.com>;
> arturx.trybula@intel.com; tjozwiakgm@gmail.com; fiona.trahe@intel.com;
> Tomasz Jozwiak <tomaszx.jozwiak@intel.com>
> Subject: [EXT] [PATCH v7 1/6] app/test-compress-perf: add weak functions
> for multi-cores test
> 
> External Email
> 
> ----------------------------------------------------------------------
...

....
> +enum cperf_perf_test_type {
I think I had feedback here on enum name: it can be just cperf_test_type

> +	CPERF_TEST_TYPE_BENCHMARK,
> +	CPERF_TEST_TYPE_VERIFY
> +};
> +
...
>  struct comp_test_data {
>  	char driver_name[64];
>  	char input_file[64];
Rather than using magic number better use MACRO NAME? also where is limitation mentioned on name sizes?

> -	struct rte_mbuf **comp_bufs;
> -	struct rte_mbuf **decomp_bufs;
> -	uint32_t total_bufs;
> +	enum cperf_perf_test_type test;
> +
>  	uint8_t *input_data;
>  	size_t input_data_sz;
> -	uint8_t *compressed_data;
> -	uint8_t *decompressed_data;
> -	struct rte_mempool *comp_buf_pool;
> -	struct rte_mempool *decomp_buf_pool;
> -	struct rte_mempool *op_pool;
> -	int8_t cdev_id;
> +	uint16_t nb_qps;
>  	uint16_t seg_sz;
>  	uint16_t out_seg_sz;
>  	uint16_t burst_sz;
>  	uint32_t pool_sz;
>  	uint32_t num_iter;
>  	uint16_t max_sgl_segs;
> +
>  	enum rte_comp_huffman huffman_enc;
>  	enum comp_operation test_op;
>  	int window_sz;
> -	struct range_list level;
> -	/* Store TSC duration for all levels (including level 0) */
> -	uint64_t comp_tsc_duration[RTE_COMP_LEVEL_MAX + 1];
> -	uint64_t decomp_tsc_duration[RTE_COMP_LEVEL_MAX + 1];
> -	size_t comp_data_sz;
> -	size_t decomp_data_sz;
> +	struct range_list level_lst;
> +	uint8_t level;
> +
>  	double ratio;
> -	double comp_gbps;
> -	double decomp_gbps;
> -	double comp_tsc_byte;
> -	double decomp_tsc_byte;
> +	enum cleanup_st cleanup;
>  };
> 
>  int
> diff --git a/app/test-compress-perf/comp_perf_options_parse.c b/app/test-
> compress-perf/comp_perf_options_parse.c
> index a7a8c1f9e..74ea81d09 100644
> --- a/app/test-compress-perf/comp_perf_options_parse.c
> +++ b/app/test-compress-perf/comp_perf_options_parse.c
...
> @@ -500,7 +501,6 @@ struct long_opt_parser {  };
> 
>  static struct option lgopts[] = {
> -
...
> -	test_data->level.min = 1;
> -	test_data->level.max = 9;
> -	test_data->level.inc = 1;
> +	test_data->level_lst.min = 1;
> +	test_data->level_lst.max = 9;
> +	test_data->level_lst.inc = 1;
Replace by Macro?

> +	test_data->test = CPERF_TEST_TYPE_BENCHMARK;
>  }
> 
>  int
> diff --git a/app/test-compress-perf/comp_perf_test_common.c b/app/test-
> compress-perf/comp_perf_test_common.c
> new file mode 100644
> index 000000000..dc9d0b0f4
> --- /dev/null
> +++ b/app/test-compress-perf/comp_perf_test_common.c
> @@ -0,0 +1,284 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2019 Intel Corporation
> + */
> +
> +#include <rte_malloc.h>
> +#include <rte_eal.h>
> +#include <rte_log.h>
> +#include <rte_compressdev.h>
> +
> +#include "comp_perf_options.h"
> +#include "comp_perf_test_verify.h"
> +#include "comp_perf_test_benchmark.h"
> +#include "comp_perf.h"
Should this is before comp_perf_xx files to maintain alphabetical order?

> +#include "comp_perf_test_common.h"
> +
...
> +
> +static uint32_t
> +find_buf_size(uint32_t input_size)
> +{
> +	uint32_t i;
> +
> +	/* From performance point of view the buffer size should be a
> +	 * power of 2 but also should be enough to store incompressible data
> +	 */
> +
> +	/* We're looking for nearest power of 2 buffer size, which is greather
Typo: greater

> +	 * than input_size
> +	 */
> +	uint32_t size =
> +		!input_size ? MIN_COMPRESSED_BUF_SIZE : (input_size <<
> 1);
> +
> +	for (i = UINT16_MAX + 1; !(i & size); i >>= 1)
> +		;
> +
> +	return i > ((UINT16_MAX + 1) >> 1)
> +			? (uint32_t)((float)input_size * EXPANSE_RATIO)
> +			: i;
> +}
> +
...
> --
> 2.13.6